-
Notifications
You must be signed in to change notification settings - Fork 359
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
Support Request Cancellation, Timeouts via RequestController #978
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting API.
What are your use cases for being able to independently control parts of the timeout?
I don't think we'll be able to move forward with this PR in the short term. It will take a deeper review to see if this API is in the direction we'd want to go with cancellation.
pkgs/http/lib/src/client.dart
Outdated
/// If this is `true`, [send] will use the supplied [RequestController] and | ||
/// will allow cancelling the request and specifying a timeout. Otherwise, | ||
/// a specified [RequestController] will be ignored. | ||
bool get supportsController; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change for implements Client
uses. For example https://github.com/google/googleapis.dart/blob/ef216bc000480b20d8a08776343bd2f8eacd423d/googleapis_auth/lib/src/auth_client.dart#L10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops - as was already the case with BaseClient
that was supposed to return false
by default to allow clients to opt-in.
Edit: should be resolved with latest commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the API at all is breaking. Any class which implement Client
without extends BaseClient
will be missing the override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, of course. I'll do some testing against something like the linked client (AuthClient
) for that then - I didn't initially find any clients that implemented Client
so I presumed they would use BaseClient
like ReplyClient
does.
That might need further review then, but off the top of my head a backwards-compatible solution could be to introduce some stub that could be mixed in or included and tested for.
For example: https://dartpad.dev/bc9a693d707f95a0356291817f38db1e
(so that it can be explicitly set by conforming clients)
I think generally the ability to control timeouts over individual parts of the lifecycle would be beneficial for specific use cases - particularly those dealing with large payloads - off the top of my head, some examples could be:
These could be used similarly to the Also, I would imagine it is most useful for things like Also, the lifecycle is tracked implicitly for the purposes of properly supporting cancellation in any client implementing the |
// connectCompleter has not yet been marked as completed, complete it. | ||
if (xhr.readyState == HttpRequest.OPENED) { | ||
if (connectCompleter != null) { | ||
connectCompleter.complete(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does HttpRequest.OPENED really imply that a connection has been established? I thought that it just meant that .open()
has been called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't (as suggested here on StackOverflow) so that probably shouldn't be implemented in the BrowserClient. (I wasn't sure if preflight requests might happen at the open
stage, but since they depend on the headers and origin - having thought on it - I guess not and checking the Chromium seems to confirm that.)
That said, whilst looking into the browser side of things, I saw that the new fetch API (caniuse: fetch API) supports streams (caniuse: ReadableStream) and could probably be used to implement this functionality as it is on the Dart side.
What kind of support is being aimed for on the browser side? Is there a reason XMLHTTPRequest is still being used or might it be worth looking at moving to the fetch API? - See #595
I think that Would it make sense to ask people who need these more advanced features to use |
I agree that it is complex and maybe there is a point to be made that its complexity falls outside the scope of the package. I had some reservations about Also, as mentioned before, tracking the lifecycle in a similar manner would still be essential (at least on I think the major benefit of this package is that it encapsulates all of the platform-specific implementations and exposes a single consistent API so as long as it is done consistently I would advocate including the feature in this package because it would save anyone needing a cross-platform HTTP solution that also requires control over |
Just seen that this has been re-opened! 🎉 I've been wondering whether an approach without a dedicated controller could work and the timeouts part could work to get the basic functionality implemented faster. Regardless, hoping this can be merged as relying on Dio for this is a bit annoying! (Separately, I was wondering whether aborting HTTP is possible in WASM?) |
Closes #204, closes #424, closes #567
Supersedes and closes #521, supersedes and closes #602, supersedes and closes #819
This PR implements an opt-in
RequestController
API to manage requests. The API is opt-in, future-proof and backwards-compatible both for implementations ofClient
and for end users. If a controller is not specified by the user, the existing functionality of the package is maintained. Likewise, clients can opt-in to the new API and show they are doing so by overridingsupportsController
to be true and implementing the functionality. Later functionality of a similar nature can also be added in an opt-in manner by simply adding it to theRequestController
class.See the full examples below, but a brief tour of the RequestController's functionality is as follows:
It can then be used as follows:
I note, from above, that there was existing work in both request cancellation and timeouts however I felt that this different approach could cleanly solve both issues with a single, more ergonomic, user-friendly and intuitive API. This work started originally as a private fork of the package to implement some functionality required for another package, however I felt that the results might be viable for inclusion in the upstream package.
Additionally, with respect to #819, I happened to implement this similarly to make it possible to abort a response of an
IOClient
(useful for applications that need to interrupt a response with a large payload). For this, I created aClosableStreamedResponse
that extendsStreamedResponse
with an additionalclose
future.This should only be necessary for the
IOStreamedResponse
(for which it is implemented with detachSocket and destroy which has been tested and works). The browser client appears to obtain the entire buffered response from the browser (via theonLoad
event) and return that, so there's no sense or utility in closing that response - it can be aborted withxhr.abort
.For
ClosableStreamedResponse
s to be cancellable with the controller,Response.fromStream
grabs theActiveRequestTracker
for the response's request (if there is one) and, if that controller hascancel
called on it, closes the response stream if and only if the response's stream is a ClosableStreamedResponse. (See lines 70-78 ofresponse.dart
).Examples
Partial Timeouts (for each step of the request-response cycle)
In this case, the send timeout triggers a
TimeoutException
because no response is received from the server.Server-side logs:
Partial Timeouts (another example)
In this case, none of the timeouts trigger an exception because the response body becomes available immediately, even if it is not complete and the server hangs for only 5 seconds before completing the response, meaning the receive timeout of 6 seconds is not triggered either.
Server-side logs:
Cancelation Example (cancel during response)
Cancels after 500ms which is whilst the response is being written. The connection is destroyed so both the client and server clean up immediately after the request is cancelled.
Server-side logs:
Cancellation Example (cancelled immediately)
The request is cancelled immediately and isn't even logged on the server.
Server-side logs:
Server implementation and Dart client example used to test:
https://github.com/SamJakob/cancellable_http_dart_testing
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.