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

Add buffered data to error.stdout, error.stderr and error.all #271

Merged
merged 4 commits into from Jun 14, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented May 30, 2019

Fixes #228. The issue was that errors on stdin/stdout/stderr were not reported the same way as other errors.

It also adds:

  • error.stdout or error.stderr set with the buffered data when that stream failed
  • error.stream set with the name of the failing stream
  • better error.message on stream errors

Issues

If one stream errors, we do not want to wait for other streams to complete, for the same reason as #270, i.e. those streams might take a long time to complete (or never complete). However this creates the following issues:

  1. we can only get the buffered data of the failing stream, not the other streams. get-stream does not seem to allow to retrieve the currently buffered data.
  2. several streams might fail at the same time when they rely on the same underlying file/TTY. In that case, there's a race condition where error.stream will be set tot the first stream to report an error.
  3. all will always fail right before stdout or stderr, taking precedence over them.

Possible solutions for each of those issues:

  1. Make the other streams fail by emitting an error event on them. Not implemented yet.
  2. Wait for one macrotask (setImmediate()) and check if other streams have failed. Not implemented yet.
  3. Ignore errors on all. This PR currently implements this.

@ehmicky ehmicky force-pushed the feature/stream-errors branch 2 times, most recently from a35e73c to 71096ef Compare May 30, 2019 17:28
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 30, 2019

CI tests fixed. Before moving forward with README and TypeScript, I would need feedback about those 3 problems and possible solutions.

@sindresorhus
Copy link
Owner

error.stream set with the name of the failing stream

Is this really useful? What would someone use it for?

get-stream does not seem to allow to retrieve the currently buffered data.

We could potentially add this to get-stream if it simplifies the implementation here?

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 3, 2019

error.stream is not super useful, I will remove it from this PR. It's also used in error.message which shows which stream failed, but we could just not indicate which stream failed if it simplifies this PR.

I think once one stream fails, we don't want the other streams to keep going, i.e. we can close or destroy them right away. If we do this by emitting an error event on them, then await them, get-stream will reject their promises and set their bufferedData. That would require no need to change get-stream. What do you think?

@sindresorhus
Copy link
Owner

error.stream is not super useful, I will remove it from this PR. It's also used in error.message which shows which stream failed, but we could just not indicate which stream failed if it simplifies this PR.

👍

What do you think?

👍

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 10, 2019

Fixed. Summary of changes:

  • on either process error or any stream error, make error.stdout, error.stderr and error.all contain the buffered stream data.
  • remove error.stream and error.bufferedData (undocumented error properties)

It turns out calling stream.emit('error') (as suggested in the previous comment) is not needed. stream.destroy() is enough to make the buffered data available. This meant the following changes:

  • stream.destroy() is called a little earlier, which is not an issue since it's not used after that point
  • stream.destroy() does not need to be called when all streams successfully completed.

This PR enables some little refactoring, but I will do it in a follow-up PR to keep the diff simple.

No TypeScript types have been changed, but I've added documentation for the new buffered data behavior.

index.js Outdated Show resolved Hide resolved
@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 14, 2019

Fixed.

@sindresorhus sindresorhus merged commit bad2140 into master Jun 14, 2019
@sindresorhus sindresorhus deleted the feature/stream-errors branch June 14, 2019 08:26
@ehmicky ehmicky changed the title Improve error handling of streams Add buffered data to error.stdout, error.stderr and error.all Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve stream error handling
2 participants