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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject pending reads when releasing reader #1168

Merged
merged 33 commits into from
Jan 13, 2022

Conversation

MattiasBuelens
Copy link
Collaborator

@MattiasBuelens MattiasBuelens commented Oct 2, 2021

Currently, ReadableStream(Default|BYOB)Reader.releaseLock() throws an error if there are still pending read requests (spec):

The releaseLock() method steps are:

  1. If this.[[stream]] is undefined, return.
  2. If this.[[readRequests]] is not empty, throw a TypeError exception.
  3. Perform ! ReadableStreamReaderGenericRelease(this).

However, in #1103 we realized that it would be more useful if we allowed releasing a reader while there are still pending reads, and to reject those reads instead. The user can then acquire a new reader to receive those chunks.

This PR implements that proposed change:

  • reader.releaseLock() no longer throws an error when there are still pending reads. Instead, it immediately rejects all pending reads with a TypeError.
  • For readable byte streams, we also discard pull-into descriptors corresponding to (now rejected) read-into requests. However, we must retain the first pull-into descriptor, since the underlying byte source may still be using it through controller.byobRequest. Instead, we mark this descriptor as "detached", such that when we invalidate this BYOB request (as a result of controller.enqueue() or byobRequest.respond()), the bytes from this descriptor are pushed into the stream's queue and used to fulfill read requests from a future reader.

This also allows expressing pipeTo()'s behavior more accurately. Currently, it "cheats" by calling ReadableStreamReaderGenericRelease directly without checking if [[readRequests]] is empty. (And yes, there's a test that relies on this happening.) With the proposed changes, we can safely release the reader when the pipe finishes (even if there's a pending read), and be sure that any unread chunks can be read by a future reader or pipe.

To do:

  • Question: should pending reads be rejected with a TypeError or a AbortError DOMException?
  • Update spec text
  • Write more tests

Originally, this PR proposed to only add some extra asserts to ensure that [[readRequests]] is empty before releasing the lock. However, as shown in the pipeTo case (above), this is not sufficient. For context, the original description of that proposal is attached below:

Original description

When using ReadableStream(Default|BYOB)Reader.releaseLock(), we throw an error if there are still pending read requests (spec):

The releaseLock() method steps are:

  1. If this.[[stream]] is undefined, return.
  2. If this.[[readRequests]] is not empty, throw a TypeError exception.
  3. Perform ! ReadableStreamReaderGenericRelease(this).

However, when we use the abstract op ReadableStreamReaderGenericRelease directly, we do not assert this. Some places in the spec often have an extra assert specifically for this (e.g. async iterator's return steps or ReadableByteStreamTee), but other places fail to perform this check (e.g. async iterator's next steps or ReadableStreamPipeTo).

I added this missing check, but now the following WPT tests start failing as a result:

The async iterator test fails because we release the lock in the middle of the read request's close steps, but ReadableStreamClose only clears the list of read requests after calling all the close steps. The fix is quite simple: empty the list before calling the close steps or error steps. I'll push a commit for that on this PR.

The pipe tests fail because the pipe loop actually starts a read request, but we don't wait for that read to complete before releasing the lock. I think it's possible you could lose a chunk this way, but I haven't yet figured out how. I'll keep trying though. 馃槢 I think we can solve this one by tracking both currentWrite and currentRead, but I'm not sure about the details yet.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@MattiasBuelens
Copy link
Collaborator Author

I've fixed ReadableStreamCancel, ReadableStreamClose and ReadableStreamError so they empty the list of read requests before calling the close/error steps. That does indeed fix the async iterator tests.

Piping was trickier, because there's interaction with "shutdown with an action". Specifically, when preventCancel = false, we don't want to wait for the read to complete before we call ReadableStreamCancel. I suggest we change it to:

  1. Wait until all writes have finished.
  2. Perform action, and wait for it to complete.
  3. Wait until all reads and writes have finished.
  4. Finalize, passing along any error from step 2.

This works perfectly, except for this one test:

promise_test(t => {

  const rs = recordingReadableStream();

  const ws = recordingWritableStream();

  const pipePromise = promise_rejects_exactly(t, error1, rs.pipeTo(ws, { preventCancel: true }),
                                              'pipeTo must reject with the same error');

  t.step_timeout(() => ws.controller.error(error1), 10);

  return pipePromise.then(() => {
    assert_array_equals(rs.eventsWithoutPulls, []);
    assert_array_equals(ws.events, []);
  });

}, 'Errors must be propagated backward: becomes errored after piping; preventCancel = true');

Because preventCancel = true, the pipe's read request never finishes, so it gets stuck. I believe this is the correct behavior though: we cannot cancel the stream, so we have to wait for the read to finish before we can safely release our reader.

I see two solutions:

  • Change the test to somehow unblock the read request, e.g. by enqueuing a chunk on the readable after erroring the writable.
  • Abort the read request without cancelling the stream, as suggested in Make read requests abortable聽#1103.

@MattiasBuelens MattiasBuelens changed the title Assert no pending reads on release Assert there are no pending reads when releasing lock Oct 3, 2021
@domenic
Copy link
Member

domenic commented Oct 12, 2021

Perhaps a dumb question, but maybe the tests indicate that we should allow releasing the lock even with pending read requests? I'm not sure that restriction is super well-motivated, and if we've found at least one higher-level combinator (namely pipeTo) where doing so is arduous, there might be more.

Otherwise, solving #1103 would make more sense...

@MattiasBuelens
Copy link
Collaborator Author

Perhaps a dumb question, but maybe the tests indicate that we should allow releasing the lock even with pending read requests?

That's also a possibility.

I wouldn't want to leave the read() promises pending though, so I'd say reader.releaseLock() must reject all pending read requests with an AbortError. Or perhaps a TypeError, to align with how we reject reader.closed? 馃

So basically releaseLock() becomes what controller.abort() would have done for the originally proposed getReader({ signal: controller.signal }).

@domenic
Copy link
Member

domenic commented Oct 14, 2021

Interesting. I think I like that idea. And we could add getReader({ signal }) as additional sugar for this behavior, similar to how addEventListener(, { signal }) gives you sugar for later calling removeEventListener().

Does this semantic of rejecting the read() promises work well even for the piping case?

@MattiasBuelens MattiasBuelens force-pushed the assert-no-pending-reads-on-release branch from 4d08211 to 4fb181c Compare October 14, 2021 22:36
@MattiasBuelens
Copy link
Collaborator Author

It works perfectly for pipeTo()! 馃榿 We already do setPromiseIsHandledToTrue(pipeLoop()), so any rejected read()s within the loop will stop the loop but not cause additional rejections.

Of course we'll need to update some other tests that expect releaseLock() to throw when there are pending reads (and instead expect it to reject those reads), but that's feasible.

However we still need to figure out what to do with the pending pull-into descriptors when calling byobReader.releaseLock(), see my other comment. 馃

index.bs Outdated Show resolved Hide resolved
@MattiasBuelens MattiasBuelens force-pushed the assert-no-pending-reads-on-release branch from 4fb181c to ce7e128 Compare December 15, 2021 00:09
@MattiasBuelens MattiasBuelens changed the title Assert there are no pending reads when releasing lock Reject pending reads when releasing lock Dec 15, 2021
@MattiasBuelens MattiasBuelens changed the title Reject pending reads when releasing lock Reject pending reads when releasing reader Dec 15, 2021
@MattiasBuelens MattiasBuelens force-pushed the assert-no-pending-reads-on-release branch from cc66569 to 8735c80 Compare December 15, 2021 21:29
@MattiasBuelens
Copy link
Collaborator Author

MattiasBuelens commented Dec 16, 2021

I started working on dealing with pending pull-into descriptors after calling reader.releaseLock(), following the ideas from #1103 (comment).

When calling releaseLock(), we go through a new controller-specific [[ReleaseSteps]] algorithm. For ReadableStreamDefaultController, this does nothing. For ReadableByteStreamController, this does two things:

  • The first pull-into descriptor is marked as "detached" from its read-into request, by setting its [[readerType]] to a new special "none" value. We cannot discard this descriptor, since the underlying byte source may have already interacted with it through controller.byobRequest.
  • Any other pull-into descriptors are immediately discarded. (It's impossible for the underlying byte source to have seen these descriptors in its controller.byobRequest.)

When the underlying byte source eventually calls controller.enqueue() or byobRequest.respond(), we finish cleaning up this detached pull-into descriptor. We have to be very careful though:

  • The descriptor may have already been partially filled. If so, these bytes need to first be copied into the stream's queue. From there, we can try to fulfill pending read() or read(view) requests.

For now, I've only updated the reference implementation. If we're okay with the approach, I can look into updating the spec text.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, with just a few questions!

@domenic
Copy link
Member

domenic commented Jan 4, 2022

Question: should pending reads be rejected with a TypeError or a AbortError DOMException?

I slightly lean TypeError? We use it for a lot of similar situations in this spec, and now that abort reasons are a thing, we don't use AbortError DOMException directly at all.

@MattiasBuelens MattiasBuelens force-pushed the assert-no-pending-reads-on-release branch from be00afc to 17cae42 Compare January 5, 2022 15:13
@MattiasBuelens
Copy link
Collaborator Author

MattiasBuelens commented Jan 5, 2022

I slightly lean TypeError? We use it for a lot of similar situations in this spec, and now that abort reasons are a thing, we don't use AbortError DOMException directly at all.

Fair enough.

In #1103 we had an alternative API proposal: readable.getReader({ signal }). If we went that route, it would have made sense to reject with the signal's abort reason instead. But with the current API proposal, there's indeed no real interaction with an AbortSignal.

If desired, users could always propagate their abort reason manually:

const abortController = new AbortController();
const { signal } = abortController;
const reader = readable.getReader();
signal.addEventListener("abort", () => {
  // Cannot propagate `signal.reason` to `releaseLock()`
  reader.releaseLock();
});
const read = reader.read().catch((e) => {
  // `e` will be a new `TypeError` instead of `signal.reason` here.
  // A user could propagate it manually though, if necessary:
  if (signal.aborted) {
    throw signal.reason;
  }
  throw e;
}).then(/* ... */);
abortController.abort(new Error("my custom abort reason"));

Although I don't think users will commonly want to use an AbortSignal to release a reader anyway. 馃し

The first pull-into descriptor is retained so the underlying byte source can continue filling it.
This descriptor becomes "detached" from the corresponding read request, such that when
the BYOB request receives a response, it *always* puts all of its bytes into the queue rather
than fulfilling a read request.

Other pull-into descriptors are discarded immediately, since the underlying byte source
cannot fill or have filled those yet.
Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I think this is a good direction. Although it adds some complexity, the new behaviour is easier to work with.

domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 13, 2022
@domenic domenic merged commit d5f92d9 into whatwg:main Jan 13, 2022
@MattiasBuelens MattiasBuelens deleted the assert-no-pending-reads-on-release branch January 13, 2022 21:13
@MattiasBuelens
Copy link
Collaborator Author

Other implementations have not yet implemented readable byte streams. As such the editors are treating this as a bugfix to readable byte streams, that falls under their previously-expressed general interest in the feature. (Although, explicit review from other engines would be welcome.)

This spec change also affects default readers. We no longer throw a TypeError when ReadableStreamDefaultReader.releaseLock() is called while there are pending read requests, instead we now reject all pending read() requests.

So technically speaking, we merged this PR a bit too early. 馃槄 We should have asked for their interest first. As discussed on Matrix, I'll mention this when filing spec bugs for all browsers, and ask them to let us know if they have any objections.

@dead-claudia
Copy link

So technically speaking, we merged this PR a bit too early. 馃槄 We should have asked for their interest first. As discussed on Matrix, I'll mention this when filing spec bugs for all browsers, and ask them to let us know if they have any objections.

Looked through the bugs and I think you may have forgotten Chrome?

@MattiasBuelens
Copy link
Collaborator Author

I filed #1287273 with Chrome. It's mentioned in the OP.

@dead-claudia
Copy link

Sorry, let me emphasize:

So technically speaking, we merged this PR a bit too early. 馃槄 We should have asked for their interest first. As discussed on Matrix, I'll mention this when filing spec bugs for all browsers, and ask them to let us know if they have any objections.

Was referring to that last part specifically.

@MattiasBuelens
Copy link
Collaborator Author

MattiasBuelens commented Jan 15, 2022

@nidhijaju and @ricea have already expressed interest on behalf of Chrome in #1168 (review) and #1168 (review), so I didn't deem it necessary to ask again in the Chrome issue. Unless I misunderstood something?

Of course, if they still have any objections or remarks, then they are also welcome to discuss them here. 馃檪

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 5, 2022
鈥ing reader, a=testonly

Automatic update from web-platform-tests
Streams: reject pending reads when releasing reader

Follows whatwg/streams#1168.
--

wpt-commits: 99d74f9529e16ec0722ef11136ab29b9e80fff26
wpt-pr: 32072
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 6, 2022
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 7, 2022
鈥ing reader, a=testonly

Automatic update from web-platform-tests
Streams: reject pending reads when releasing reader

Follows whatwg/streams#1168.
--

wpt-commits: 99d74f9529e16ec0722ef11136ab29b9e80fff26
wpt-pr: 32072
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 7, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 8, 2022
Previously, calling releaseLock() on ReadableStreamDefaultReader or
ReadableStreamBYOBReader while there are pending read requests would
throw a TypeError. The specification has been changed[1] to allow this
case, and to reject such pending read requests with a TypeError
instead.

[1] whatwg/streams#1168

Bug: 1287273
Change-Id: Id4013571212e20b0d6ecccdcf68cd6d3927d38b2
aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 12, 2022
Previously, calling releaseLock() on ReadableStreamDefaultReader or
ReadableStreamBYOBReader while there are pending read requests would
throw a TypeError. The specification has been changed[1] to allow this
case, and to reject such pending read requests with a TypeError
instead.

[1] whatwg/streams#1168

Bug: 1287273
Change-Id: Id4013571212e20b0d6ecccdcf68cd6d3927d38b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3750760
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1023012}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
Previously, calling releaseLock() on ReadableStreamDefaultReader or
ReadableStreamBYOBReader while there are pending read requests would
throw a TypeError. The specification has been changed[1] to allow this
case, and to reject such pending read requests with a TypeError
instead.

[1] whatwg/streams#1168

Bug: 1287273
Change-Id: Id4013571212e20b0d6ecccdcf68cd6d3927d38b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3750760
Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1023012}
NOKEYCHECK=True
GitOrigin-RevId: 1e39b24fb910a075f4d337e6bced80f117dfbd7a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants