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: make stream.destroy with callback API public #32021

Closed
wants to merge 7 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Feb 29, 2020

This makes the stream.destroy(err, callback) API public.

Additionally it makes some changes:

  • The callback is always (see TODO) invoked with the same behavior as eos
  • The callback is no longer immediately invoked if destroy has already been invoked (but not actually completed).
  • The error is assumed to be handled and uncaughException is
    suppressed.
  • The callback timing is the same regardless whether destroy
    has already been called or not.
    (see TODO)
  • The callback is always invoked asynchronously.
  • The callback used to be invoked before emitting 'error' and/or
    'close'.
    (see TODO)

Also fixes a bug for fs streams where the callback to close(cb) could either be invoked synchronously or before the stream was actually closed. Likewise during errored destruction with implementations that use stream commons writeGeneric.

This is made possible by #31509

This affects a non-public API which is used internally at the following locations:

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 ronag added the stream Issues and PRs related to the stream subsystem. label Feb 29, 2020
@ronag ronag requested review from mcollina and lpinca February 29, 2020 09:15
@ronag ronag force-pushed the stream-cb-destroy branch 5 times, most recently from fa2d4b6 to 740e441 Compare February 29, 2020 09:21
@ronag ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 29, 2020
This makes the `stream.destroy(err, callback)` API public.

Additionally it makes some changes for easier use:

- The callback is always invoked with the same behavior as eos.
- The error is assumed to be handled and uncaughException is
  supressed.
- The callback timing is the same regardless whether destroy
  has already been called or not.
- The callback is always invoked asynchronously.
- The callback used be invoked before emitting 'error' and/or
  'close'.
@ronag ronag force-pushed the stream-cb-destroy branch from 740e441 to 4d51ce2 Compare February 29, 2020 09:26
@nodejs-github-bot

This comment has been minimized.

@ronag
Copy link
Member Author

ronag commented Feb 29, 2020

This is useful for bringing destroy of e.g. http requests inline with streams, since they essentially wrap a socket and need to call socket.destroy(err, callback) in any potential _destroy implementation.

lib/_stream_writable.js Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-cb-destroy branch from 00cd9b3 to 43472c4 Compare February 29, 2020 10:22
@ronag ronag force-pushed the stream-cb-destroy branch from cf8de82 to c21a15c Compare February 29, 2020 10:35
@ronag ronag force-pushed the stream-cb-destroy branch 2 times, most recently from 8a13506 to 97d1fd4 Compare February 29, 2020 10:57
@ronag ronag force-pushed the stream-cb-destroy branch from 97d1fd4 to 81de706 Compare February 29, 2020 10:58
@ronag
Copy link
Member Author

ronag commented Mar 1, 2020

Giving this some more thought I think it's better to revisit properly after #29179

@ronag ronag closed this Mar 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants