sse-starlette 1.6.1 appears to leak asyncio.Lock across instantiations

[rra]

With sse-starlette 1.6.1, I'm getting new test failures that all look like this:

.tox/py/lib/python3.11/site-packages/sse_starlette/sse.py:237: in __call__
    async with anyio.create_task_group() as task_group:
.tox/py/lib/python3.11/site-packages/anyio/_backends/_asyncio.py:662: in __aexit__
    raise exceptions[0]
.tox/py/lib/python3.11/site-packages/sse_starlette/sse.py:240: in wrap
    await func()
.tox/py/lib/python3.11/site-packages/sse_starlette/sse.py:215: in listen_for_exit_signal
    await AppStatus.should_exit_event.wait()
.tox/py/lib/python3.11/site-packages/anyio/_backends/_asyncio.py:1842: in wait
    if await self._event.wait():
/opt/hostedtoolcache/Python/3.11.3/x64/lib/python3.11/asyncio/locks.py:210: in wait
    fut = self._get_loop().create_future()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <asyncio.locks.Event object at 0x7ff6f15d1bd0 [unset]>

    def _get_loop(self):
        loop = events._get_running_loop()
    
        if self._loop is None:
            with _global_lock:
                if self._loop is None:
                    self._loop = loop
        if loop is not self._loop:
>           raise RuntimeError(f'{self!r} is bound to a different event loop')
E           RuntimeError: <asyncio.locks.Event object at 0x7ff6f15d1bd0 [unset]> is bound to a different event loop

I think this may be related to #57. It looks like the should_exit_event may not be properly cleared between tests, which causes it to fail because each test uses a separate event loop.

Reverting to sse-starlette to 1.6.0 makes this problem go away again.

[sysid]

Hi @rra, thanks for reporting. However, I cannot reproduce the error.

I know that the original PR did not have appropriate tests, but I think I have fixed this before merging. You can check the GH actions where the tests are passing.

Running make test does not fail on my machine or in the CI/CD pipeline.

[rra]

I unfortunately don't know how to minimize a test case for you, but here are my GitHub Actions failures:

https://github.com/lsst-sqre/jupyterlab-controller/actions/runs/5017943281/jobs/8996712105

The first test that consumes progress events passes, and then the subsequent ones all seem to fail with this error. The only thing I changed to make the error disappear again was to downgrade sse-starlette to 1.6.0.

The use of sse-starlette seems to be fairly simple and straightforward; I don't think I'm doing anything weird. The async iterator is here:

https://github.com/lsst-sqre/jupyterlab-controller/blob/b600a73dba3caa9f5a1afc9a88af9a3c06e65fed/src/jupyterlabcontroller/services/state.py#L132

and the handler is here:

https://github.com/lsst-sqre/jupyterlab-controller/blob/b600a73dba3caa9f5a1afc9a88af9a3c06e65fed/src/jupyterlabcontroller/handlers/labs.py#L134

The actual translation into a ServerSentEvent object happens here, but is very simple:

https://github.com/lsst-sqre/jupyterlab-controller/blob/b600a73dba3caa9f5a1afc9a88af9a3c06e65fed/src/jupyterlabcontroller/models/v1/event.py#L37

[sysid]

I had the same error and it is easy to fix: Reset should_exit_event in your tests. I created a fixture for that:

@pytest.fixture
def reset_appstatus_event():
    # avoid: RuntimeError: <asyncio.locks.Event object at 0x1046a0a30 [unset]> is bound to a different event loop
    AppStatus.should_exit_event = None

[elonzh]

reset_appstatus_event fixture is not working for me.

[sysid]

This is strange. Re-init of reset_appstatus_event at the beginning of every did the trick for me. In any case, the issue is purely test related. In any real world app there should be no re-use of event-loop with partially initialised eventsource.

However, I am not sure how I can help. I experienced exactly the same issue after merging 57, but for me the fixture resolved the problem.

[rra]

For the record, the approach that I went with was to add:

AppStatus.should_exit_event = None

to the shutdown event handler for my FastAPI webapp. I realize that it's probably a test-specific problem and I could have used a test fixture instead, but this felt a bit cleaner, since when the FastAPI app is shutting down I'd like to discard all state, since it won't be used again until it's reinitialized. (This may not work for other people if their application is juggling multiple FastAPI apps that may be using sse-starlette.)

Thanks for the help!

[sysid]

Thanks for the feedback @rra ! Glad you could solve your problem.

[msukmanowsky]

Don't love that there's this workaround but for reference, this is the fixture that we ended up using to address the same issue

@pytest.fixture
def reset_sse_starlette_appstatus_event():
    """
    Fixture that resets the appstatus event in the sse_starlette app.

    Should be used on any test that uses sse_starlette to stream events.
    """
    # See https://github.com/sysid/sse-starlette/issues/59
    from sse_starlette.sse import AppStatus

    AppStatus.should_exit_event = None

[sysid]

Thanks for sharing @msukmanowsky .

[hamacekh]

The issue is still present on sse-starlette 2.0.0, but the workaround works.

[arnoan]

For the record:
I just ran into the same issue (sse-starlette==2.1.3) and it took me quite a long time trying to find a solution to this. I assumed the error must be on my end.. Luckily by chance I eventually did stumble on this and the solution proposed by @msukmanowsky also solves it for me 🎉🙏

@sysid
I am very grateful for this package. And I agree that this issue "only" affects tests. But I feel this is quite a big trap. The moment someone implements more than one SSE endpoint in their application and tries to put them under test coverage (as ideally they should) they will get bitten by this with no easy way to find a fix.

Would you be open to include a small section in the project's readme to guide users in the right direction if they run into this problem while building their tests?

[arnoan]

I proposed a small PR to include guidance on this in the Readme:
#118