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

Request cancellation/abort through cancellation-token #602

Closed
wants to merge 4 commits into from

Conversation

FaFre
Copy link

@FaFre FaFre commented Aug 2, 2021

This is an effort trying to tackle the problems discussed in #567, #424, and #204.

This solution introduces a CancellationToken, which is passed into the http-methods of Client e.g.:

http.get(url,
      cancellationToken: http.CancellationToken(autoDispose: false));

This design is avoiding breaking changes.

Despite most cancellation-token-based solutions (working with a simple Completer), this approach is also trying to really abort()a request if necessary.

It is possible to add multiple requests to a CancellationToken, to cancel them at once when triggered (disposal of the token has to get done manually in that case).

A request can (if requested) get aborted at multiple points:

  1. Before the request is getting prepared
  2. When the request is getting registered at the CancellationToken
  3. When the request is running, through a Stream-based invoke of abort()

Status:
Cancellation is supported and implemented for both IOClient and BrowserClient.
Existing tests have been fixed, but no new tests written yet.
It will need some further reviews.

Problems:
The abort() on HttpRequest (for web) doesn't seem to have any effect during my tests (request time < 800ms) - although it's called during the according readyState (https://api.flutter.dev/flutter/dart-html/HttpRequest/abort.html)

@google-cla
Copy link

google-cla bot commented Aug 2, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Aug 2, 2021
@natebosch
Copy link
Member

@FaFre - did you have a chance to check out the CLA agreement linked above?

@FaFre
Copy link
Author

FaFre commented Aug 3, 2021

@natebosch - haven't had the time to read the agreement yet. Will do it today or tomorrow.

@FaFre
Copy link
Author

FaFre commented Aug 4, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Aug 4, 2021
@aytunch
Copy link

aytunch commented Dec 4, 2021

@FaFre Thank you very much for this Draft and PR
This is a very important feature.
Just to make sure I have a question:
if user is downloading a big file and they are at 50%
when the download is cancelled, the rest of the file will not be downloaded right?
Also does Firebase Storage give the necesaary server support out of the box?
Thanks Fabian.

@FaFre
Copy link
Author

FaFre commented Dec 4, 2021

@aytunch I'm happy you find use in it 👍
Well, it depends. The provided API is pretty high level. The only option we have is to call abort, but the definition, what actually happens with the underlying TCP connection is not given and will probably also vary depending on the underlying platform libraries/native client.

You need to check that on your own with an package sniffer on each platform you want to provide your app for (unless somebody did already research on that...). I'm not familiar with Firebase storage but I assume you just have an basic HTTP-Download. In that case it should be enough to close the TCP connection to stop the download. And this is what you need to confirm with an package sniffer.

@aytunch
Copy link

aytunch commented Dec 5, 2021

I understand @FaFre thanks
Do you have an ETA for this PR?

@FaFre
Copy link
Author

FaFre commented Dec 5, 2021

@aytunch
I use the feature already in my App's since a couple of months and didn't encounter any problems with it (not targeting web).

However, the implementation for web may need some further investigation. Although I implemented it like in the documentation described, my requests didn't get cancelled accordingly. I couldn't fix that on my own so far, but it's also possible that this effect is caused by the dart libraries and I do everything correctly here.

@FaFre
Copy link
Author

FaFre commented Dec 7, 2021

@aytunch While doing some research on the web request I actually discovered that also the dart:io implementation isn't able to abort requests once the server started sending data.

The implementation seems to see a request as done once the server starts sending data. Therefore they cannot get aborted anymore.

final bigFileUrl = Uri.https('ftp5.gwdg.de',
      '/pub/misc/openstreetmap/planet.openstreetmap.org/pbf/planet-latest.osm.pbf'); //~60GB file
  final req = await HttpClient().getUrl(bigFileUrl);
  unawaited(Future.delayed(const Duration(seconds: 1)).whenComplete(() {
    req.abort();
    print('sent abort');
  }));

  final resp = await req.close();
  print(await resp.toList());

The code above confirms with DevTools that the HTTP request is still running and memory is filling up.

@natebosch I saw you also did some research on abort() and opened some issues regarding it. Is this behavior already discussed somewhere?

@aytunch
Copy link

aytunch commented Dec 7, 2021

@FaFre Have you seen this issue?
#424

@natebosch
Copy link
Member

natebosch commented Dec 7, 2021

Is this behavior already discussed somewhere?

I think once we have a response, instead of request.abort() we need to use response.detachSocket().

https://github.com/dart-lang/http/pull/521/files#diff-7a772406b9a2c46d9b0b95da73d10dd5ad9807fa78c9519bac0d247690482957R83

I do have plans to revisit timeouts and request aborting soon. I suspect we won't be able to use this PR because it would be a breaking change that might be too hard to roll out, but I have not yet finished exploring the implications of various designs.

@JonathanPeterCole
Copy link

I've been working on a fork with cancellation using a CancellationToken, but I've taken a different approach with the tokens so they're not specific to HTTP requests: https://github.com/JonathanPeterCole/dart-cancellation-token-http
I haven't had a chance to properly test this implementation in a Flutter app yet though.

@mosuem
Copy link
Member

mosuem commented Mar 12, 2024

Closing this, as it is stale and the files are out of date. Feel free to reopen in case you still want to land it!

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

Successfully merging this pull request may close these issues.

6 participants