stream: fix `writableStream.abort()` by daeyeon · Pull Request #44327 · nodejs/node · GitHub

This includes:

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

WritableStreamAbort(stream, reason) performs the following steps:

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

/cc @nodejs/whatwg-stream

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

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

node/test/fixtures/wpt/streams/writable-streams/aborting.any.js

Lines 1380 to 1391 in a99fa50

    
           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');

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

Why is the main branch passing all tests currently then?

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

node/test/wpt/status/streams.json

Lines 110 to 114 in e5fb452

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

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.

lgtm

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

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

CI: https://ci.nodejs.org/job/node-test-pull-request/46403/

Landed in 4af0a26

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>

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

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

stream: fix writableStream.abort()

b9e7f6f

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 added needs-ci PRs that need a full CI run. web streams labels Aug 21, 2022

lpinca approved these changes Aug 21, 2022

View reviewed changes

mcollina approved these changes Aug 22, 2022

View reviewed changes

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2022