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

Docs: add exception handling examples in stream #4491

Closed
stevemao opened this issue Dec 30, 2015 · 9 comments
Closed

Docs: add exception handling examples in stream #4491

stevemao opened this issue Dec 30, 2015 · 9 comments
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.

Comments

@stevemao
Copy link
Contributor

It is tempting to do something like this:

var error = new Error('some error');

stream
  .pipe(through(function(chunk) {
    throw error ;
  }))
  .on('error', function(err) {
    assert.equal(error, err);
  });

For me because in promise throwing an exception equals rejection.

The docs should tell what really happens when exceptions are thrown in these kind of functions (_read, _write, _writev, _transform, _flush).

@mscdex mscdex added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Dec 31, 2015
@stevemao
Copy link
Contributor Author

stevemao commented Jan 1, 2016

ping @nodejs/documentation :)

@Qard
Copy link
Member

Qard commented Jan 1, 2016

I think that's just a wrong expectation. Core generally avoids behaviour changes like converting a thrown error to an emitted one. If the throw happened in an async thing within the through handler it'd be outside what try/catch can handle, placing it in domains territory, which we'd like to avoid. Not sure how exactly that should be expressed in documentation though.

@stevemao
Copy link
Contributor Author

stevemao commented Jan 2, 2016

Yeah, I don't want to change the behaviour. Just adding docs :)

Sent from my iPhone

On 2 Jan 2016, at 10:49 AM, Stephen Belanger notifications@github.com wrote:

I think that's just a wrong expectation. Core generally avoids behaviour changes like converting a thrown error to an emitted one. If the throw happened in an async thing within the through handler it'd be outside what try/catch can handle, placing it in domains territory, which we'd like to avoid. Not sure how exactly that should be expressed in documentation though.


Reply to this email directly or view it on GitHub.

@ryansobol
Copy link
Contributor

@stevemao I'd love to see your understanding of this behavior written up and submitted as a PR. :)

@stevemao
Copy link
Contributor Author

I can try. But it would take longer for me than people who already know how it internally works.

@mcollina
Copy link
Member

👍 in documenting this. It is obvious for the seasoned node devs, but really frustrating for newbies.

Also you need to document that errors are not propagated through the stream chain:

var error = new Error('some error');

stream
  .pipe(through2(function(chunk, enc, cb) {
     cb(null, chunk)
  }))
  .on('error', function(err) {
    // this will never happen
    assert.equal(error, err);
  });

stream.emit('error', error)

and you should use pump.

@Trott
Copy link
Member

Trott commented Jul 5, 2017

Hmmm.. at first blush, this seems like maybe a good first contribution, but then a quick look at nodejs/docs#82 suggests that maybe this requires an experienced contributor with a thick skin. Thoughts or volunteers on making this happen?

@BridgeAR
Copy link
Member

@mafintosh @mcollina is this resolved due to adding pump?

@mcollina
Copy link
Member

Yes, I think so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants