stream: fix error handling with async iteration by julien-f · Pull Request #20329 · nodejs/node

julien-f · 2018-04-26T09:56:28Z

Fix an issue when an error was emitted by the stream before
iterator.next() is called.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

I don't have the time to figure out how to run the tests, sorry.

Fix an issue when an error was emitted by the stream before `iterator.next()` is called.

benjamingr · 2018-04-26T11:02:23Z

+ read() {}
+ })
+
+ let err


Nit, missing semicolon. Please run make lint :)

benjamingr · 2018-04-26T11:03:22Z

+ } catch (e) {
+ err = e;
+ }
+ assert.strictEqual(err.message, 'kaboom');


Can we assert that we have reference equality and assert Node doesn't mutate the error (basically define the error above and assert that strictEqual on the instance?
Also - you can use the newly added assert.rejects (although no obligation)

benjamingr · 2018-04-26T11:03:52Z

+ console.log('call next() after error');
+ const readable = new Readable({
+ read() {}
+ })


Nit, missing semicolon

benjamingr

First of all - good to see you here.

Second - thank you for doing this - any feedback about the Symbol.asyncIterator feature is greatly appreciated.

Also - Good catch! I've left some comments. You're welcome to address them (and maybe amend the PR description to meet our standard format). If you won't a collaborator would do this for you when landing it.

julien-f · 2018-04-26T12:25:11Z

Thanks for your welcome and comments 😃

I've fixed the code accordingly, but I still cannot run the tests so I'm not sure it works 😜

benjamingr · 2018-04-26T12:29:53Z

@julien-f what issue are you having with running the tests?

If you're having trouble running the tests let's:

figure out what the issue running the tests is
file an issue to fix that so that other people don't have it

We really want to avoid having contributors climb technical barriers in orders to contribute.

In addition, I've kicked a CI for it: https://ci.nodejs.org/job/node-test-pull-request/14524/console

julien-f · 2018-04-26T12:34:08Z

@benjamingr No particular issues, I simply have never compiled Node on my machine and I don't have time to figure it out for the moment.

benjamingr · 2018-04-26T12:37:46Z

There seem to be some lint errors in the tests: https://ci.nodejs.org/job/node-test-linter/18544/console

julien-f · 2018-04-26T12:40:26Z

On it, sorry for this, I missed it 😳

benjamingr · 2018-04-26T12:47:32Z

Don't worry about it, new CI: https://ci.nodejs.org/job/node-test-pull-request/14525/console

Also note that while cloning and building Node sounds like a lot of work - it's actually not nearly as complicated as it appears. Basically:

git clone https://github.com/nodejs/node
cd node
./configure
make -j4

Assuming you have Python (comes built in on mac), git (which is common for developers) and a C++ compiler (which is an xcode-select --install away if you've never installed it)
We have detailed instructions here

julien-f · 2018-04-26T14:01:54Z

Ok, I successfully built it with ./configure --openssl-no-asm.

benjamingr · 2018-04-26T14:04:45Z

CI is green, what's going to happen next is that this will sit for 72 hours so collaborators have a chance to review it and then we'll land it if no one objects. (since it already has one approval).

Any future work on async iterator support would be appreciated :)

TimothyGu

Good catch!

benjamingr · 2018-04-27T15:30:23Z

Added [dont-land-on] labels for versions of Node that don't support async iterators.

BridgeAR · 2018-04-29T15:23:23Z

Landed in 9c48926 🎉

Fix an issue when an error was emitted by the stream before `iterator.next()` is called. PR-URL: nodejs#20329 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

benjamingr · 2018-04-29T15:27:25Z

Welcome to the Node contributor ranks @julien-f ! Any follow-up work that you'd like to work up with in the async iterator space would be very appreciated and if you need any help or resources to better contribute by all means please reach out.

Fix an issue when an error was emitted by the stream before `iterator.next()` is called. PR-URL: #20329 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

stream: fix error handling with async iteration
0b529b2
Fix an issue when an error was emitted by the stream before `iterator.next()` is called.

benjamingr reviewed Apr 26, 2018
View reviewed changes

benjamingr approved these changes Apr 26, 2018
View reviewed changes

benjamingr added stream Issues and PRs related to the stream subsystem. promises Issues and PRs related to ECMAScript promises. labels Apr 26, 2018

julien-f added 2 commits April 26, 2018 14:13

coding style
0e5995a

test by error identity
fea77e0

add missing semi
c8c410a

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 26, 2018

TimothyGu approved these changes Apr 26, 2018
View reviewed changes

jasnell approved these changes Apr 27, 2018
View reviewed changes

benjamingr added dont-land-on-v4.x labels Apr 27, 2018

cjihrig approved these changes Apr 27, 2018
View reviewed changes

mcollina mentioned this pull request Apr 27, 2018
ReadableAsyncIterator error not handled #20347
Closed

apapirovski approved these changes Apr 28, 2018
View reviewed changes

BridgeAR closed this Apr 29, 2018

julien-f deleted the stream-async-iteration branch April 30, 2018 08:38

MylesBorins mentioned this pull request May 8, 2018
v10.1.0 proposal #20606
Merged

Uh oh!

Uh oh!

Conversation

julien-f commented Apr 26, 2018

Checklist

Uh oh!

benjamingrApr 26, 2018

Choose a reason for hiding this comment

Uh oh!

benjamingrApr 26, 2018

Choose a reason for hiding this comment

Uh oh!

benjamingrApr 26, 2018

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

julien-f commented Apr 26, 2018

Uh oh!

benjamingr commented Apr 26, 2018

Uh oh!

julien-f commented Apr 26, 2018

Uh oh!

benjamingr commented Apr 26, 2018

Uh oh!

julien-f commented Apr 26, 2018

Uh oh!

benjamingr commented Apr 26, 2018

Uh oh!

julien-f commented Apr 26, 2018

Uh oh!

benjamingr commented Apr 26, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Apr 27, 2018

Uh oh!

BridgeAR commented Apr 29, 2018

Uh oh!

benjamingr commented Apr 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants