Add thread event primitives to track server start/exit

[rmorshea]

Add did_start/did_exit thread events to ServerState and turn Server.started into alias of Event.is_set - downstream applications can then hook into Server.server_state.did_* events to wait() for certain state changes.

While this does not strictly solve #742 it makes it easier to implement a run_in_thread style method.

Instead of writing:

import contextlib
import time
import threading
import uvicorn

class Server(uvicorn.Server):
    def install_signal_handlers(self):
        pass

    @contextlib.contextmanager
    def run_in_thread(self):
        thread = threading.Thread(target=self.run)
        thread.start()
        try:
            while not self.started:
                time.sleep(1e-3)
            yield
        finally:
            self.should_exit = True
            thread.join()

You would do:

import contextlib
import uvicorn

@contextlib
def run_in_thread(server):
    ...

class Server(uvicorn.Server):
    def install_signal_handlers(self):
        pass

    @contextlib.contextmanager
    def run_in_thread(self):
        threading.Thread(target=self.run).start()
        try:
            self.server_state.did_start.wait()
            yield
        finally:
            self.should_exit = True
            self.server_state.did_exit.wait()

[SommerEngineering]

All changes are implemented correctly and without issues from my point of view.

[rmorshea]

@SommerEngineering it looks like you haven't given explicit approval in the review, is there anything that should be fixed?

[SommerEngineering]

@rmorshea In my opinion, nothing else needs to be done 🙂 I did not approve because I am unsure what the rules are in this repository. The following rules are linked on the PR page: CONTRIBUTING.md

Unfortunately, the rules do not say anything about this point 🙁 Can anyone give me some advice on this?

[rmorshea]

Hmm, looks like I've gotta push this through some chat channels with maintainers.

[euri10]

I have mixed feelings about this, the use of threading events in the server state is far from being an obvious choice to me, on top of that I dont know how this will play when we use multiple process or the reload machinery (nor are there tests to check that), and finally as you said it doesn't solve the issue in the first place, so maybe we should take a step back and as @florimondmanca said agree on the API people would likely see and mostly if we should support that use case.

[rmorshea]

mostly if we should support that use case.

@euri10 I see ample evidence to suggest that there's a need for an interface which would allow users to more easily run the server in a thread. #742 is a rather long lived issue that has seen relatively consistent activity since its creation. Additionally, as I pointed out in some out-of-band conversations, in many testing scenarios (i.e. when using sync tools Selenium) there isn't actually an alternative - you have to run the server in a thread.

While I can understand not wanting to add these primitives to make things easier in the short-term. In the long-run, I think there's good reasons to develop some interface that would meet these needs, if only to prevent people from having to subclass the server as is suggested in #742.