Skip to content

Loading…

emit 'error' when write to WriteStream failed #167

Open
wants to merge 3 commits into from

8 participants

@fhinkel

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

Hey is there a reason for this not being merged?

@andrewrk
Collaborator

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

@svnlto
Collaborator

i believe this has been fixed in #195

@svnlto svnlto closed this
@egirshov
Collaborator

Nope, this one is not fixed yet

@egirshov egirshov reopened this
@tim-smart
Collaborator

Also see #210

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

@fhinkel

When will you merge fc41a83 to master?

Franziska Hi... added some commits
@jci-shotola

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 12, 2012
  1. @fhinkel
Commits on Jun 26, 2013
  1. emit previous error, not new Error

    Franziska Hinkelmann committed
  2. catch exception if fs.unlink does not work

    Franziska Hinkelmann committed
Showing with 21 additions and 4 deletions.
  1. +5 −1 lib/file.js
  2. +16 −3 lib/incoming_form.js
View
6 lib/file.js
@@ -45,7 +45,11 @@ File.prototype._backwardsCompatibility = function() {
};
File.prototype.open = function() {
- this._writeStream = new WriteStream(this.path);
+ var self = this;
+ self._writeStream = new WriteStream(this.path);
+ self._writeStream.on('error', function (err) {
+ self.emit('error', err);
+ });
};
File.prototype.write = function(buffer, cb) {
View
19 lib/incoming_form.js
@@ -202,11 +202,17 @@ IncomingForm.prototype.handlePart = function(part) {
hash: self.hash
});
+
+
this.emit('fileBegin', part.name, file);
file.open();
this.openedFiles.push(file);
-
+
+ file.on('error', function(error) {
+ self._error(error);
+ });
+
part.on('data', function(buffer) {
self.pause();
file.write(buffer, function() {
@@ -257,8 +263,15 @@ IncomingForm.prototype._error = function(err) {
this.emit('error', err);
this.openedFiles.forEach(function(file) {
- file._writeStream.destroy();
- setTimeout(fs.unlink, 0, file.path);
+ file._writeStream.destroy();
+
+ setTimeout(function() {
+ try{
+ fs.unlink();
+ } catch (e) {
+ console.log("Error unlinking file: " + err);
+ }
+ }, 0, file.path);
});
};
Something went wrong with that request. Please try again.