Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streaming response early disconnect mode #2687

Conversation

dmitry-mli
Copy link

@dmitry-mli dmitry-mli commented Sep 6, 2024

Summary

Starlette 0.38.4 introduced a bugfix that also introduced a backward incompatible change. This PR keep both the original fix as well as recovers the backward compatibility through parameterization.

#2620 solved issue

#2620 delivered an important fix for BaseHTTPMiddleware which relied on StreamingResponse for chained upstream and downstream communication. Unfortunately, StreamingResponse optimized for early disconnect which resulted into unexpected termination for some of middleware handlers, in case the communication did not manage to propagate through a longer stack (a stack of 4 was enough to reproduce).

#2620 created issue

Change #2620 introduced a backward incompatible change for streaming response from BaseHTTPMiddleware (background tasks were dropped). This impacted software that relied async post-processing.

This PR content

This PR extends StreamingResponse with early_disconnect flag (default True) that controls the early disconnect behavior that is undesirable for the middleware case BaseHTTPMiddleware. The flag is backward compatible for clients of StreamingResponse. The _StreamingResponse is reverted back to inherit from StreamingResponse but this time suppressing the early disconnect mode. Unit tests extended to cover the case.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • [N/A] I've updated the documentation accordingly.

@dmitry-mli dmitry-mli marked this pull request as draft September 6, 2024 21:54
@dmitry-mli dmitry-mli marked this pull request as ready for review September 6, 2024 21:54
@dmitry-mli dmitry-mli marked this pull request as draft September 6, 2024 21:58
@dmitry-mli dmitry-mli force-pushed the streaming-response-early-disconnect-mode branch 4 times, most recently from 345b34b to 7d0bfbe Compare September 6, 2024 22:30
@dmitry-mli
Copy link
Author

dmitry-mli commented Sep 6, 2024

@Kludex @adriangb please review, thank you! This fixes backward incompatible change made in #2620, details in the description.

@dzhulgakov FYI

@adriangb
Copy link
Member

adriangb commented Sep 6, 2024

@Kludex another reason we should do #2176

@adriangb
Copy link
Member

adriangb commented Sep 6, 2024

Thank you for proposing a fix!

Can we go with #2688 instead? It's a much smaller diff / code change.

@dmitry-mli
Copy link
Author

dmitry-mli commented Sep 6, 2024

Sure thing, approved (with comments)!

@Kludex Kludex closed this Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants