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

emit 'error' when write to WriteStream failed #167

Closed
wants to merge 3 commits into from

Conversation

fhinkel
Copy link

@fhinkel fhinkel commented Sep 12, 2012

We tried to write to a non-existing directory, which resulted in an uncaught exception. I added an error handler for WriteStream, so 'error' is emitted when writing the file failed.

@limeyd
Copy link

limeyd commented Oct 17, 2012

Hey is there a reason for this not being merged?

@andrewrk
Copy link
Contributor

2 reasons: 1. lacks test case. 2. Wrapping the error in a new error is incorrect.

@svnlto
Copy link
Contributor

svnlto commented Feb 23, 2013

i believe this has been fixed in #195

@svnlto svnlto closed this Feb 23, 2013
@egirshov
Copy link
Contributor

Nope, this one is not fixed yet

@egirshov egirshov reopened this Feb 23, 2013
@tim-smart
Copy link
Contributor

Also see #210

While not directly related, fixing that issue resulted in fixing this one as well. Reference fc41a83

@fhinkel
Copy link
Author

fhinkel commented Jun 25, 2013

When will you merge fc41a83 to master?

@jci-shotola
Copy link

I agree that this is an issue that needs to be addressed, since I cannot find any way to capture an unhandled exception from the stream writer. This pull request is definitely on the right track, but it's flawed. Once the error is raised, the parser's onEnd event is called which sets ended === true. If ended is set to true before _error() is called the error is eaten. Something needs to be done to be able to bubble the error past where ended === true eats it. Once that happens, I think the fix will do the job.

@jpapillon
Copy link

It happens, for example, when the disk is full. This should definitely be merged. At least commit c3e6ca6.

@fhinkel fhinkel closed this Jun 5, 2018
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.

None yet

8 participants