Skip to content

emit 'error' when write to WriteStream failed#167

Closed
fhinkel wants to merge 3 commits intonode-formidable:masterfrom
fhinkel:master
Closed

emit 'error' when write to WriteStream failed#167
fhinkel wants to merge 3 commits intonode-formidable:masterfrom
fhinkel:master

Conversation

@fhinkel
Copy link
Copy Markdown

@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
Copy Markdown

limeyd commented Oct 17, 2012

Hey is there a reason for this not being merged?

@andrewrk
Copy link
Copy Markdown
Contributor

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

@svnlto
Copy link
Copy Markdown
Contributor

svnlto commented Feb 23, 2013

i believe this has been fixed in #195

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

Nope, this one is not fixed yet

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

Also see #210

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

@fhinkel
Copy link
Copy Markdown
Author

fhinkel commented Jun 25, 2013

When will you merge fc41a83 to master?

@jci-shotola
Copy link
Copy Markdown

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
Copy Markdown

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.

8 participants