search

sysid / sse-starlette

BSD 3-Clause "New" or "Revised" License
505 stars 36 forks source link

Replace listen_for_exit_signal anyio.sleep with anyio.Event #45

Closed jakkdl closed 1 year ago

jakkdl commented 1 year ago

Heya, I was requested by @Zac-HD to replace the busy-wait in the signal handler with an Event. Found it a bit tricky since anyio don't know what backend it's using at import time so ended up initializing the Event in the signal listener, and to be thread safe (which I'm not 100% sure is necessary, but doesn't hurt) I had to fiddle a little.

The tox tests all work well, and after way too long I realized I had to open 127.0.0.1:8000 in a web browser for conditional_yielding_endpoint.py and the examples to work. And afaict it should all work as before. Fixed #pragma: no cover on the relevant lines not touched by the tests, the race condition doesn't get tested by the examples/manual tests either. No mypy complaints on the code I touched (but 2 errors in existing code). A lot of the make commands are broken for me, but afaict I've checked for most/all that they would.

sysid commented 1 year ago

@jakkdl , thanks for your nice work! Can you just give me a bit more context where this requirement is coming from? It looks at first glance as adding more complexity for essentially the same purpose. The sleep is not really a busy-wait, is it?

BTW: If you have ideas to improve the tests, I would love to hear them.

Zac-HD commented 1 year ago

The current sleep-and-poll approach adds up to a second of latency, which isn't much in production but can be a big deal for tests. Reducing that interval makes it more like a busy-wait, whereas this Event-based approach has no extra latency and does no work at all while waiting!

jakkdl commented 1 year ago

Right, calling it a busy-wait was maybe a bit of a misnomer. Will open a separate issue with some comments on tests :)