Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: invoke buffered write callbacks on error #30596

Closed
wants to merge 6 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Nov 22, 2019

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Nov 22, 2019
@ronag
Copy link
Member Author

ronag commented Nov 22, 2019

Added comment and fixed linting

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

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

@ronag
Copy link
Member Author

ronag commented Nov 22, 2019

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

@ronag
Copy link
Member Author

ronag commented Nov 22, 2019

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

@lpinca
Copy link
Member

lpinca commented Nov 22, 2019

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
Copy link
Member Author

ronag commented Nov 22, 2019

@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
Copy link
Member

lpinca commented Nov 22, 2019

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
Copy link
Member Author

ronag commented Nov 22, 2019

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
Copy link
Member

lpinca commented Nov 22, 2019

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
Copy link
Member

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

@lpinca
Copy link
Member

lpinca commented Nov 23, 2019

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
Copy link
Member Author

ronag commented Nov 23, 2019

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

We should probably fix that as well...

@lpinca
Copy link
Member

lpinca commented Nov 23, 2019

We should probably fix that as well...

And many more, I guess.

@lpinca
Copy link
Member

lpinca commented Nov 23, 2019

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
Copy link
Member Author

ronag commented Nov 23, 2019

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
Copy link
Member Author

ronag commented Nov 23, 2019

@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
Copy link
Member

lpinca commented Nov 23, 2019

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
Copy link
Member Author

ronag commented Nov 23, 2019

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
Copy link
Member Author

ronag commented Nov 23, 2019

The state.length change broke this PR. Investigating.

@ronag
Copy link
Member Author

ronag commented Nov 23, 2019

@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.

@ronag ronag requested a review from mcollina November 23, 2019 14:14
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 24, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Dec 31, 2019
Buffered write callbacks were only invoked upon
error if `autoDestroy` was invoked.

PR-URL: #30596
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Dec 31, 2019

Landed in e66c4de

@Trott Trott closed this Dec 31, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Jan 3, 2020

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 added a commit to nxtedition/node that referenced this pull request Jan 3, 2020
Buffered write callbacks were only invoked upon
error if `autoDestroy` was invoked.

Backport-PR-URL: nodejs#31179
PR-URL: nodejs#30596
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@ronag
Copy link
Member Author

ronag commented Jan 3, 2020

Done, #31179

codebytere pushed a commit that referenced this pull request Feb 29, 2020
Refs: #30596

Buffered write callbacks were only invoked upon
error if `autoDestroy` was invoked.

Backport-PR-URL: #31179
PR-URL: #30596
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Feb 29, 2020
lpinca added a commit to websockets/ws that referenced this pull request Mar 3, 2020
If the socket is closed while data is being compressed, invoke the
current send callback and the buffered send callbacks with an error
before emitting the `'close'` event.

Refs: nodejs/node#30596
lpinca added a commit to websockets/ws that referenced this pull request Mar 7, 2020
If the socket is closed while data is being compressed, invoke the
current send callback and the buffered send callbacks with an error
before emitting the `'close'` event.

Refs: nodejs/node#30596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants