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

[Breaking Change Request] Support abort() in HttpClientRequest in Dart:IO #41904

Closed
zichangg opened this issue May 14, 2020 · 14 comments
Closed
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. breaking-change-request This tracks requests for feedback on breaking changes library-io

Comments

@zichangg
Copy link
Contributor

zichangg commented May 14, 2020

Summary

With current APIs in Dart:IO, there is no way to abort the HttpClientRequest. Once HttpClientRequest has been sent, the request cannot be canceled until HttpClientResponse comes back. See the example below:

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

Abort() will allow users to stop waiting for HttpClientResponse, so users can abort the request if it takes too long.

Function signature

  abort([Object? exception, StackTrace? stackTrace]);

The intended behavior

  • The future returned by close() will complete with exception and stackTrace if abort() is called prior to HttpClientResponse being available. The default value for exception and stackTrace will be HttpException and StackTrace.empty.
  • Data cannot be sent through IOSink's API after abort().
  • In case of HttpClientResponse returning before abort, abort() will not make any effect.

Expected use case

HttpClient client = new HttpClient();
client.getUrl(Uri.parse("http://www.example.com/"))
   .then((HttpClientRequest request) {
     // Optionally set up headers...
     request.write();
     // Optionally write to the request object...
     // Then call close.
     Timer(Duration(seconds: 1), () {
       request.abort(Exception("Customized value"));
     }); 
     return request.close();
   })
   .then((HttpClientResponse response) {
     // Process the response.
     ...
   }, onError: (e) {
      // e will be `Exception("Customized value")`
   });

Expected impact

All classes extends or implements HttpClientRequest in Dart:IO will need to update accordingly.

Proposed Implementation

https://dart-review.googlesource.com/c/sdk/+/147339

Relative discussions

#22265

@zichangg zichangg added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io breaking-change-request This tracks requests for feedback on breaking changes labels May 14, 2020
@zichangg
Copy link
Contributor Author

@lrhn @mit-mit @sortie

@franklinyow

This comment has been minimized.

@zichangg

This comment has been minimized.

@franklinyow

This comment has been minimized.

@zichangg

This comment has been minimized.

@mit-mit
Copy link
Member

mit-mit commented May 19, 2020

lgtm, @franklinyow can you enquire with partner teams?

@franklinyow
Copy link
Contributor

cc @Hixie @matanlurey @vsmenon for review and approval.

@Hixie
Copy link
Contributor

Hixie commented May 20, 2020

An abort method on the request object seems like an elegant solution.

@zichangg
Copy link
Contributor Author

Based on some discussion in draft cl, abort() now will accept two optional parameters. Users are allowed to provide customized exception and stackTrace, which will trigger onError() callback.
@Hixie @matanlurey @vsmenon

@Hixie
Copy link
Contributor

Hixie commented May 28, 2020

SGTM.

@zichangg
Copy link
Contributor Author

zichangg commented Jun 8, 2020

A friendly ping. @matanlurey @vsmenon
If it looks good, I can starting preparing fixing patch on packages and flutter. ^_^

@vsmenon
Copy link
Member

vsmenon commented Jul 6, 2020

lgtm

1 similar comment
@matanlurey
Copy link
Contributor

lgtm

@franklinyow franklinyow removed their assignment Jul 7, 2020
@franklinyow
Copy link
Contributor

Mark Approved
@zichangg Please go ahead

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 <[email protected]>
Reviewed-by: Lasse R.H. Nielsen <[email protected]>
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 <[email protected]>
> Reviewed-by: Lasse R.H. Nielsen <[email protected]>

[email protected],[email protected]

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 <[email protected]>
Commit-Queue: Zichang Guo <[email protected]>
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 <[email protected]>
> Reviewed-by: Lasse R.H. Nielsen <[email protected]>

Bug: #22265
Change-Id: Ibfe9565a3f9d5ef84274fba33a68fb57dbbe28c9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/155581
Reviewed-by: Siva Annamalai <[email protected]>
Commit-Queue: Zichang Guo <[email protected]>
@zichangg zichangg closed this as completed Sep 1, 2020
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. breaking-change-request This tracks requests for feedback on breaking changes library-io
Projects
None yet
Development

No branches or pull requests

6 participants