stream: invoke buffered write callbacks on error

[ronag]

Buffered write callbacks were only invoked upon error if autoDestroy is enabled.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ronag]

Added comment and fixed linting

[mcollina]

LGTM

[mcollina]

@ronag Is this based on a semver-major commit that landed in 13? Are the above labels correct?

[ronag]

@mcollina: Yes it is based on #29028 which landed in 13. Labels look correct.

[ronag]

I'll try to advise on those labels in future PR's.

[lpinca]

Are you sure this is a good idea? Technically those writes never happened so why their callbacks are invoked? I noticed that this behavior was introduced in #29028. I missed it.

[ronag]

@lpinca: Not invoking callbacks in a callback based API is in my opinon very strange and could lead to deadlocks.

e.g. this could deadlock...

async function writePacket () {
  dst.write('header')
  for (const data of src) {
    await new Promise((resolve, reject) => 
      dst.write(data, err => err ? reject(err) : resolve()
    )
  }
}

Not saying that's the best way to do that, but it should still be valid.

[lpinca]

Invoking all of them synchronously with an error that say "Cannot call write after a stream was destroyed" is not much better imho because:

1. The stream was not destroyed when write was called
2. It makes me think that buffered writes are attempted even after a stream is destroyed

[ronag]

Well in this case the error basically means and says that it wasn't attempted... no?

In a callback based API if the operation is not attempted you still call the callback with something like aborted.

If we promisified the API. Would the returned promise possibly never resolve nor reject in your opinion?

[lpinca]

In a callback based API if the operation is not attempted you still call the callback with something like aborted.

Hmm I don't buy it in this case because the actual failure is already signaled on the stream and all writes are bound to it, so it's obvious that if it is no longer writable no more writes will be done. No writes attempted no callbacks.

I preferred previous behavior but I guess it's too late :)

[mcollina]

If you call a function with a calback, the expectation throughout our codebase is that the callback is called.

[lpinca]

If you call a function with a calback, the expectation throughout our codebase is that the callback is called.

Is it? Here is one example where the callback is not called regardless of this change.

const { createDeflateRaw } = require('zlib');
const { randomBytes } = require('crypto');

const deflate = createDeflateRaw();

deflate.resume();
deflate.write(randomBytes(1024));
deflate.flush(function() {
  throw new Error('Unexpected callback invocation');
});

process.nextTick(function() {
  deflate.close();
});

[ronag]

Is it? Here is one example where the callback is not called regardless of this change.

We should probably fix that as well...

[lpinca]

We should probably fix that as well...

And many more, I guess.

[lpinca]

Sorry to pester but there is one more thing which makes me very uncomfortable with this behavior.

const { Writable } = require('stream');

function callback1(err) {
  console.log('callback 1', err);
}

function callback2(err) {
  console.log('callback 2', err);
}

const chunk = Buffer.alloc(1024);

const writable = new Writable({
  write(chunk, encoding, callback) {
    setTimeout(callback, 1000);
  }
});

writable.on('close', function() {
  console.log('close');
});

writable.write(chunk, callback1);
writable.write(chunk, callback2);

setTimeout(function() {
  writable.destroy();
}, 500);

Callbacks of queued writes can be called before a callback (or possibly callbacks in case of _writev()) of a write in progress. That is callback order is no longer respected.

[ronag]

Sorry to pester but there is one more thing which makes me very uncomfortable with this behavior.

No worries.

Callbacks of queued writes can be called before a callback (or possibly callbacks in case of _writev()) of write in progress. That is callback order is no longer respected.

I'm not sure I follow? The write callback is called first and then any queued callbacks. Order is maintained? Do you think you can write a failing test I can look into?

[ronag]

@lpinca: I think the behaviour you are referring to is what's done in #29028, not something this PR changes? If so I would propose we take that as a separate issue. I'm not sure whether destroy acts like you describe but I if so I think it can be changed so that be buffered callbacks are invoked after all active write calls are completed.

[lpinca]

Yes correct, too bad I missed that PR. Anyway I think it is relevant here too because this patch extends that behavior to streams that do not use autoDestroy.

[ronag]

extends that behavior to streams that do not use autoDestroy.

Yes, but it happens in onwriteError when there is no active write.

@lpinca: If you create an issue and assign it to me I will try to resolve your concern in a follow up PR to this.

[ronag]

The state.length change broke this PR. Investigating.

[ronag]

@lpinca Doesn't seem to much of a trouble to fix. I've included a fix commit for your concern in this PR. I'll move it to a separate PR if that's preferable.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

Landed in e66c4de

[BridgeAR]

This does not land cleanly on v13.x. Please open a manual backport in case this should be backported or leave a comment to update the labels in case it should not be backported.

[ronag]

Done, #31179