Closing fs streams early could call close during or before I/O

[briangreenery]

Suppose you fs.createWriteStream, pipe something into it, and then need to close the stream early because of an error somewhere else.

var writeStream = fs.createWriteStream(someFile);
source.pipe(writeStream);

// ...time passes...

source.unpipe(writeStream);
writeStream.close();

Calling close on the write stream in this case could cause close to be called on the underlying file descriptor while a write operation is still pending. Or, if more than one worker thread is being used, it's possible for the close to happen before the write begins.

Specifically, WriteStream.close does not check whether a fs.write operation is pending before calling fs.close:

https://github.com/nodejs/io.js/blob/41951d45b6df789d7e9cf134f0029b0e791706c4/lib/fs.js#L1770

It seems like this makes it impossible to safely close a write stream early. I've never seen bad behavior from this in practice though, so maybe I'm misunderstanding something.

Are we instead supposed to call Writable.end and should never use WriteStream.close?

[bnoordhuis]

I think you're right and it's a rather insidious data corruption bug. I put together a test case that demonstrates the race condition:

'use strict';

const assert = require('assert');
const fs = require('fs');
const buf = Buffer('.');

pummel();

function pummel() {
  const a = fs.createWriteStream('a.txt');
  // Assumes UV_THREADPOOL_SIZE=4
  for (var i = 0; i < 3; i += 1) a._write(buf, 'buffer', function() {});

  a.on('open', function() {
    const afd = a.fd;
    a.close();

    for (;;) {
      const bfd = fs.openSync('b.txt', 'a');
      if (afd === bfd) break;
      fs.closeSync(bfd);
    }

    setTimeout(function() {
      fs.closeSync(afd);
      assert.equal(fs.statSync('b.txt').size, 0);
      pummel();
    }, 50);
  });
}

It's easier to hit the assert when you attach strace to the process, otherwise file operations complete just too darn fast.

The issue is that:

1. The libuv threadpool is (by design) unordered. A write operation and a close operation can race with each other and can even run in reverse order if one thread is faster than the other.
2. fs.WriteStream takes no steps to impose total ordering on open/write/close operations (which is its responsibility.)

I could have sworn fs.WriteStream was backed by a work queue. Some digging turns up that it used to until commit 44b308b from 2012, which is when @isaacs landed his streams2 work and dropped the queue for reasons that are not clear to me.

As a workaround, you can set UV_THREADPOOL_SIZE=1 in the environment. That restricts libuv's threadpool to a single thread, imposing a total order on the operations. That won't protect against synchronous file operations racing with asynchronous ones, though.

[chrisdickinson]

I could have sworn fs.WriteStream was backed by a work queue. Some digging turns up that it used to until commit 44b308b from 2012, which is when @isaacs landed his streams2 work and dropped the queue for reasons that are not clear to me.

The queue is now part of the inner workings of writable streams – only one write (or "end") is outstanding at a time in streams2+. The streams problem is not the queue, it's that another actor is killing the resource out from under the stream (and that the other actor has no way of knowing whether the stream is in the middle of a write – otherwise you could writableStream.pause(); writableStream.close().)

[saquibkhan]

Hi @bnoordhuis ,

Why you using _write?

for (var i = 0; i < 3; i += 1) a._write(buf, 'buffer', function() {});

I tried the above test after replacing _write with write and after a lag i got below error:

events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: EBADF: bad file descriptor, write
    at Error (native)

[bnoordhuis]

@saquibkhan ._write() makes it easier to trigger the race condition. You can still hit it with .write() but going through the streams machinery makes the test much less reliable (as you've experienced.)

[isaacs]

@bnoordhuis Well, going through the streams machinery introduces a work queue which avoids the race condition. You can write() a million times and then end(), and it'll only close the fd when it's done writing.

[bnoordhuis]

Was my last comment unclear? No snark, honest question. The race condition still exists with .write(), it's just harder to trigger.

[isaacs]

Oh I see. I was missing the context of your test. It looks like close should be put in the same queue as end and write, yes. That method was left out of the streams API proper because it differs so much between streams. If you want an eventual close, then the method to call is end(). close is destructive.

[bnoordhuis]

Is the solution to basically WriteStream.prototype.close = WriteStream.prototype.end? I've tried that and it seems to work mostly okay. There are some test failures but it fixes the race condition.

diff --git a/lib/fs.js b/lib/fs.js
index 58704e5..9631094 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -1869,7 +1869,11 @@ WriteStream.prototype._write = function(data, encoding, cb) {


 WriteStream.prototype.destroy = ReadStream.prototype.destroy;
-WriteStream.prototype.close = ReadStream.prototype.close;
+
+WriteStream.prototype.close = function(cb) {
+  this.once('end', ReadStream.prototype.close.bind(this, cb));
+  return this.end();
+};

 // There is no shutdown() for files.
 WriteStream.prototype.destroySoon = WriteStream.prototype.end;

By the way, I think fs.ReadStream has the same issue but there's a complication: it doesn't have an .end() method, it gets overridden with options.end.

[saquibkhan]

@bnoordhuis @isaacs
Plz note: If in the above testcase i replace close with end still i am able to see the issue.

[isaacs]

I think the solution here might be for close and destroy to be blessed in the Streams base class semantics. close() should wait for any pending write/read to finish, and then do whatever implementation-specific behavior close is for that stream. (In this case, fs.close(this.fd))

@bnoordhuis In your patch, it should be this.once('finish', ...) since Writables don't have an 'end' event, they have an end method.

What's essentially happening is that there's one or more writes in progress, and the close method doesn't wait in line--it just closes the fd right now. That is clearly more destructive than expected. Even destroy() should probably wait until any already-started write()s are finished (but perhaps drop any that haven't been started yet?).

In most cases, you shouldn't even be calling stream.close(); it should be implicit when either end() is called (for writable streams) or when the 'end' event is encountered (for readable streams).

Can we zoom out a bit on this issue? Why are you calling close() explicitly on a write stream? Why not end()?

[briangreenery]

The reason that I used close() was ignorance. I didn't know the correct way to abort a write stream, and using destroy() was probably the first thing on stack overflow that I saw.

The reason that I want to abort a write stream early is to handle the case that some other part of my program has an error, and I want to stop and clean everything up.

As a user, I'm okay if the answer is to just use end().

It does seem like this is a common error. For example, the pump package makes this mistake despite having a knowledgeable author:

https://github.com/mafintosh/pump/blob/dc0a3c33ac51a37f2ac3551d1a292620fdc5ad91/index.js#L39

Also, during a conversation on a previous issue about destroy, there is this conversation:

But what the accepted method to close a stream is? As far as I know, close only exists for fs streams.

and indutny responded:

yes, it is .close().

[isaacs]

Yeah, it's a common enough error that we should handle an in-process _read or _write in close().

The only question is whether it should drop any pending (but not yet started) writes. And, this gets into an area where we probably don't want fs.js touching streams internals, so the streams API needs to make close a first class thing.

[iliakan]

As of now, most streams actually do implement close.
But the semantic is different.

For instance:

1. On http response res.on('close') to detect an "unexpected" connection close. Either close or end, not 2 events happen.
2. On fs the close event always happens at the end (file closed)

The idea to add close to the core is great, because it's a de-facto standard. But, guess, need to deal with the semantic incompatibilities somehow.

[jasnell]

@bnoordhuis @isaacs ... any further thoughts on this one? Suggestions on how to proceed?

[bnoordhuis]

Someone from @nodejs/streams should probably take a look.

[mcollina]

I agree with @isaacs:

I think the solution here might be for close and destroy to be blessed in the Streams base class semantics. close() should wait for any pending write/read to finish, and then do whatever implementation-specific behavior close is for that stream. (In this case, fs.close(this.fd))

In the next meeting of @nodejs/streams we will start discussing just that. @calvinmetcalf @mafintosh we should probably also cover close(). See nodejs/readable-stream#183.

[iliakan]

Also a question if taking "close" in: is it possible for other stream-interaction events to happen after close?

As of now, error can be after finish. Is it normal (guess, yes)?
Can it happen after close (guess no)?

[sonewman]

👍 on adding close and destroy since this would hopefully help amend any ambiguities in third party implementations of this behaviour.

[Trott]

In the next meeting of @nodejs/streams we will start discussing just that. @calvinmetcalf @mafintosh we should probably also cover close(). See nodejs/readable-stream#183.

@mcollina Did anything notable happen with this?

[mcollina]

We implemented destroy(err), and it is standardized across all of core.

[Trott]

We implemented destroy(err), and it is standardized across all of core.

So this issue can be closed? Or not quite?

[mcollina]

I would need to look into this with a bit more detail to be certain.

[mcollina]

IMHO we should deprecate .close(), it does not add anything to .end(). And we should make close() be equal to end()  in the case of fs.WriteStream, as proposed in #2006 (comment).

destroy() is standardized now, and we would need to move the current logic in close() to _destroy() as it is the other way around atm: https://github.com/nodejs/node/blob/master/lib/fs.js#L2009-L2013.

I can take care of the change, but I do not know how write a unit test to reproduce this issue just using the stream API, as #2006 (comment) uses all the internals. @bnoordhuis can you help?

This change will likely be semver-major.

[jasnell]

ping @mcollina and @bnoordhuis

[mcollina]

A tentative fix in #15407.