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 abort (or similar) to HttpClientRequest #22265

Closed
kevmoo opened this issue Feb 4, 2015 · 43 comments
Closed

Add abort (or similar) to HttpClientRequest #22265

kevmoo opened this issue Feb 4, 2015 · 43 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug

Comments

@kevmoo
Copy link
Member

kevmoo commented Feb 4, 2015

HttpRequest (in dart:html) has an abort() method

There is a request to add a feature to the http package to support timeout.

This can be done with dart:html, but not dart:io

@kevmoo
Copy link
Member Author

kevmoo commented Feb 4, 2015

Marked this as blocking #19831.

@kevmoo
Copy link
Member Author

kevmoo commented Feb 5, 2015

Added C11 label.

@sgjesse
Copy link
Contributor

sgjesse commented Feb 6, 2015

When creating a HTTP request in dart:io we have the following steps:

HttpClient client = new HttpClient();
client.getUrl(Uri.parse("http://www.example.com/"))
   .then((HttpClientRequest request) {
     // Optionally set up headers...
     // Optionally write to the request object...
     // Then call close.
     ...
     return request.close();
   })
   .then((HttpClientResponse response) {
     // Process the response.
     ...
   });

The call to getUrl completes when the connection has been established. This can take time, so it should be possible to cancel that part. Currently there is no object to hook up any abort call to. A Future is returned, but futures cannot be canceled/aborted.

Then the future completes with a HttpClientRequest. That is closed to send the request and wait for the response. This can take time. In this step there is an object (the HttpClientRequest) on which you can actually call something (e.g. abort). However is seems wrong to call abort after calling close.

Finally the HttpClientResponse comes along and the response can be processed. Here there is also an object where an abort method can be added.

Having to add abort to both HttpClientRequest and HttpClientResponse seems wrong.

One option could be to add an additional argument to getUrl (and friends) where the called can pass an object where getUrl can place a callback for aborting the HTTP transaction, e.g.:

var c = new HttpClientConnectionController();
new Timer(new Duration(seconds: 2), () {
  c.cancel();
});
client.getUrl(Uri.parse("http://www.example.com/"), controller: c)
   .then((HttpClientRequest request) {
   ...
   .catchError((e) => ..., test: (e) => e is CancledException);

Straight-line code here ensures that the cancel member of HttpClientConnectionController has been set by getUrl before the timer callback can be called.

This is also related to issue #19120.


cc @lrhn.

@kasperl
Copy link

kasperl commented Mar 20, 2015

Added this to the 1.10 milestone.

@DartBot
Copy link

DartBot commented Apr 7, 2015

This comment was originally written by @kaendfinger


In all honesty, it would be nice to have some sort of Cancelable Future. In fact, in theory, it would be easy to add cancellation to futures, since it's built upon the Timer, you could either do an if statement to check if it was canceled, but it's unclear how this would propagate to the other futures.

@lrhn
Copy link
Member

lrhn commented Apr 7, 2015

There are definitely cases where it would be nice to have some sort of channel to tell an asynchronous computation that its result is no longer needed.

I don't think adding a function to Future is the best way to do that. Futures can be safely shared with other code. If the future could also be used to cancel the computation for everybody, then sharing it would no longer be safe.
I'd rather have something like a CancelableComputation class with a future and a cancel method (and a shorter name) - since we can't just return a tuple of a future and cancel function.

@sgjesse
Copy link
Contributor

sgjesse commented Apr 20, 2015

Removed this from the 1.10 milestone.
Added this to the 1.11 milestone.

@kevmoo kevmoo added Type-Enhancement library-io P1 A high priority bug; for example, a single project is unusable or has many test failures labels Apr 20, 2015
@kevmoo kevmoo added this to the 1.11 milestone Apr 20, 2015
@kevmoo kevmoo added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Jun 16, 2015
@sgjesse sgjesse modified the milestones: 1.12, 1.11 Jun 17, 2015
@sgjesse sgjesse modified the milestones: 1.13, 1.12 Aug 13, 2015
@mit-mit
Copy link
Member

mit-mit commented Sep 16, 2015

@sgjesse is anyone actually working on this? If not, should we close it as not planned?

@sgjesse sgjesse modified the milestones: Later, 1.13 Sep 16, 2015
@sgjesse
Copy link
Contributor

sgjesse commented Sep 16, 2015

At the moment we don't have a good solution to this. Using Future.timeout on in the various steps should work. That will not cancel the underlying operation, but that will fail in the background.

@davenotik
Copy link

Wow, still no timeout support. We'll continue using our custom httpRead() method then. Here it is:

https://gist.github.com/davenotik/169cc3a00d428516b257

It uses a Completer to build and return a Future. We start a Timer that calls completeError() to throw an error if the Future still hasn't completed after a specified duration.

@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed triaged labels Mar 1, 2016
@frank-rollpin
Copy link

Just wondering if there are any plans to address it, or should we forget about it and use workarounds?

@mit-mit
Copy link
Member

mit-mit commented Jun 18, 2019

cc @sortie

@mnordine
Copy link
Contributor

Any update on this?

@mit-mit
Copy link
Member

mit-mit commented Nov 18, 2019

cc @zichangg

@PavelPZ
Copy link

PavelPZ commented Jan 7, 2020

I can see three infos regarding timeouts in this thread, how they are related?

  • @butlermatt : Now that Timeout has been added to Socket.connect... here
  • @zanderso : new static field HttpClient.connectionTimeout may help with some component here... What does it mean?
  • @jonasfj : call HttpClientResponse.detachSocket which gives you a socket you can destroy... here

package:http with timeout is blocking feature for me. As I understand, problem is with dart:io (dart:html has solution for it).

Some example with the best solution so far would be very helpful. Is dart-lang/http#21 (comment) the correct one (using @zanderso's httpClient.connectionTimeout only, no @jonasfj's HttpClientResponse.detachSocket and @butlermatt's Socket.connect Timeout)?

@a-siva
Copy link
Contributor

a-siva commented Jan 7, 2020

zichang@ should be able to look at this once he is back from vacation.

@j0nscalet
Copy link

Seems like CancellableOperation could be helpful here? A potential solution route?

@PavelPZ
Copy link

PavelPZ commented Feb 13, 2020

@zichangg, I am still waiting for explanation if #22265 (comment)

@kevmoo
Copy link
Member Author

kevmoo commented Feb 13, 2020

@j0nscalet – sadly not. In many cases you want the network connection to terminate. The CancellableOperation won't do that – although it will give you back control flow

@j0nscalet
Copy link

j0nscalet commented Feb 14, 2020

@kevmoo Ah right, makes sense. Thanks.

In case this is helpful to anyone...
As a workaround I'm using CancelableOperation to get control flow around fetching the response stream, and while I don't like it I'm ok with the underlying network operation failing in the background, later, for now.

@zichangg
Copy link
Contributor

connectionTimeout is probably the answer. Here is an example.

  HttpClient client = new HttpClient();
  // Set up a time limit.
  client.connectionTimeout = Duration(microseconds: 1);

  client.getUrl(Uri.parse("https://www.google.com/"))
   .then((HttpClientRequest request) {
     // Optionally set up headers...
     // Optionally write to the request object...
     // Then call close.
     return request.close();
   })
   .then((HttpClientResponse response) {
     // response may not come back, so detachSocket() is not reliable.
   });

For other methods like Socket.connect and detachSocket are not good solutions for this problem. Socket.destroy will close the connection but it will not be exposed to user unless detachSocket is called. Since everything here is connected with Future.then mechanism. The only place to call detachSocket to abort the connection is when response is received. (It doesn't make any sense to cancel after request.close().) In this case, HttpClientResponse may not come back and detachSocket will not be called.

Using Future.timeout or CancelableOperation is probably not closing the underlying connection.

For providing a timeout handler, perhaps a try catch block can be used to catch timeout exception.

try {
  // Http connections.
} catch (e) {
  // Check exception type and provide handler.
}

@januwA
Copy link

januwA commented Apr 3, 2020

There is no good solution yet

@lirantzairi
Copy link

I'm also looking for a way to do this. I have a project where I need to upload images to the server, which of course can take time depending on the size of the image. I want provide the user a way to cancel the operation in case it's taking too long without the upload request continuing in the background. Can anybody help with a workaround? Thanks.

@zahidaz
Copy link

zahidaz commented Apr 19, 2020

it's almost five years and still looking for a standard solution, :(

dart-bot pushed a commit that referenced this issue Jul 22, 2020
The breaking change request for this cl: #41904

Bug: #22265
Change-Id: I36db64b4db307b78cd188a2f1701ec733f2e73db
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/147339
Commit-Queue: Zichang Guo <zichangguo@google.com>
Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>
@kevmoo
Copy link
Member Author

kevmoo commented Jul 22, 2020

Five years later! Never say never!

4b96f20

dart-bot pushed a commit that referenced this issue Jul 23, 2020
This reverts commit 4b96f20.

Reason for revert: Windows bots are broken. Because --socket-short-read is specified, the server doesn't receive full header at once.

https://dart-ci.appspot.com/log/vm-kernel-win-debug-x64/dartk-win-debug-x64/8907/standalone_2/io/http_client_connect_test/3

Original change's description:
> [dart:io] Add Abort() on HttpClientRequest
> 
> The breaking change request for this cl: #41904
> 
> Bug: #22265
> Change-Id: I36db64b4db307b78cd188a2f1701ec733f2e73db
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/147339
> Commit-Queue: Zichang Guo <zichangguo@google.com>
> Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>

TBR=lrn@google.com,zichangguo@google.com

Change-Id: I48f7a2ee3bb75e0e0ba0bd24ed53fcac372e016d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: #22265
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155548
Reviewed-by: Zichang Guo <zichangguo@google.com>
Commit-Queue: Zichang Guo <zichangguo@google.com>
@vsmenon
Copy link
Member

vsmenon commented Aug 5, 2020

Is the plan to reland the above CL?

@zichangg
Copy link
Contributor

zichangg commented Aug 7, 2020

Reland was ready! Because Lasse went on vacation. He will be back next week and I'll get it land next week.

@vsmenon vsmenon added this to the September Release 2020 milestone Aug 10, 2020
@vsmenon
Copy link
Member

vsmenon commented Aug 10, 2020

Thanks @zichangg - marking this as part of Sept release for now.

dart-bot pushed a commit that referenced this issue Aug 14, 2020
The test was poorly written. The response from Socket can arrive
separately. So the check for content-length header will fail.

This is a reland of 4b96f20

Original change's description:
> [dart:io] Add Abort() on HttpClientRequest
>
> The breaking change request for this cl: #41904
>
> Bug: #22265
> Change-Id: I36db64b4db307b78cd188a2f1701ec733f2e73db
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/147339
> Commit-Queue: Zichang Guo <zichangguo@google.com>
> Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>

Bug: #22265
Change-Id: Ibfe9565a3f9d5ef84274fba33a68fb57dbbe28c9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155581
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Zichang Guo <zichangguo@google.com>
@mit-mit
Copy link
Member

mit-mit commented Aug 14, 2020

@zichangg looks like this was relanded?

@zichangg
Copy link
Contributor

Yes. It has fixed the broken test and is ready to go.

@mit-mit
Copy link
Member

mit-mit commented Sep 24, 2020

Related issue for exposing this in package:http: dart-lang/http#424

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io P1 A high priority bug; for example, a single project is unusable or has many test failures type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests