Skip to content

Conversation

@ronag
Copy link
Member

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

@ronagronag added the stream Issues and PRs related to the stream subsystem. label Feb 29, 2020
@ronagronag requested review from lpinca and mcollinaFebruary 29, 2020 09:15
@ronagronagforce-pushed the stream-cb-destroy branch 5 times, most recently from fa2d4b6 to 740e441CompareFebruary 29, 2020 09:21
@ronagronag 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'.
@nodejs-github-bot

This comment has been minimized.

@ronag
Copy link
MemberAuthor

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.

@ronagronagforce-pushed the stream-cb-destroy branch 2 times, most recently from 8a13506 to 97d1fd4CompareFebruary 29, 2020 10:57
@ronag
Copy link
MemberAuthor

ronag commented Mar 1, 2020

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

@ronagronag closed this Mar 1, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-majorPRs that contain breaking changes and should be released in the next major version.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@ronag@nodejs-github-bot