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

stream: fix writableStream.abort() #44327

Merged

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Aug 21, 2022

This includes:

  • Fixing the following step in writableStream.abort(reason). Passing the reason was missing.

WritableStreamAbort(stream, reason) performs the following steps:

  1. Signal abort on stream.[[controller]].[[signal]] with reason.
  • Leaving a TODO to remove the internal abortReason property of WritableStreamDefaultController in a follow-up.

Refs: https://streams.spec.whatwg.org/#writable-stream-abort

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

This includes:

- Fixing `writableStream.abort(reason)`. Passing the reason was missing.

- Leaving a TODO to remove the internal abortReason property of
  WritableStreamDefaultController.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Aug 21, 2022
@daeyeon
Copy link
Member Author

daeyeon commented Aug 21, 2022

/cc @nodejs/whatwg-stream

@mscdex
Copy link
Contributor

mscdex commented Aug 21, 2022

I think this should add a test that checks that reason is passed?

@daeyeon
Copy link
Member Author

daeyeon commented Aug 21, 2022

There is a test and this fix will get it passed.

test(t => {
let ctrl;
const ws = new WritableStream({start(c) { ctrl = c; }});
const e = Error('hello');
assert_true(ctrl.signal instanceof AbortSignal);
assert_false(ctrl.signal.aborted);
assert_equals(ctrl.signal.reason, undefined, 'signal.reason before abort');
ws.abort(e);
assert_true(ctrl.signal.aborted);
assert_equals(ctrl.signal.reason, e);
}, 'WritableStreamDefaultController.signal');

lpinca
lpinca approved these changes Aug 21, 2022
@mscdex
Copy link
Contributor

mscdex commented Aug 22, 2022

There is a test and this fix will get it passed.

Why is the main branch passing all tests currently then?

@RaisinTen
Copy link
Member

There is a test and this fix will get it passed.

Why is the main branch passing all tests currently then?

I think that's because the test was expected to fail previously on main

"writable-streams/aborting.any.js": {
"fail": {
"expected": ["WritableStreamDefaultController.signal"]
}
}

@daeyeon
Copy link
Member Author

daeyeon commented Aug 22, 2022

Why is the main branch passing all tests currently then?

Understand the query. Unlike other Node.js APIs, developing Web compatible APIs is allowed to use preparing tests first and developing later approach (TDD) if there are Web Platform Tests we can rely on for basic compatibility testing. When running WPT, our WPT harness refers to a JSON status file describing tests we don't support yet. And it ignores a test result if a certain test is marked as expected to fail.

This updates test/wpt/status/streams.json so that the result previously ignored can be reflected in the final result.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 29, 2022
@joyeecheung
Copy link
Member

Please do not use resume to re-run the tests - a proper rebase from the CI is necessary to include #44359

@daeyeon
Copy link
Member Author

daeyeon commented Aug 29, 2022

Thanks for the information. I mistook that the resume build also includes the rebase process.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Contributor

@daeyeon daeyeon added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 5, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 5, 2022
@nodejs-github-bot nodejs-github-bot merged commit 4af0a26 into nodejs:main Sep 5, 2022
57 checks passed
@nodejs-github-bot
Copy link
Contributor

Landed in 4af0a26

@daeyeon daeyeon deleted the main.aboring-wstream-220815.Mon.9fc4 branch Sep 5, 2022
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
This includes:

- Fixing `writableStream.abort(reason)`. Passing the reason was missing.

- Leaving a TODO to remove the internal abortReason property of
  WritableStreamDefaultController.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: nodejs#44327
Refs: https://streams.spec.whatwg.org/#writable-stream-abort
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
This includes:

- Fixing `writableStream.abort(reason)`. Passing the reason was missing.

- Leaving a TODO to remove the internal abortReason property of
  WritableStreamDefaultController.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44327
Refs: https://streams.spec.whatwg.org/#writable-stream-abort
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 26, 2022
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
This includes:

- Fixing `writableStream.abort(reason)`. Passing the reason was missing.

- Leaving a TODO to remove the internal abortReason property of
  WritableStreamDefaultController.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44327
Refs: https://streams.spec.whatwg.org/#writable-stream-abort
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@juanarbol
Copy link
Member

This is not landing cleanly in the v16.x release line; would yo mind rebasing to v16.x?

@juanarbol juanarbol added the backport-requested-v16.x PRs awaiting manual backport to the v16.x-staging branch. label Oct 3, 2022
@daeyeon
Copy link
Member Author

daeyeon commented Oct 3, 2022

Backporting this is pending now since it depends on both #43455 and #44234.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v16.x PRs awaiting manual backport to the v16.x-staging branch. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants