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

Add listener for disconnect to FileResponse #1427

Conversation

idotobi
Copy link

@idotobi idotobi commented Jan 22, 2022

This fixes #1301.

This makes FileResponse behave the same way as StreamResponse,
namely aborting sending data once a http.disconnect is received from the client.

Note that this behaviour is currently not tested in StreamResponse.
I found one way to test this behaviour. However, it would require to add httpx as a testing requirement hence it is not part of the PR yet.
FileResponse is even more tricky to test without having a way to mark a FileResponse as finished (would require a new property).

This makes FileResponse behave the same way as StreamResponse,
namely aborting sending data once a "http.disconnect" is received.
@idotobi
Copy link
Author

idotobi commented Jan 22, 2022

Note: The code for __call__ and listen_for_disconnect are implemented exactly as in StreamResponse. It might be worthwhile to make FileResponse inherit from StreamResponse in the future as they are quite similar in nature.

@Kludex
Copy link
Sponsor Member

Kludex commented Jan 23, 2022

Interesting. We don't have a specific test for StramingResponse as well, for the same case.

@Kludex
Copy link
Sponsor Member

Kludex commented Jan 29, 2022

I found one way to test this behaviour. However, it would require to add httpx as a testing requirement hence it is not part of the PR yet.

@idotobi How?

@adriangb adriangb added feature New feature or request clean up Refinement to existing functionality labels Feb 2, 2022
@Kludex Kludex added bug Something isn't working and removed feature New feature or request labels Feb 2, 2022
@tomchristie
Copy link
Member

Hiya @idotobi - thank you for the neatly tackled bit of work here.

I'd like to close this off with the justification as outlined on #1301

There's always a complexity trade-off in these kinds of cases, and my feeling is that Starlette ought to lean very strongly towards as-simple-as-possible-wherever-possible.

(Something we need to get better at as a team is making sure we're fielding incoming requests like this, so that you're not sending time on neatly done pull requests like this one, and then having them closed off. We're working on that.)

@tomchristie tomchristie closed this Feb 3, 2022
@idotobi
Copy link
Author

idotobi commented Feb 3, 2022

@Kludex : It was a rather messy prototype, where I had to remove some asserts within the httpx testclient, so I wouldn't recommend it for production usage.

The idea was basically to exchange the receive callback in the app definition

    async def app(scope, receive, send):
        async def receive() -> dict:
            return {"type": "http.disconnect"}

This way the server would abort the connection itself, if it listened to http.disconnect.
...As I said. Not pretty but sadly there is to Testclient I found that was able to send disconnect while waiting on the response out-of-the-box.

@idotobi
Copy link
Author

idotobi commented Feb 3, 2022

@tomchristie : No worries. I learned a bit on the way, so it's no time wasted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working clean up Refinement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileResponse Stream continues streaming after http.disconnect
4 participants