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

Fix pipe not aborting when both preventAbort and preventCancel are set #1006

Conversation

MattiasBuelens
Copy link
Collaborator

@MattiasBuelens MattiasBuelens commented Jul 12, 2019

When both flags are set, the abortAlgorithm passes an empty list of actions to "wait for all". This helper should immediately resolve the returned promise when no promises are given, but instead the promise remained pending forever.

This fixes the reference implementation of the "wait for all" helper to resolve correctly when given an empty list of promises. This matches the spec change in w3ctag/promises-guide#58.

Originally reported in this comment on #1004.

WPT: web-platform-tests/wpt#17816

@ricea
Copy link
Collaborator

ricea commented Jul 16, 2019

See my comment at w3ctag/promises-guide#58 (comment) about whether or not successSteps is called synchronously. We should probably make this match whatever resolution we choose there.

@domenic
Copy link
Member

domenic commented Jul 16, 2019

We can use https://nodejs.org/dist/latest-v12.x/docs/api/globals.html#globals_queuemicrotask_callback / https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/queueMicrotask to implement the promises guide revision.

@MattiasBuelens
Copy link
Collaborator Author

Looks like jsdom doesn't expose queueMicrotask on its window yet... I guess we could expose it ourselves in run-web-platform-tests.js?

  const failures = await wptRunner(testsPath, {
    rootURL: 'streams/',
    setup(window) {
      window.queueMicrotask = queueMicrotask;
      window.eval(bundledJS);
    },
    // ...
  });

@domenic
Copy link
Member

domenic commented Jul 16, 2019

Bah, yeah, that's probably simplest for now.

When both flags are set, the abortAlgorithm passes an empty list of actions to "wait for all".
This helper should immediately resolve the returned promise when no promises are given,
but instead the promise remained pending forever. Fix it.
@MattiasBuelens MattiasBuelens force-pushed the fix-pipe-abort-with-preventcancel-and-preventabort branch from bbafe17 to a9a6341 Compare July 16, 2019 23:01
@domenic domenic merged commit e4d3b1a into whatwg:master Jul 29, 2019
@MattiasBuelens MattiasBuelens deleted the fix-pipe-abort-with-preventcancel-and-preventabort branch July 29, 2019 18:22
MattiasBuelens added a commit to MattiasBuelens/web-streams-polyfill that referenced this pull request Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants