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

Verify that aborting a pipe with both preventCancel and preventAbort works #17816

Conversation

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Jul 12, 2019

@ricea
ricea approved these changes Jul 16, 2019
Copy link
Contributor

ricea left a comment

lgtm

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jul 16, 2019

Is it possible to test the sync-vs-async issue, or is that hidden behind other machinery?

@MattiasBuelens

This comment has been minimized.

Copy link
Contributor Author

MattiasBuelens commented Jul 16, 2019

Is it possible to test the sync-vs-async issue, or is that hidden behind other machinery?

I think that's all behind internal machinery. The first observable thing that happens after aborting the signal is the streams becoming unlocked (readable.locked === false && writable.locked === false) and the pipe promise getting rejected.

I suppose you could count how many micro-ticks have passed between those two events with something like:

const controller = new AbortController();
readable.pipeTo(writable, { preventCancel: true, preventAbort: true, signal: controller.signal });
controller.abort();

let microTicks = 0;
while (readable.locked) {
  await Promise.resolve();
  microTicks++;
}

However, the current spec doesn't require any precise number of micro-ticks. For example: step c in "shutdown with an action" says to "wait until every chunk that has been read has been written". If there are no pending writes, then it should be feasible to continue the steps synchronously. But even the reference implementation adds one micro-tick in that case because it uses .then() (source).

I don't think it's worth dictating the amount of micro-ticks that it should take to shutdown the pipe. Perhaps some browser can do it with fewer micro-ticks because of clever optimizations for special streams. Or some browser may need additional round-trips to figure out whether there are still pending writes. As long as the pipe eventually shuts down properly, I think both implementations should be valid. 🙂

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jul 16, 2019

Agreed, it's always been an anti-goal to constrain the number of micro-ticks. In that case I'm happy to merge.

@domenic domenic merged commit 0da3476 into web-platform-tests:master Jul 16, 2019
7 of 10 checks passed
7 of 10 checks passed
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
Azure Pipelines Build #20190712.88 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
@MattiasBuelens MattiasBuelens deleted the MattiasBuelens:readable-stream-abort-pipe-with-preventcancel-and-preventabort branch Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.