http
http copied to clipboard
dart-lang
Add support for request timeout
Wire up a timer to perform the appropriate type of aborting and resource cleanup for each client. Handles canceling the timer when the request completes.
cc @lrhn @jakemac53
I'd like to get some initial feedback on the approach and whether this is worth doing.
I think this is worth doing because timeouts are a common case for wanting to abort requests, and they are easier to get wrong than to get right. The naive approach of .timeout doesn't clean up resources and the entire result is still going to get read. Even with a general mechanism to abort requests a timeout argument is handy because we can handle the timer cancellation which would otherwise be boilerplate. We also likely can't arrive at a solid design for request aborting in general, but the API design with timeout: argument is fairly straightforward.
Please let me know what you think of this initial version, if we think it looks like a good approach I can start work on tests. I'm not sure how easy it is going to be to cover all the behavior here well.
One more question about the API design.
Some backround: There are differences between behaviors with the send API. In the browser the Future won't complete until the entire response body is read, while on the vm the Future will complete as soon as headers are received, and the response body stream is kept open until the entire body is read. I do think it's plausible we could change this in the browser - perhaps not going all the way to a real streaming implementation, but potentially at least making the Future complete with the headers. I don't know if we need to block on that.
The immediate question is, should the Stream emit the Timeout or not. If it doesn't the timeout would apply only to receiving the headers and you'd need to cancel a stream subscription to get a full timeout.
My current implementation emits the timeout, which is convenient because it makes the Future only APIs work as expected without any extra effort.
It sounds like we have two APIs, one future based and one stream based. They should probably act differently.
It's hard to say what a "timeout" means if the author cannot specify it. Do they need the entire result before that time (when do I need to move on, with or without a response?), or do they just need some evidence that the remote end exists and is willing to answer before that time (how long do I wait if the remote server is down?). I can see both use-cases as valid.
Is there any way to cancel the timeout? So, set a timeout of 5 seconds, which will then kill the connection after that time (and put an exception on the stream), but if I get a header and plausible start of the response before that time, then I can cancel the timeout (or start a new timer). That would make it easier for the client to control the timeout behavior without having to know up-front whether they're waiting for a non-remote-end timeout or a large response.
I can see the use cases for both variants of timeout. One thing I'm not sure of is whether most authors would want to timeout the connection (which they can do with a dart:io client) or the headers.
I can see both use-cases as valid.
I agree that both are probably valid, it's hard to say whether both are worth the API space. After some discussion I think we are leaning towards the timeout applying to the Stream as well. It's slightly less general, but also a friendlier API for the case it solves, and my hunch is that is the more common one.
What we can do, though, is hold on to a little room for the other one. If we name the argument contentTimeout instead of timeout then it's easier to add a headerTimeout argument later. It would still be a breaking change and a major version bump, but the vast majority of uses would not be impacted.
Currently I'm leaning towards using the name timeout for the Future APIs, and contentTimeout for the Future<Stream> API. We could add a headerTimeout to the Future APIs but it starts to get weird to have a Future which might be timed out within some period, or might wait indefinitely (or to some longer separate timeout) without any way to observe what is happening. I don't know if anyone is asking for a timeout specifically for headers either - I think most users expect a Future to have a single timeout, and the name timeout is nicest for that. I'd be happy to discuss using the name contentTimeout for all of these arguments consistently if anyone wants to bikeshed a bit more or thinks it is important to leave room to add a headerTimeout for these other APIs as well.
Is there any way to cancel the timeout?
Not that I can think of without a difficult to understand API.
After some further discussion we think there is more risk in getting this API wrong than there is in delaying it until after the dust settles on NNBD and we can take more time to design it alongside general request aborting and decide about the rollout plan from there. We may end up with a slower alternate-import type migration for request aborting, and so we could use that same approach to deliver a timeout feature alongside abort with a gradual migration.
We should continue to use this discussion to inform our future design, but I don't think I'll hold the null safe publish on it.
One thing I'm noticing about this as I look at overrides of send in other client implementations - it isn't very friendly to deal with the Timer. From an overrider standpoint the Future<void> abortOn seems easier. It's unfortunate that the design for customizing a client deals with overriding a public facing API - if those were separate signatures we could give a friendly-to-use API for users and a friendly-to-implement API for implementers.
This PR contains important features, has gone under extensive reviews, and should be merged by now.
There are only a few improvements left on the documentation, and some merge conflicts.
@natebosch Can you please address this? So an approval and merge can be made?
