-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: add auto-destroy mode #22795
Conversation
@mafintosh I think it needs to call I think this is the first step to implement #20096, right? |
doc/api/stream.md
Outdated
@@ -1492,6 +1492,8 @@ changes: | |||
[`stream._destroy()`][writable-_destroy] method. | |||
* `final` {Function} Implementation for the | |||
[`stream._final()`][stream-_final] method. | |||
* `autoDestroy` {boolean} Whether this stream should automatically call | |||
.destroy() on itself after ending. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.destroy()
-> `.destroy()`
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide the default value explicitly here?
doc/api/stream.md
Outdated
@@ -1741,6 +1743,8 @@ constructor and implement the `readable._read()` method. | |||
method. | |||
* `destroy` {Function} Implementation for the | |||
[`stream._destroy()`][readable-_destroy] method. | |||
* `autoDestroy` {boolean} Whether this stream should automatically call | |||
.destroy() on itself after ending. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
lib/_stream_readable.js
Outdated
@@ -1075,6 +1078,10 @@ function endReadable(stream) { | |||
} | |||
} | |||
|
|||
function writableAutoDestroy(wState) { | |||
return !wState || (wState.autoDestroy && wState.finished); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this just be inlined?
It looks like the code in |
doc/api/stream.md
Outdated
@@ -1492,6 +1492,8 @@ changes: | |||
[`stream._destroy()`][writable-_destroy] method. | |||
* `final` {Function} Implementation for the | |||
[`stream._final()`][stream-_final] method. | |||
* `autoDestroy` {boolean} Whether this stream should automatically call | |||
.destroy() on itself after ending. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide the default value explicitly here?
t.resume(); | ||
t.on('end', common.mustCall()); | ||
t.on('finish', common.mustCall()); | ||
t.on('close', common.mustCall()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we maybe check the order of the events/destroy callback as well?
Gentle ping @mafintosh :) @targos I think it’s this way around in order to support shutting down Duplex streams? |
@addaleax i'll fix up the nits+comments |
Seems like a good addition to me, minus others' comments. |
semver-minor commits should contain metadata in the YAML |
lib/internal/http2/compat.js
Outdated
@@ -251,6 +251,7 @@ function onStreamTimeout(kind) { | |||
|
|||
class Http2ServerRequest extends Readable { | |||
constructor(stream, headers, options, rawHeaders) { | |||
if (!options) options = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I’m not missing anything – how does this change relate to the others?
bb2cb3c
to
387516c
Compare
387516c
to
ccc94a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixed all the nits @addaleax @targos @vsemozhetbyt @Fishrock123 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I would like to see a CITGM run.
doc/api/stream.md
Outdated
@@ -1493,6 +1493,11 @@ changes: | |||
pr-url: https://github.com/nodejs/node/pull/18438 | |||
description: > | |||
Add `emitClose` option to specify if `'close'` is emitted on destroy | |||
- version: TBD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the value we put here TBD? I never remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REPLACEME
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right! Thanks, will fix
One more CITGM with readable-stream enabled master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1606/ |
The CITGM runs seem to report similar errors on master + this PR, landing ... |
PR-URL: nodejs#22795 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Landed in f24b070 |
PR-URL: #22795 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
PR-URL: #22795 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesThis adds a new options to the stream constructor called
autoDestroy
that is default false.When autoDestroy is enabled .destroy() will automatically be called when the stream has ended and/or finished depending on whether it's a readable/writable/duplex.
This simplifies error handling / resource management for users as you can then always do your teardown logic in the destroy method.