stream: improve Readable.from error handling by benjamingr · Pull Request #37158 · nodejs/node

benjamingr · 2021-01-31T15:25:29Z

This pull request aligns the behaviour of Readable.from with other stream behaviours namely:

Errors thrown in the stream now propagate via .throw and not via .on and reach catch blocks. This means things like addAbortSignal work with Readable.from now.
The generator is always closed, even if next was not called - this is because the previous implementation had a timing bug where if next was called and the stream was destroyed in the same tick - finally blocks would not run.

lib/internal/streams/from.js

benjamingr · 2021-01-31T15:27:35Z

lib/internal/streams/from.js

  };

-  async function close() {
+  async function close(error) {


This is tricky and I want feedback on the semantics, basically:

If we had an error ( .destroy(err) ) we call .throw - this aligns the behaviour of .from with other methods and means stuff like addAbortSignal automagically "works".

If after calling throw the iterator did not finish - we also call .return

benjamingr · 2021-01-31T15:28:05Z

lib/internal/streams/from.js

      if (done) {
        readable.push(null);
-      } else if (readable.destroyed) {
-        await close();


This check is no longer necessary without needToClose since _destroy is called and if the stream is destroyed the iterator is closed.

ronag

LGTM

lib/internal/streams/from.js

mcollina

lgtm

vweevers · 2021-01-31T23:45:37Z

lib/internal/streams/from.js


-  async function close() {
+  async function close(error) {
+    const hadError = typeof error !== 'undefined';


Wouldn't error be null?

node/lib/internal/streams/destroy.js

Line 67 in 683754c

const result = self._destroy(err || null, (err) => {

aduh95 · 2021-02-01T14:31:49Z

I've rebased and force pushed to solve to git conflicts introduced by my PR.

benjamingr · 2021-02-01T17:07:11Z

@aduh95 very kind of you, thanks 🙇‍♂️

nodejs-github-bot · 2021-02-01T17:17:47Z

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

nodejs-github-bot · 2021-02-02T08:29:29Z

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

nodejs-github-bot · 2021-02-02T12:13:11Z

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

benjamingr · 2021-02-02T12:14:59Z

@mcollina I might be a bit overcautious* - but I prefer to err on the side of caution.

What steps can I take to ensure there is less breakage here?

Thoughts:

Tag this as semver minor eventhough it's a bug so it's more obvious.
Fire off a CITGM run. (Edit: started CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2611/ )

Anything else?

(since the old behaviour was just an overlooked bug/edge-case and probably not relied on)

benjamingr · 2021-02-02T13:35:31Z

Putting "blocked" so this doesn't land by accident in the meantime.

benjamingr · 2021-02-02T17:51:35Z

I went through most CITGM failures and there don't seem to be new/related ones

nodejs-github-bot · 2021-02-02T17:52:00Z

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

nodejs-github-bot · 2021-02-02T22:25:02Z

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

nodejs-github-bot · 2021-02-03T08:38:29Z

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

mcollina · 2021-02-03T08:45:32Z

I do not think we have any modules using this in CITGM. It's not touching any old/critical APIs, so it should be good to land.

benjamingr · 2021-02-03T10:05:19Z

Thanks matteo. Landed in 03380bc

PR-URL: #37158 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable Changes: * crypto: * add keyObject.export() 'jwk' format option (Filip Skokan) #37081 * deps: * upgrade to libuv 1.41.0 (Colin Ihrig) #37360 * doc: * add dmabupt to collaborators (Xu Meng) #37377 * refactor fs docs structure (James M Snell) #37170 * fs: * add fsPromises.watch() (James M Snell) #37179 * use a default callback for fs.close() (James M Snell) #37174 * add AbortSignal support to watch (Benjamin Gruenbaum) #37190 * perf_hooks: * introduce createHistogram (James M Snell) #37155 * stream: * improve Readable.from error handling (Benjamin Gruenbaum) #37158 * timers: * introduce setInterval async iterator (linkgoron) #37153 * tls: * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070

Notable Changes: * crypto: * add keyObject.export() jwk format option (Filip Skokan) #37081 * deps: * upgrade to libuv 1.41.0 (Colin Ihrig) #37360 * doc: * add dmabupt to collaborators (Xu Meng) #37377 * refactor fs docs structure (James M Snell) #37170 * fs: * add fsPromises.watch() (James M Snell) #37179 * use a default callback for fs.close() (James M Snell) #37174 * add AbortSignal support to watch (Benjamin Gruenbaum) #37190 * perf_hooks: * introduce createHistogram (James M Snell) #37155 * stream: * improve Readable.from error handling (Benjamin Gruenbaum) #37158 * timers: * introduce setInterval async iterator (linkgoron) #37153 * tls: * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070

PR-URL: #37406 Notable Changes: * crypto: * add keyObject.export() jwk format option (Filip Skokan) #37081 * deps: * upgrade to libuv 1.41.0 (Colin Ihrig) #37360 * doc: * add dmabupt to collaborators (Xu Meng) #37377 * refactor fs docs structure (James M Snell) #37170 * fs: * add fsPromises.watch() (James M Snell) #37179 * use a default callback for fs.close() (James M Snell) #37174 * add AbortSignal support to watch (Benjamin Gruenbaum) #37190 * perf_hooks: * introduce createHistogram (James M Snell) #37155 * stream: * improve Readable.from error handling (Benjamin Gruenbaum) #37158 * timers: * introduce setInterval async iterator (linkgoron) #37153 * tls: * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070

benjamingr added the stream Issues and PRs related to the stream subsystem. label Jan 31, 2021

benjamingr commented Jan 31, 2021

View reviewed changes

lib/internal/streams/from.js Show resolved Hide resolved

benjamingr commented Jan 31, 2021

View reviewed changes

ronag approved these changes Jan 31, 2021

View reviewed changes

ronag reviewed Jan 31, 2021

View reviewed changes

lib/internal/streams/from.js Outdated Show resolved Hide resolved

ronag reviewed Jan 31, 2021

View reviewed changes

lib/internal/streams/from.js Outdated Show resolved Hide resolved

mcollina approved these changes Jan 31, 2021

View reviewed changes

vweevers reviewed Jan 31, 2021

View reviewed changes

benjamingr added 4 commits February 1, 2021 15:27

stream: improve Readable.from error handling

e7adbab

fixup! lint

0f9324a

fixup! comment

11fe4f2

fixup! code review

0bce498

aduh95 force-pushed the fix-async-iterator-errors branch from 3f712a5 to 0bce498 Compare February 1, 2021 14:30

benjamingr added the blocked PRs that are blocked by other issues or PRs. label Feb 2, 2021

mcollina removed the blocked PRs that are blocked by other issues or PRs. label Feb 3, 2021

benjamingr closed this Feb 3, 2021

benjamingr added a commit that referenced this pull request Feb 3, 2021

stream: improve Readable.from error handling

03380bc

PR-URL: #37158 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

benjamingr deleted the fix-async-iterator-errors branch February 3, 2021 11:23

benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 3, 2021

danielleadams pushed a commit that referenced this pull request Feb 16, 2021

stream: improve Readable.from error handling

1fea051

PR-URL: #37158 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This was referenced Feb 16, 2021

v15.9.0 proposal #37405

Closed

v15.9.0 proposal #37406

Merged

targos added dont-land-on-v12.x labels Aug 8, 2021

Uh oh!

Conversation

benjamingr commented Jan 31, 2021

Uh oh!

Uh oh!

benjamingr Jan 31, 2021

Choose a reason for hiding this comment

Uh oh!

benjamingr Jan 31, 2021

Choose a reason for hiding this comment

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

vweevers Jan 31, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Feb 1, 2021

Uh oh!

benjamingr commented Feb 1, 2021

Uh oh!

nodejs-github-bot commented Feb 1, 2021

Uh oh!

nodejs-github-bot commented Feb 2, 2021

Uh oh!

nodejs-github-bot commented Feb 2, 2021

Uh oh!

benjamingr commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr commented Feb 2, 2021

Uh oh!

benjamingr commented Feb 2, 2021

Uh oh!

nodejs-github-bot commented Feb 2, 2021

Uh oh!

nodejs-github-bot commented Feb 2, 2021

Uh oh!

nodejs-github-bot commented Feb 3, 2021

Uh oh!

mcollina commented Feb 3, 2021

Uh oh!

benjamingr commented Feb 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

benjamingr commented Feb 2, 2021 •

edited

Loading