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

Fix FileResponse not listening for http.disconnect #1302

Closed

Conversation

idotobi
Copy link

@idotobi idotobi commented Oct 7, 2021

Fixes #1301

Summary

  • implement aborting FileResponse stream on http.disconnect similar to
    StreamResponse
  • implement on_complete Background Tasks for
    StreamResponse/FileResponse
    • background: always executed (even if stream aborted)
    • on_complete: only executed if stream not abored by client
      I initially assumed that background would work as on_complete is now
      implemented. However as this is not the case, adding on_complete
      seemed like the best choice to ensure backwards compatibility.
  • implement testsuite for abort scenarios

On_complete

I'm aware that on_complete rather counts as a new feature, however it was very useful in writing the test and I would consider it a reasonable feature (or even default behaviour 🤔 ).
Consider it as a basis for discussion. If you prefer I'm happy to separate it out in a different PR.

Further improvements

It might also make sense to implement FileResponse as a child class of Streaming Response, as they share quite some code.
If you agree I'd be happy to implement this.

@idotobi idotobi force-pushed the fix/1301-not-listening-to-disconnect branch 3 times, most recently from 1252e2a to c0dcf34 Compare October 9, 2021 11:55
@idotobi
Copy link
Author

idotobi commented Oct 9, 2021

@Kludex : Thanks for approving the workflow. I updated the import sorting to fix the failing linting.

Is there an easy way for me to get all checks passing in my fork? I don't want to bug you too much with my attempts to make the checks pass and keep the iteration cycles small 😅

@Kludex
Copy link
Sponsor Member

Kludex commented Oct 9, 2021

I don't think there is a way for that 🤔

@idotobi
Copy link
Author

idotobi commented Oct 9, 2021

Ok. No worries, then I'll try to run the checks locally. Thanks for the quick reply 👍

 - implement aborting FileResponse stream on http.disconnect similar to
   StreamResponse
 - implement on_complete Background Tasks for
   StreamResponse/FileResponse
    - background: always executed (even if stream aborted)
    - on_complete: only executed if stream not abored by client
   I initially assumed that background would work as on_complete is now
   implemented. However as this is not the case, adding on_complete
   seemed like the best choice to ensure backwards compatibility.
 - implement testsuite for abort scenarios
@idotobi idotobi force-pushed the fix/1301-not-listening-to-disconnect branch from c0dcf34 to ff5e3c7 Compare October 9, 2021 15:55
@idotobi
Copy link
Author

idotobi commented Oct 9, 2021

@Kludex : Should pass the tests now 🤞

Warnings:
 - change invalid permissions after test as otherwise pytest cannot
   clean the tmpdir

Errors:
 - get free tcp/udp port dynamically for test_response_abort
 - use global variable for defining sleep values
@idotobi
Copy link
Author

idotobi commented Oct 23, 2021

Ok, I think I would need some help to get the test to run consistently.

The problem I'm trying to solve is the following:
I need to abort an ongoing streaming request while it's running.
It seems that only requests really supports this as both the TestClient as well as httpx seem to block until the full response is received, even if stream=True is set.

My current approach is:
I use two processes:

  • one for serving starlette via uvicorn
  • one for calling requests

However, this is not really pretty as it is very sensitive to timing (seems to not play well with azure pipelines) and it basically only tests starlette when served by uvicorn.

Does someone have an idea for a better approach? Did I simply use httpx/TestClient wrong?

@Kludex
Copy link
Sponsor Member

Kludex commented Nov 27, 2021

@idotobi Thanks for the PR! 😄

This PR is huge, and it aims to address multiple things. It will be great if you can prepare the PR that solves #1301 first. I've already confirmed the issue, and the PR is more than welcome.

As for other things you mentioned on this PR, I think a discussion on Gitter or GitHub discussions is better before trying to jump into a PR for them. :)

@Kludex Kludex closed this Nov 27, 2021
@idotobi
Copy link
Author

idotobi commented Nov 28, 2021

This PR is huge, and it aims to address multiple things. It will be great if you can prepare the PR that solves #1301 first. I've already confirmed the issue, and the PR is more than welcome.

@Kludex : Fair point. Will do so during the week.

@idotobi
Copy link
Author

idotobi commented Jan 22, 2022

Took a bit longer than a week 😅 #1427

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.

FileResponse Stream continues streaming after http.disconnect
2 participants