fix FileWriter.write to always return true or false (not undefined)

[demmer]

Since streams in 0.10 no longer have a _queue, FileWriter.write will return undefined instead of false when the underlying fs write stream returned false.

This simple change makes it always return either true or false.

[demmer]

@isaacs I'm sure you're quite busy, but I think this should be a no brainer PR to merge and I'd definitely appreciate it.

It actually manifested as a pretty irritating bug since our CI systems that use node-tar to unpack some packages would periodically OOM since the stream backpressure signal of undefined instead of false wasn't being detected properly.

[isaacs]

I think this is actually incorrect, though, per the Node stream semantics.

Only false is the backpressure-inducing magic value. If there's no _queue, then that means that there isn't anything waiting, and false is the incorrect return value.

I'm fine with having it return either true or false but Boolean-ifying the return value will result in a behavior change.

Can you provide a test where returning false on a non-existent queue is actually a benefit? I worry that the change to your node-tar example is actually just a "works by accident" result, perhaps obscuring a more fundamental bug by just throwing unnecessary backpressure into the mix somewhere else.

[demmer]

I've been trying to come up with a reliable standalone test case but it's not trivial since the triggering of the blocked behavior depends on the speed of the underlying filesystem writes. I will try to find time keep working on it, but fundamentally I feel like the current code is incorrect precisely because the backpressure-inducing false return is converted to undefined in node 0.10 since the stream returned false to indicate that it's buffering, but _queue doesn't exist.

How about the following:

var ret = me._stream.write(c)
if (ret === false && me._stream._queue) {
    return me._stream._queue.length <= 2;
} else {
    return ret;
}

This way if ret is anything but false, it isn't changed, and the policy of checking the queue is only applied if the queue exists.

[isaacs]

@demmer That seems pretty reasonable.

[demmer]

Great. Do you want me to update this PR or do you want to just make the change directly?

[isaacs]

You can just update this PR. Thanks.

[demmer]

Done.

[demmer]

Just noticed you merged the changes to master a few weeks ago... can you please push a v0.1.29 release and then close this PR? Thanks in advance.

[isaacs]

Whoops, forgot to push. Done now! Thanks for the nudge :)