fix!: ensure that generator clean up is correctly propagated

[winstxnhdw]

Description

As discussed on Discord, we should ensure that synchronous generators are properly cleaned up. Async generators will be cleaned up, so we should do the same for synchronous generators as well.

The following snippet will now properly print "cleaned up inner" on premature disconnects.

from litestar import get
from litestar.response.sse import ServerSentEvent
from litestar.background_tasks import BackgroundTask
import time

@get('/test', sync_to_thread=True)
def test() -> ServerSentEvent:
    def inner_gen():
        try:
            for i in range(10):
                yield str(i)
                time.sleep(1)
        finally:
            print('cleaned up inner')

    def gen():
        igen = inner_gen()
        try:
            yield from igen
        except ClientDisconnectException:
            return 
        finally:
            igen.close()

    g = gen()
    return ServerSentEvent(g)

Closes #3772

[codecov]

Codecov Report

❌ Patch coverage is 79.59184% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.78%. Comparing base (d478219) to head (12ef3f3).

Files with missing lines	Patch %	Lines
litestar/response/streaming.py	73.91%	5 Missing and 1 partial ⚠️
litestar/utils/sync.py	84.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4394      +/-   ##
==========================================
- Coverage   97.83%   97.78%   -0.05%     
==========================================
  Files         296      296              
  Lines       15286    15315      +29     
  Branches     1711     1716       +5     
==========================================
+ Hits        14955    14976      +21     
- Misses        189      197       +8     
  Partials      142      142              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[winstxnhdw]

I believe this is closer to what you want? We make AsyncIteratorWrapper into an actual async generator. Then handle the clean up within the SSE wrapper. This way the wrapper remains agnostic. We also proactively handle the clean up for both async and sync generators.

In the end, this should probably be considered a fix since this fixes a discrepancy in clean up between async and sync generators.

[winstxnhdw]

Finally found the issue with the test. The SSE needs to continuously yield something otherwise the generator cannot handle the athrow. Is this something we should be worried about?

[winstxnhdw]

Marking this as breaking again because async generators will no longer clean up by default as we no longer call CancelScope.

[winstxnhdw]

This line is still necessary for asend because asend needs a return value.

[github-actions]

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/4394

[provinzkraut]

This fundamentally changes the functionality of AsyncIteratorWrapper. It's supposed to wrap a synchronous iterable / iterator, and turn it into an async iterable. Now with the added functionality of also wrapping instances of itself (?), it gets really confusing.

[provinzkraut]

Again, I do not understand why you have to be so over specific here. It should be good enough to simply check:

• Is it an async-generator? athrow
• Is it an async-iterable? Just break

[winstxnhdw]

The issue here is how _ServerSentEventIterator is designed. The content is kept in self.content_async_iterator, which means to close the original generator from the handler, we need to specifically throw self.iterator.content_async_iterator because self.iterator will only contain the header chunks like event_id, event_type, etc.

[provinzkraut]

_ServerSentEventIterator should handle that though, not the code that's calling it. It knows about its internal state, and is best suited to decide where to throw what exception. From the outside, it should be treated as any other async iterator / generator.

[provinzkraut]

Hey @winstxnhdw, thanks for bringing up this issue and attempting to fix it!

I know you've invested some time into this, so I'm sorry to say it, but I'll be closing this PR now though, since it doesn't seem to be getting into a state where I'd consider it mergeable, despite me sinking a lot of time into reviews, suggestions and conversations - time that could also have been spent fixing other (or this) bugs.

Should you want to contribute again in the future, I suggest you try to familiarise yourself with the material a bit more, and approach changes with a more holistic view, i.e. think deeper about the side effects your changes might have, and the intention behind them. To that end, I can also recommend simply reaching out on our discord server, where we'll be happy to answer all questions :)

[euri10]

you can also just put your PR as a draft and ask for anothers their pov if needed, I do that often when I try things and dont want to "pollute" other's workflow, which happens often for me, some PRs lead nowhere, as a draft it doesnt impact anyone else other than me.