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: 'prefinish' timing and 'finish' after 'close' #31401

Closed
ronag opened this issue Jan 17, 2020 · 3 comments
Closed

stream: 'prefinish' timing and 'finish' after 'close' #31401

ronag opened this issue Jan 17, 2020 · 3 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Jan 17, 2020

  • 'finish' can be emitted after 'close' if destroy() is called inside a'prefinish' handler.
  • 'prefinish' has different timing (sync vs async) depending on whether Writable implements _final which can cause problems such as above to occur or not occur. (stream: simplify Transform stream implementation #32763)

I assume 'prefinish' is mostly used internally so it's probably not a huge issue.

@ronag ronag changed the title stream: Writable issues stream: 'prefinish' timing Jan 17, 2020
@ronag
Copy link
Member Author

ronag commented Jan 17, 2020

One of these will fail while the other won't:

{
  const write = new Writable({
    write(chunk, enc, cb) { cb(); },
    final(cb) { cb(); }
  });

  write.on('close', common.mustCall(() => {
    write.on('finish', common.mustNotCall());
  }));
  write.on('prefinish', common.mustCall(() => {
    write.destroy();
  }));
  write.end();
}

{
  const write = new Writable({
    write(chunk, enc, cb) { cb(); }
  });

  write.on('close', common.mustCall(() => {
    write.on('finish', common.mustNotCall()); // fails
  }));
  write.on('prefinish', common.mustCall(() => {
    write.destroy();
  }));
  write.end();
}

@ronag ronag changed the title stream: 'prefinish' timing stream: 'prefinish' timing and 'finish' after 'close' Jan 17, 2020
@ronag ronag closed this as completed Apr 11, 2020
@ronag ronag reopened this Apr 11, 2020
ronag added a commit to nxtedition/node that referenced this issue Apr 15, 2020
This PR fixes a few different things:

The timing of 'prefinish' depends on whether or not
_final is defined. In on case the event is emitted
synchronously with end() and otherwise asynchronously.

_final is currently unecessarily called asynchronously
which forces implementors to use 'prefinish' as a hack
to emulate synchronous behaviour. Furthermore, this hack
is subtly broken due to the above issue.

The stream should not finish if errored or destroyed
synchronously during the prefinish stage.

Refs: nodejs#31401
Refs: nodejs#32763 (comment)
ronag added a commit that referenced this issue Apr 22, 2020
This PR fixes a few different things:

The timing of 'prefinish' depends on whether or not
_final is defined. In on case the event is emitted
synchronously with end() and otherwise asynchronously.

_final is currently unecessarily called asynchronously
which forces implementors to use 'prefinish' as a hack
to emulate synchronous behaviour. Furthermore, this hack
is subtly broken due to the above issue.

Refs: #31401
Refs: #32763 (comment)

PR-URL: #32780
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos
Copy link
Member

targos commented Dec 26, 2020

what's the status on this?

@targos targos added the stream Issues and PRs related to the stream subsystem. label Dec 26, 2020
@ronag
Copy link
Member Author

ronag commented Dec 26, 2020

I think this is fixed.

@ronag ronag closed this as completed Dec 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants