Skip to content

Streaming response early disconnect mode#2687

Closed
dmitry-mli wants to merge 1 commit intoKludex:masterfrom
dmitry-mli:streaming-response-early-disconnect-mode
Closed

Streaming response early disconnect mode#2687
dmitry-mli wants to merge 1 commit intoKludex:masterfrom
dmitry-mli:streaming-response-early-disconnect-mode

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 dmitry-mli force-pushed the streaming-response-early-disconnect-mode branch from 7d0bfbe to 5cd92cd Compare September 6, 2024 22:32
@dmitry-mli dmitry-mli marked this pull request as ready for review September 6, 2024 22:34
@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
Contributor

adriangb commented Sep 6, 2024

@Kludex another reason we should do #2176

@adriangb
Copy link
Contributor

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