Recent behavior change of buffered write callbacks

[lpinca]

As requested by @mcollina in #31179 (comment) I'm opening this issue to discuss the possibility of reverting a recent behavior change of writable streams.

writable.write() takes an optional callback which, quoting the documentation, is a

Callback for when this chunk of data is flushed

The documentation further clarifies the callback behavior as follows:

The writable.write() method writes some data to the stream, and calls the
supplied callback once the data has been fully handled. If an error occurs,
the callback may or may not be called with the error as its first argument.
To reliably detect write errors, add a listener for the 'error' event. If
callback is called with an error, it will be called before the 'error' event
is emitted.

This is quite accurate and inline with the actual implementation. There can only be a single write at time so if writable.write() is called while a write is in progress that write is buffered. If the stream errors or is destroyed the callbacks of buffered writes are not called.

In #29028 and then in #30596 this behavior was changed to call the callbacks of buffered writes with an error.

In my opinion this change was not need or better not justified by a specific issue. From what I've gathered (correct me if I am wrong) the main reason behind the change was that code like this

await for (const chunk of src) {
  new Promise(function(resolve, reject) {
    dst.write(chunk, err => err ? reject(err) : resolve());
  });
}

could result in a promise that is never settled, but the destination stream in the example does not buffer writes so the callback is always called.

I think this change creates more problems than it solves:

• Not all callbacks must be called. A lot of callbacks are only called in response of a specific event / operation. If that event or operation never occurred its associated callback should not be called. This depends on case-by-case basis. In this specific case if the callback is called it makes me think that buffered writes are attempted even after a stream errors or is destroyed and this is not true even after stream: invoke buffered write callbacks on error #30596.
• AFAIK no issues have been created? about buffered write callbacks not being called on error in all these years so I think the original behavior is not broken.
• There are some side effects like net: write(cb) not called if destroy():ed before 'connect' #30841 or [v13.x backport] stream: invoke buffered write callbacks on error #31179 (comment).
• It is hard to adapt user land code to this new behavior. For example net.Socket buffered write callbacks are now called with an error if a write error occurs. Doing the same on Node.js < 13 will be hard and hacky for userland libs. They should wait until Node.js 12 goes EOL to have the same behavior across all supported Node.js versions.

Refs: #29028
Refs: #30596
Refs: #31179

[lpinca]

cc: @nodejs/streams @ronag

[ronag]

For me this is less about a specific case and more about whether all async apis should always call the callback or not. Callbacks always being invoked is what I would expect and wouldn't even normally consult the documentation (even though that is probably a good idea).

Though I would prefer for callbacks to always be invoked, I'm not strongly against reverting. Not always calling the callback would make the code simpler and the documentation does state that the "the callback may or may not be called with the error as its first argument".

-0

Not sure about #29028. If revert, would probably change that so that the buffered writes are removed but the callbacks are not invoked.

[mcollina]

I LGTM the original change, however this issue highlight several interesting points.

As it is implemented in 12, I think that the callback of write is useless, because I cannot know if it will be called or not.

Somebody might write this code:

function shutdown(stream) {
  stream.write('SHUTDOWN', function() {
    db.teardown() // note that this will cause a memory leak if not called.
    stream.end()
  })
}

This code is not safe as a developer would need to add a lot of checks to make it safe. Note that our docs does not say it any place that the callback is advisory-only and it might not be called at all. It does not mention that this callback might not be called if the stream errors, it has ended, etc.

I don't use the write callback because of it is not reliable and I think it should not be used until we can make it reliable. In fact, my alternative point of view is to doc-deprecate it.

Not all callbacks must be called. A lot of callbacks are only called in response of a specific event / operation. If that event or operation never occurred its associated callback should not be called. This depends on case-by-case basis.

This is true. However the callbacks that might not be called at all are also the ones that can be called many times. Take the callback in http.createServer(opts, listener): we clearly state this is added as an event handler. I don't think the callback in .write() belongs to this case.

Not calling callbacks creates situations where we do not know what's the state inside things, making it impossible to rely on that API. Moreover, a tool like util.promisify and await imply that a callback is always called.

---

The most valuable point for me is backward compatibility:

It is hard to adapt user land code to this new behavior. For example net.Socket buffered write callbacks are now called with an error if a write error occurs. Doing the same on Node.js < 13 will be hard and hacky for userland libs. They should wait until Node.js 12 goes EOL to have the same behavior across all supported Node.js versions.

Have you got any specific code that relies on the unreliable behavior?

[lpinca]

This code is not safe as a developer would need to add a lot of checks to make it safe.

If that callback is not called it means the stream had buffered writes and it already errored from a previous write or was destroyed and the developer must handle that case anyway.

Note that our docs does not say it any place that the callback is advisory-only and it might not be called at all. It does not mention that this callback might not be called if the stream errors, it has ended, etc.

I disagree. This sentence

If an error occurs, the callback may or may not be called with the error as its first argument.

was added in b801e39 and from commit message it was explicitly made vague to keep that case (callback not called if the stream errors) possible.

I don't use the write callback because of it is not reliable and I think it should not be used until we can make it reliable.

It is reliable at least with the streams I've used (mainly net.Socket). Again, if the callback is not called it is because the stream already finished.

Have you got any specific code that relies on the unreliable behavior?

Yes, in ws, ws.send() takes a callback which is forwarded to socket.write(). When permessage-deflate is used, sends are buffered. There are two buffers. The socket buffer and the ws buffer. If the socket is closed while there is still data in the ws buffer the callbacks of buffered sends are not called. This is consistent with current behavior of buffered write callbacks.

To make it consistent with the new behavior we can call all buffered send callbacks but

1. The error will be different as we can't use Node.js errors.
2. There will be "holes". Buffered send callbacks will be called on all supported Node.js versions but buffered write callbacks will not be called on Node.js < 13.

[ronag]

my alternative point of view is to doc-deprecate it.

I wouldn't be opposed to this as well as doc-deprecate the callback for end() and instead refer to the events. Regardless whether we revert or not.

[ronag]

@mcollina @lpinca any chance of consensus here? This is blocking the backport of #31179

[lpinca]

I would revert but if that is not an option then go ahead, close this, and remove the blocked label from #31179. I don't want to keep it on hold indefinitely.

[mcollina]

Would you like to get this escalated to the tsc about doing a revert?

[lpinca]

Yes I think it would be nice to have TSC feedback here.

[mcollina]

Hey fellow @nodejs/tsc members, @lpinca is asking our opinion in this regard. In #29028 and #30596, he would have objected to them landing, and he is asking if we should do a revert. The changes landed on Node 13.

Note that both PRs were approved by @jasnell @addaleax and myself. I've added this to discuss in the next meeting.