-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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: use state.ending to see if stream called end() #18170
stream: use state.ending to see if stream called end() #18170
Conversation
I do not understand what is the current behavior and what you want to achieve with this change. Can you please edit the description of this PR to clarify that? Examples works better than a textual form. Can you please add/change the unit tests? |
@mcollina I added test cases. are they right? |
This is how i would expect things to work today and I would consider this a bug |
@mcollina There are some failed checks. What should I do? |
I think it's CI that is flaky. CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1208/ |
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.
I would feel okay with considering this semver-major
just to be careful (but also okay with not doing so).
We can land it semver-minor and taking it some more time before backporting. |
Can you please rebase on top of master? |
Calling `writable.end()` will probably synchronously call `writable.write()`, in such a situation the `state.ended` is false and `writable.write()` doesn't trigger `writeAfterEnd()`.
1401476
to
e6d3654
Compare
Rebased. |
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
CITGM seems similar, landing. |
Landed as c7ca07a |
Calling `writable.end()` will probably synchronously call `writable.write()`, in such a situation the `state.ended` is false and `writable.write()` doesn't trigger `writeAfterEnd()`. PR-URL: #18170 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Calling `writable.end()` will probably synchronously call `writable.write()`, in such a situation the `state.ended` is false and `writable.write()` doesn't trigger `writeAfterEnd()`. PR-URL: #18170 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: * cluster - add cwd to cluster.settings (cjihrig) [#18399](#18399) * deps - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * meta - add Leko to collaborators (Leko) [#18117](#18117) - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432) * n-api - expose n-api version in process.versions (Michael Dawson) [#18067](#18067) * perf_hooks - add performance.clear() (James M Snell) [#18046](#18046) * stream - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170) PR-URL: #18464
Notable changes: * cluster - add cwd to cluster.settings (cjihrig) [#18399](#18399) * deps - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * meta - add Leko to collaborators (Leko) [#18117](#18117) - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432) * n-api - expose n-api version in process.versions (Michael Dawson) [#18067](#18067) * perf_hooks - add performance.clear() (James M Snell) [#18046](#18046) * stream - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170) PR-URL: #18464
Notable changes: * cluster - add cwd to cluster.settings (cjihrig) [#18399](#18399) * deps - upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * meta - add Leko to collaborators (Leko) [#18117](#18117) - add vdeturckheim as collaborator (vdeturckheim) [#18432](#18432) * n-api - expose n-api version in process.versions (Michael Dawson) [#18067](#18067) * perf_hooks - add performance.clear() (James M Snell) [#18046](#18046) * stream - avoid writeAfterEnd() while ending (陈刚) [#18170](#18170) PR-URL: #18464
Calling `writable.end()` will probably synchronously call `writable.write()`, in such a situation the `state.ended` is false and `writable.write()` doesn't trigger `writeAfterEnd()`. PR-URL: nodejs#18170 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: * cluster - add cwd to cluster.settings (cjihrig) [nodejs#18399](nodejs#18399) * deps - upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * meta - add Leko to collaborators (Leko) [nodejs#18117](nodejs#18117) - add vdeturckheim as collaborator (vdeturckheim) [nodejs#18432](nodejs#18432) * n-api - expose n-api version in process.versions (Michael Dawson) [nodejs#18067](nodejs#18067) * perf_hooks - add performance.clear() (James M Snell) [nodejs#18046](nodejs#18046) * stream - avoid writeAfterEnd() while ending (陈刚) [nodejs#18170](nodejs#18170) PR-URL: nodejs#18464
Calling
writable.end()
will probably synchronously callwritable.write()
, in such a situation thestate.ended
is false and
writable.write()
doesn't triggerwriteAfterEnd()
.There is an example as below:
when 'prefinish' listener doesn't call
w.write()
,w.end()
will synchronously emit 'finish' event. The console's output is: finish.when 'prefinish' listener calls
w.write()
, the 'finish' event will be asynchronously emitted. Thew.write()
that called by the 'finish' listener will triggerwriteAfterEnd()
and throw a Error.I think using
state.ended
to decide whether to triggerwriteAfterEnd
is diffcult to understand and is somewhat unreasonable.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
stream