Getting errored/closed reader, Auto-releasing reader, Forcible cancel() on a stream
yutakahirano opened this issue · 22 comments
I read @domenic's pull request and would like to talk about getting / releasing reader. If you think the PR is the right place to discuss, feel free to close this issue.
getReader() should succeed on closed stream
Imagine we have a Response (in Fetch API) res.
res.body.getReader() should succeed even when the response has an empty body. That means res.body.getReader() should succeed when the body is closed. The result is a closed reader.
getReader() should succeed on errored stream
Mainly for being consistent with the closed case. We do not lose anything by allowing this. The result is an errored reader.
Do we need auto-release?
Currently a reader is automatically released when it is closed or errored. Do we need the functionality?
It was introduced at #251 (comment), but given that our API is simpler than before and the current API requires a user to be sane, I would like to revisit the problem.
Concern: What happens when canceled via stream? What happens when canceled via reader?
Do we need .closed in stream?
I don't think of a use case. If we drop it, Resource management of Response.body will be much simpler.
The usefulness may depend on whether if keep auto-release or not.
Thanks for bringing these up. I was actually just preparing a follow-up commit to the PR to propose a solution for these issues :). It is nice to have a new issue to discuss though.
getReader() should succeed on closed stream
getReader() should succeed on errored stream
Great justifications, agreed.
Do we need auto-release?
Yes, I agree it is not as necessary anymore. We can probably remove it. The change in behavior between auto-release and manual release only impacts a few minor cases.
But on the other hand, I am not sure what the downsides of auto-release are, if any. So maybe we should more carefully consider what the changes in behavior are and which result we would prefer.
Do we need .closed in stream?
I am less sure about this one. I am having a hard time with use cases though. I will have to think on it harder.
Another question:
Should we allow cancellation of a locked stream?
The PR #296 does allow this. It is kind of a violation of the authority principles getReader() is based around. However, without it, you can get in trouble by setting up a pipe chain without having any way to shut it down.
I think the proper solution to this is to change pipeTo to return a { finished, unpipe() } object instead of a promise directly. I think this is OK. Most consumer code just changes from rs.pipeTo(ws).then(...) into rs.pipeTo(ws).finished.then(...). (We used to have rs.pipeTo(ws) return ws and say that people should do rs.pipeTo(ws).closed.) And now we allow people to unpipe(). unpipe() is nicer even than rs.cancel(), since it gives more control over exactly what happens to rs and ws. By default it will leave both open, but then you have the option of cancelling rs and either close()ing or abort()ing ws.
If we have this in place then we can remove cancellation of locked streams and keep the authority with the reader. We will still have rs.cancel() as sugar for (a safer version of) const reader = rs.getReader(); const result = reader.cancel(); reader.releaseLock(); return result;.
I added bb47488 to the pull request which address much of these. I will do another follow-up commit changing the pipeTo return value.
Regarding auto-release: there are very very few differences in behavior that you can discern without inspecting isActive.
The only difference I can find is that if you write "bad" reader-using code that doesn't release the reader when it finishes with it, nobody will be able to get any more readers, even though those readers will always just be closed/error. (Actually, the readableStreamToArray function in the pull request is an example of such bad code: it will release upon successful close, but not if the stream errors.)
So, I think it would be better to keep auto-release.
Mainly from Fetch API point of view...
Regarding auto-release:
We had a long discussion about Body.bodyUsed which is summalized at https://github.com/tyoshino/streams_integration/blob/master/FetchBodyPrecondition.md. In the discussion, people tended to like a flag which would never be released after locked (e.g. new Request, cache.put()). If it is generally true, leaving an option for a user to "not release" might be good.
Regarding forcible cancel:
Without it, we don't have a means to cancel fetch operation after calling text().
var p = res.body.text();
// res.body.cancel doesn't work here, so we can do nothing if the stream is infinite.
Not having forcible cancel means that any consumer should have a cancel method, and it should be an object, not a function (i.e. text() is not a good API). (Of course, we could have cancel() on Request or Response, but it looks a bad sign to me.)
Yutaka: Good point. To address an issue the cancellation issue with pipeTo(), basically we need to do the same for the methods (text(), json(), ...) on the Body.
It's sad but we cannot adopt { finished, cancel() } approach for these stuff. They're already shipped.
Hmm, will it be better if we provide not forcible cancel() but forcible releaseLock() on the stream?
I think it is not so bad to not support cancellation of json() etc. At least, to not support them until we have a better scheme for cancellable promises in general. When we have such a scheme (which might be a .cancel() method on the promise, or might be something like const canceller = new Canceller(); fetch({ ..., canceler }); /* later */ canceller.cancel();) we can retrofit json() etc. to allow it. Until then we just say that they cannot be interrupted---which is true today.
On the other hand the forcible releaseLock() is an interesting idea... Kind of counterintuitive, but I will think about it more.
I see. Should we also address pipeTo() cancellation issue by the cancellable promises?
const p = rs.pipeTo(dest);
p.cancel();
or
const p = rs.pipeTo(dest, { ..., canceller });
canceller.cancel();
Wouldn't the latter be implemented adopting the revealing constructor pattern?
Oh wow, good point, I hadn't thought of that. That would make more sense. Maybe we should leave pipes uncancellable until cancellable promises land. That would give me more motivation to work on cancellable promises, haha -_-.
The latter is pretty similar to revealing constructor, yeah. Although it would tie into a more general cancelable promise framework.
The canceller idea looks good to me.
We just had more discussion about cancelling fetch(). In #297 (comment), Yutaka meant that we should have some method to stop receiving HTTP response body. It could be any of res.cancel(), res.body.cancel() and reader.cancel(). He thought having three points is bad and wondered what we should have.
Sorry, I forgot but text(), json(), etc. are on res, not res.body. I'm fine with having a .cancel() on res to address this issue. It makes the stream errored by terminating the ongoing fetch algorithm.
I'm also fine with your (Domenic's) canceller approach.
Regarding the Fetch API, we also need a method to terminate the fetch algorithm while the promise is not yet fulfilled (response headers are not yet available). So, my gut feeling is that we shouldn't have two termination methods but only one that is available from the time we start the fetch algorithm. Yutaka and I also discussed this a little. Request is an object that represents HTTP request but not what represents ongoing HTTP fetch. It can be used for multiple different fetch() calls. So, such a cancellation method should live on the promise or something else. The promise is abstraction of a result. Here result is Response which becomes available once the HTTP headers are received. So, it seems a bit strange if we also give it a power to cancel receiving HTTP body.
So, I like the canceller revealing approach. My preference on single method over X to cancel receiving header, and Y on Response to cancel receiving body is slight. Maybe I could live with two canceller approach is we can define it cleanly.
Right, there is a lot of discussion about how to do fetch cancellation, I think in the service worker repo. I think it is OK to (eventually) have multiple possibilities as long as they all related together. E.g. maybe fetchPromise.cancel() is defined as doing either (1) internal method to cancel request before headers are received; or (2) canceling the stream, and maybe (2) is exposed directly (since it is more general).
Hmm. But if we allow fetchPromise.cancel() (or the canceller approach) then we have to decide what it does when the stream is locked. So that comes back to our original problem.
Related issue at the Fetch API issue trakcer: whatwg/fetch#20
OK, quick in-person huddle yielded the following proposal.
cancel()method is on reader, and not on streampipeTo, and any operations a developer might create (likereadableStreamToArray), should get cancellation ability in whatever way promises get cancellation ability. Since they work internally by usinggetReader(), they can callreader.cancel()in reaction to cancellation requests.- fetch cancellation ability has the ability to stop the flow of data at any time, even if the headers are already delivered and the body is locked. Since this is more disruptive, and can interfere with people who thought they had exclusive control, it should not work via the same cancellation path. Instead it makes the stream errored. This is identical to how, when the browser stops a stream (e.g. because the user pressed the "Stop" button or unplugged their internet connection or something), it will error the stream with an abort error of some kind.
This makes a lot of sense to me. Action items/takeaways:
- Revert
{ finished, unpipe() }in that PR, pending future more general cancellable promise solution - Keep
cancel()on reader only - Cancelling a fetch, in whatever form it takes, should error the stream (if it exists).
- A canceller approach is probably better for fetch() than a
fetchPromise.cancel()approach. Since otherwisefetchPromise.cancel()would affect things after the promise has already settled. (Or, alternately, we would allowfetchPromise.cancel()to affect the fetch only before headers are retrieved---which is awkward.)
OK, let me summarize the cancellable promise approach.
const fetchPromise = fetch(request);
- [Plan A]
fetchPromise.cancel()works only while theResponseis not yet available- returns true if the
fetchPromiseis not yet fulfilled - returns false if the
fetchPromiseis already fulfilled- to terminate the fetch algorithm, we need to call some method on the
Response(fulfillment value offetchPromise) that terminates HTTP body receiving
- to terminate the fetch algorithm, we need to call some method on the
- returns true if the
- [Plan B]
fetchPromise.cancel()works for terminating the fetch algorithm at any stage- return type of fetchPromise.cancel() is void.
- fetchPromise may be already fulfilled and there's already a microtask to run the fulfillment callback.
- even in such situation,
fetchPromise.cancel()can be used for stopping HTTP body receiving
- even in such situation,
Is your idea described well in [Plan B]?
Oh. Conflict :). So, plan A and B are elaboration of the last bullet point of #297 (comment). Tell me if there's anything to correct.
I think I was originally thinking of [Plan B] but while writing the end of my last comment I realized it was kind of dumb. So if promise cancellation is accomplished via p.cancel() then I think [Plan A] is better.
But if we do promise cancellation via a cancellation token ("canceller") approach then I think we could allow [Plan D] canceller.cancel() to cancel either the promise, headers are not yet received, or error the stream, after headers are received. I think this is probably able to be modeled, although we haven't worked out the details of how canceller works. But, maybe that is needless complication and we would do [Plan C] canceller.cancel() only works before the headers are received, and you need to do res.cancel() or similar (res.error()? res.abort()?).
(Plan A <-> Plan C, Plan B <-> Plan D, is the mapping.)
[Plan D] is useful. [Plan C] also looks clean given current Fetch procedure (receive Response, then receive body via Streams) and one slight benefit I see is that we can forget Request. I don't object to [Plan C].
need to do req.cancel() or similar (req.error()? req.abort()?).
Do you mean res?
Yes, sorry, I did mean res! Editing post so people don't get confused.
I think we can delay deciding between [Plan A], [Plan C], and [Plan D] until we figure out cancellable promises in a bit more detail. But the important thing is that we do have a path forward that gives us all the abilities we want, and its impact on streams is essentially that we won't be able to cancel a pipe until we also figure out how to cancel a promise.
OK! List of plans about API for cancelling Fetch:
- [A] #297 (comment)
- [C] #297 (comment)
- [D] #297 (comment)
Decision about forcible cancel regarding pipeTo() or any other operations requiring lock:
See #297 (comment)
Remaining issues in this thread are:
- Auto-release? If yes (which I think I prefer), can we still define fetch's bodyUsed correctly?
- Remove closed promise from stream (and move it only to the reader)? May help with resource management, and it is unclear what the use case is for
closedwithout first getting a reader. But feels wrong to me...
OK. Managed to convince @yutakahirano offline that auto-release is good. Am willing to remove closed, at least for now; we could potentially add it back later if someone comes up with a really good use case.
#296 updated and build is passing. Ready for more review! Closing this issue.