This currently breaks test/parallel/test-fs-write-stream-err.js because it overwrites fs.write which the fs.WriteStream no longer depends on. I hope someone can advise me how to deal with this problem.

Thanks for starting on this. fs.js needs some love and cleanup.

CI: https://ci.nodejs.org/job/node-test-pull-request/345/

@trevnorris Thanks. I'm quite invested in good file system performance right now, so you can expect me to continue to contribute here. Btw, CI is too soon, as one test is currently broken because it replaces fs.write with its own version (but our stream no longer depends on fs.write). Advice on this would be appreciated (especially if possible today, as I'm about to go offline for 10 days on vacation).

@ronkorving Is the offending test still relevant? If not then just delete it. If it is then we'll figure out a way to rewrite it. If you have to leave before finishing the test then I can look into finishing that for you.

@trevnorris To be honest, I fear I'm running short on time. Travel preparations will take up most of my time tomorrow (typing this from a phone, won't see my computer again today), so by all means have a look at it and do as you please :) Alternatively, advise me but allow this to sit until I'm back on Oct 3rd. Either is fine by me.

The failing test was introduced in d65832c. Reason for it failing isn't obvious to me yet. Will take a look later.

@trevnorris it fails because the test depends on fs.write being used by WriteStream which is no longer the case in this PR.

@ronkorving The description of the original commit is "Close the file descriptor when a write operation fails." Does that still apply? If so, can a similar test be created?

Either way it seems like this failing test will need to be removed.

"Close the file descriptor when a write operation fails."

why not do a quick backoff and retry (maybe after a small setTimeout) first before closing the fd?

Is fixing test-fs-write-stream-err.js the last outstanding issue?

I think so. I'll try to give that some love soon, but have been a bit occupied I'm afraid.

The test ensures that if there are errors passed to the callback (write failures), the stream closes the file descriptor and emits the error. That seems like a good test to have, but it achieves this by creating a fake write error in fs.write (which is public, so it can do that). In the new WriteStream implementation, it no longer uses fs.write, so I'm not sure how I could inject a fake error. Suggestions welcome (as I continue looking into this).

Here's a diff for a fix:

diff --git a/test/parallel/test-fs-write-stream-err.js b/test/parallel/test-fs-write-stream-err.js
index bc289d4..7d64014 100644
--- a/test/parallel/test-fs-write-stream-err.js
+++ b/test/parallel/test-fs-write-stream-err.js
@@ -2,6 +2,7 @@
 var common = require('../common');
 var assert = require('assert');
 var fs = require('fs');
+var binding = process.binding('fs');

 common.refreshTmpDir();

@@ -10,21 +11,18 @@ var stream = fs.createWriteStream(common.tmpDir + '/out', {
 });
 var err = new Error('BAM');

-var write = fs.write;
+var writeBuffer = binding.writeBuffer;
 var writeCalls = 0;
-fs.write = function() {
+binding.writeBuffer = function() {
   switch (writeCalls++) {
     case 0:
-      console.error('first write');
       // first time is ok.
-      return write.apply(fs, arguments);
+      console.error('first write');
+      return writeBuffer.apply(this, arguments);
     case 1:
       // then it breaks
       console.error('second write');
-      var cb = arguments[arguments.length - 1];
-      return process.nextTick(function() {
-        cb(err);
-      });
+      return process.nextTick(arguments[5].oncomplete, err);
     default:
       // and should not be called again!
       throw new Error('BOOM!');

Smart! Why didn't I think of overriding the methods on the binding object? Thanks @trevnorris :)

@trevnorris I'm a bit new to this, so please bear with me. I find this very strange, with your patch I can make the test fail or succeed, depending on how I run it:

Success:

$ ./node test/parallel/test-fs-write-stream-err.js && echo $?
first write
first cb
second write
fs.close 17 17
second cb
error handler
0

Failure:

$ python tools/test.py -v --mode=release parallel/test-fs-write-stream-err
[00:00|%   0|+   0|-   0]: release test-fs-write-stream-err # out/Release/node /Users/rk/Projects/node/test/parallel/test-fs-write-stream-err.js
=== release test-fs-write-stream-err ===                   
Path: parallel/test-fs-write-stream-err
/Users/rk/Projects/node/test/parallel/test-fs-write-stream-err.js:28
      throw new Error('BOOM!');
      ^

Error: BOOM!
    at Object.binding.writeBuffer (/Users/rk/Projects/node/test/parallel/test-fs-write-stream-err.js:28:13)
    at Object.fs.writeSync (fs.js:663:20)
    at SyncWriteStream.write (fs.js:1990:6)
    at Console.warn (console.js:44:16)
    at Object.binding.writeBuffer (/Users/rk/Projects/node/test/parallel/test-fs-write-stream-err.js:24:15)
    at Object.fs.writeSync (fs.js:663:20)
    at SyncWriteStream.write (fs.js:1990:6)
    at Console.warn (console.js:44:16)
    at Object.binding.writeBuffer (/Users/rk/Projects/node/test/parallel/test-fs-write-stream-err.js:20:15)
    at write (fs.js:1866:11)
Command: out/Release/node /Users/rk/Projects/node/test/parallel/test-fs-write-stream-err.js
[00:00|% 100|+   0|-   1]: Done 

Any idea how this could happen or how the test can behave in 2 completely different ways depending on how its run?

PS: I've pushed the test update to this PR, so you can try it out.

friendly poke @trevnorris

I'm not sure.

@bnoordhuis I believe you were the one to introduce this test. have any thoughts?

What happens with ./node test/parallel/test-fs-write-stream-err.js | cat? The test monkey-patches fs.write and expects it to be called n times. I speculate there is something in core calling fs.write when stdout/stderr is a pipe.

@bnoordhuis thanks. that doesn't trigger it directly but the following seems to:

./node test/parallel/test-fs-write-stream-err.js 2> /tmp/blah.out

By changing console.error() to process._rawDebug() I get the following output from the file:

first write
first cb
second write
fs.close 11 11
/var/projects/node/test/parallel/test-fs-write-stream-err.js:28
      throw new Error('BOOM!');
      ^

Error: BOOM!
    at ...

Ah yes. binding.writeBuffer() is being called an additional time with a different file descriptor. Here's a patch to work around that case:

diff --git a/test/parallel/test-fs-write-stream-err.js b/test/parallel/test-fs-write-stream-err.js
index 7d64014..b97fef9 100644
--- a/test/parallel/test-fs-write-stream-err.js
+++ b/test/parallel/test-fs-write-stream-err.js
@@ -14,6 +14,9 @@ var err = new Error('BAM');
 var writeBuffer = binding.writeBuffer;
 var writeCalls = 0;
 binding.writeBuffer = function() {
+  if (arguments[0] != stream.fd)
+    return writeBuffer.apply(this, arguments);
+
   switch (writeCalls++) {
     case 0:
       // first time is ok.

Thanks much @bnoordhuis for the help.

Wow, thanks so much guys. I wouldn't have been able to figure that one out on my own.

@ronkorving ... any updates on this one?

I need to get back on this one. No updates at this time, hope to get back to it soon.

Because of the changes that have happened to fs, I'll restart this later.