Design an API to support aborting requests

[natebosch]

On the web HttpRequest supports abort: https://api.dart.dev/stable/2.8.1/dart-html/HttpRequest/abort.html

We have a proposal for adding abort on the dart:io HttpClientRequest: https://dart-review.googlesource.com/c/sdk/+/147339

We need to come up with a design for supporting abort through the interfaces in this package and validate that we can build it on both dart:html and dart:io implementations.

The easiest option might be to add a method on StreamedResponse. There is some precedent for that with IOStreamedResponse.detachSocket.

http/lib/src/io_streamed_response.dart

Line 36 in 9063ba3

Future<Socket> detachSocket() async => _inner.detachSocket();

(we need to make sure the differences between these are clearly documented.

I do wonder if we need to consider some sort of support for the more convenient methods than send though...

cc @kevmoo @jakemac53 @grouma @zichangg @lrhn for thoughts

[zichangg]

The easiest option might be to add a method on StreamedResponse.

The proposed dart:io's abort() is a method in Class HttpClientRequest. The method on StreamedResponse should not be able to access abort().(Technically you can, but it won't make a difference as the response has already come back) I think the only use case for abort() is timing out the request if it takes too long. We can adapt `send` to accept an optional parameter `duration` and possibly a callback to be fired when timing out.

…

On Thu, May 28, 2020 at 12:52 PM Nate Bosch ***@***.***> wrote: On the web HttpRequest supports abort: https://api.dart.dev/stable/2.8.1/dart-html/HttpRequest/abort.html We have a proposal for adding abort on the dart:io HttpClientRequest: https://dart-review.googlesource.com/c/sdk/+/147339 We need to come up with a design for supporting abort through the interfaces in this package and validate that we can build it on both dart:html and dart:io implementations. The easiest option might be to add a method on StreamedResponse. There is some precedent for that with IOStreamedResponse.detachSocket. https://github.com/dart-lang/http/blob/9063ba379b7e7386cced830feb27cc7a5fd3ec7e/lib/src/io_streamed_response.dart#L36 (we need to make sure the differences between these are clearly documented. I do wonder if we need to consider some sort of support for the more convenient methods than send though... cc @kevmoo <https://github.com/kevmoo> @jakemac53 <https://github.com/jakemac53> @grouma <https://github.com/grouma> @zichangg <https://github.com/zichangg> for thoughts — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#424>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AK3IVS7MXK2PWZYCPT4NA7TRT26GZANCNFSM4NNMB7PA> .

-- Best Regards, Zichang

[natebosch]

Here are a couple other options - I'm not advocating for these, just trying to brainstorm. Both of these are breaking changes for anyone with implements http.Client.

Allow passing a Future back to the client to cancel it. We'd have some state that lets us abort if you give us back the exact same instance.

var response = client.get(url);
// something happens
client.abort(response);

Add a named argument to all the methods:

var abortCompleter = Completer<void>();
var response = client.get(url, abortOn: abortCompleter.future);
// something happens
abortCompleter.complete();

The latter could pair well with a timeout argument. Passing timeout: Duration(seconds: 5) would have the same effect as passing abortOn: Future.delayed(Duration(seconds: 5))

[grouma]

I like the latter assuming a future that completes after the request has completed doesn't cause an issue.

[liranzairi]

Hey, in my app I want to give the user an option to abort while uploading a file. I saw that dart:io is going to support it soon. However I use http package in my app so I looked here and saw this thread. Do you think you'll support it too, soon? Thanks

[jakemac53]

I think having an abort method on the response itself is really the ideal design here, for a few reasons:

• It doesn't require the Client to keep track of outgoing requests. This is cleaner, more memory efficient, and less likely to result in memory leaks.
• It is more intuitive as a user of the API - all the useful functionality is encapsulated on the response itself - you can pass that one object around to other parts of the code and they can decide to cancel it, etc. They don't also need a client, and you can decouple the knowledge about when to cancel from the service making the actual requests.
• It allows you to synchronously cancel requests, instead of waiting an arbitrary amount of time for the event queue to catch up. This helps to mitigate race conditions between the cancellation and completion of the request, and it helps to waste less data by cancelling the request sooner.

This does get a bit trickier to implement - you are left with a few not so great options:

• Implement Future with some custom type, which beyond being considered an anti-pattern also raises some difficult questions around how you handle the cancel case. I think for this you could just complete with a special type of error, and not try to do what CancelableOperation does (with a special onCancel callback that complicates error handling).
• Make a user-facing breaking change in the api to return some new type (possibly CancelableOperation).

[natebosch]

Do you think you'll support it too, soon?

The trickiest part of this is rolling it out, I think no matter what design we use it's going to be a breaking change for anyone with a class which implements Client.

[natebosch]

• It doesn't require the Client to keep track of outgoing requests. This is cleaner, more memory efficient, and less likely to result in memory leaks.

I don't think that will be difficult in any case. We don't need an extra collection - we can handle this all at the point where we have the HttpClientRequest or the HttpRequest.

• It is more intuitive as a user of the API - all the useful functionality is encapsulated on the response itself - you can pass that one object around to other parts of the code and they can decide to cancel it, etc. They don't also need a client, and you can decouple the knowledge about when to cancel from the service making the actual requests.

Attaching it to a StreamedResponse has the downside of pushing more users onto that API over http.get. I think cancellation (usually timeout) is a common enough use case that we want to support it on those APIs as well. Changing the return type from Future to CancelablleOperation is more breaking, and I'm not entirely convinced one way or the other whether it's a better API if we were starting from scratch.

• It allows you to synchronously cancel requests, instead of waiting an arbitrary amount of time for the event queue to catch up. This helps to mitigate race conditions between the cancellation and completion of the request, and it helps to waste less data by cancelling the request sooner.

I would expect that the typical case cancelations only happen after some async trigger either way. I think at most we could shave a microtask or 2, but I don't think that gives much power to typical usages. Which likely can't make many guarantees about behavior even if we give them a synchronous cancel.

[natebosch]

I'm running with the abortOn approach for now.

One thing that's tricky here, I didn't realize the dart:io implementation causes the done future to complete as an error as opposed to never completing. I'm pretty sure the implementation in the browsers wouldn't do that.

I think it would be easier to build the error behavior in the browser than to build the never-complete behavior for dart:io but probably either is possible.

I don't have a strong opinion between completing as error on abort, or never completing on abort. One nice thing about the error approach is that if you do want to forward an error object and stack trace through it's possible to do so.

cc @lrhn for opinions on completing as error vs never completing for aborted requests.

[jakemac53]

I really don't think abortOn with a Future type is the right api here... it has a lot of downsides. Specifically since you can't cancel the future at all I think it will be prone to memory leaks, hung processes on exit, and other similar issues.

How about just supporting the timeout argument which we can ensure is implemented better (using a timer which we can cancel) as opposed to a delayed future?

[aytunch]

This is a must have feature. A lot of community backed packages are using this package at their core. And because there is not yet a abort functionality for downloads, there are huge amount of wasted data for Flutter users. This is really bad user experience when users check their data usage on their phones.

I am not sure why people are so concerned about breaking changes. Flutter is still new. Breaking changes are to be expected and for a core functionality like this, I am sure devs wont complain.

Imho what devs would really complain about could be a platform with APIs so hard to understand because they were patched for every new feature instead of getting redesigned. If we want Flutter to remain and feel fresh and different from the old and slow SDks then we have to live with breaking changes for a while.

Personally, the lack of aborting is a big deal for us because we have a video heavy app and we are using flutter_cache_manager to pre-download some of the videos in the list. If the user scrolls even further, we would like to abort the started downloads and start new ones. But for now we are stuck with 10's of started downloads which will be just byte waste.