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

Cancel streamed downloads & fix progress for downloads #2032

Closed
wants to merge 5 commits into from

Conversation

kuhnroyal
Copy link
Member

@kuhnroyal kuhnroyal commented Nov 13, 2023

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

@AlexV525 AlexV525 mentioned this pull request Nov 13, 2023
7 tasks
@AlexV525
Copy link
Member

Before review: Could we abstract the implementation of this cancellation above the specific adapter? If we're doing this in adapters, then every adapter needs to consider the same functionality.

@kuhnroyal
Copy link
Member Author

I will see what I can do, but I already know that this is basically not supported on web.

@AlexV525
Copy link
Member

I will see what I can do, but I already know that this is basically not supported on web.

Sure, but we have thousands of adapters around the world. :)

@kuhnroyal
Copy link
Member Author

Before review: Could we abstract the implementation of this cancellation above the specific adapter? If we're doing this in adapters, then every adapter needs to consider the same functionality.

This is hard without breaking. If we want to do this in the DioMixin we have no access to the underlying http implementation and can not close the stream/connection. We would have to pass some new argument to the adapters or extend ResponseBody with a new close function. But this seems like an issue for dio 6.

@kuhnroyal
Copy link
Member Author

This is a lot more complex. We have completely different behavior for different adapters when doing streamed responses/downloads and most of it can not be abstracted away.

For IOHttpClientAdapter we have a ResponseBody with a stream so the await dio.get succeeds after the connection is established with a Response and the cancellation exception is thrown on the stream.

For BrowserHttpClientAdapter we have a ResponseBody with bytes and the adapter has to wait until everything is received, before completing with a response. So the cancellation exception is thrown on the await dio.get.
And there is no way to access other information from the XHR when aborting.

@kuhnroyal kuhnroyal marked this pull request as ready for review November 14, 2023 13:07
@kuhnroyal kuhnroyal requested a review from a team as a code owner November 14, 2023 13:07
@AlexV525
Copy link
Member

What about the HTTP/2 adapter?

@kuhnroyal
Copy link
Member Author

What about the HTTP/2 adapter?

Fixed it there as well. I think we should merge this and brain storm if we can abstract this away separately.

@AlexV525
Copy link
Member

Fixed it there as well.

Marvelous!

AlexV525
AlexV525 previously approved these changes Nov 14, 2023
Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

/// Cancel any up/download streams and connections
/// if the [CancelToken] is cancelled
/// and propagate the [DioException] to the response stream.
cancelFuture?.whenComplete(() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have asynchronous gaps between this call and other awaits. Can we do this earlier or separated?

@AlexV525 AlexV525 dismissed their stale review November 15, 2023 08:08

For new comment

@AlexV525 AlexV525 added p: dio Targeting `dio` package s: bug Something isn't working platform: io s: feature This issue indicates a feature request labels Nov 25, 2023
@AlexV525
Copy link
Member

@kuhnroyal Is this still on your track? The last time I review this it only remains a few nits.

@kuhnroyal
Copy link
Member Author

@kuhnroyal Is this still on your track? The last time I review this it only remains a few nits.

Yea

@kuhnroyal
Copy link
Member Author

Superseeded by #2068 due to lots of updates on main

@kuhnroyal kuhnroyal closed this Dec 17, 2023
@kuhnroyal kuhnroyal deleted the feat/cancel-stream branch December 22, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: dio Targeting `dio` package platform: io s: bug Something isn't working s: feature This issue indicates a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CancelToken be able to cancel response streams from ResponseType.stream requests
2 participants