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

Seemingly redundant code in fileStream error handler #103

Closed
chiihuang opened this issue Mar 7, 2015 · 4 comments
Closed

Seemingly redundant code in fileStream error handler #103

chiihuang opened this issue Mar 7, 2015 · 4 comments

Comments

@chiihuang
Copy link

The following code in Line 151-151 seems redundant:

fileStream.on('error', function(error) {
  // trigger "file error" event
  if (options.onError) { options.onError(error, next); }
  else next(error);
});

Because in Line 167-170, we got fileStream.on('error', ...) again. No matter options.inMemory is true or not, fileStream.on('error', ...) always works with the same error handler.

if (options.inMemory)
  fileStream.on('error', onFileStreamError );
else 
  ws.on('error', onFileStreamError );

Is this simply a miss or is there any reason to do fileStream.on('error', ...) twice?

@jpfluger
Copy link
Contributor

jpfluger commented Mar 7, 2015

Hmm... you did spot something: fileStream is the read-stream; ws is the write-stream. That means line 167-168 is redundant. All streams will be read but only the write-to-disk stream will go to disk. I think you can have separate errors for read or writes.

@chiihuang
Copy link
Author

So if fileStream and ws both crash, onError will be fired twice, right?

Would it be better if we rename and duplicate onError to onReadFileStreamError and onWriteFileStreamError?

@jpfluger
Copy link
Contributor

jpfluger commented Mar 7, 2015

I don't know without testing. It's true that the writestream handles the filestream, so I would guess the writestream would emit one error. If that's true, then the code segment at 151 goes away but then leave lines 167-170.

@LinusU
Copy link
Member

LinusU commented Jul 18, 2015

All code is rewritten as of 1.0.0, this should be solved now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants