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.

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

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

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

ronag

LGTM

mcollina

lgtm

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

Wouldn't error be null?
node/lib/internal/streams/destroy.js
Line 67 in 683754c
constresult=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

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

Comment threadlib/internal/streams/from.js Outdated

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

Comment threadlib/internal/streams/from.js Outdated

ronag reviewed Jan 31, 2021
View reviewed changes

Comment threadlib/internal/streams/from.js Outdated

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 0bce498Compare 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!

Uh oh!

Conversation

benjamingr commented Jan 31, 2021

Uh oh!

Uh oh!

benjamingrJan 31, 2021

Choose a reason for hiding this comment

Uh oh!

benjamingrJan 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!

vweeversJan 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