A detailed analysis of the cause of this bug is in my linked comment on
the corresponding issue. The primary fix is the new setImmediate call in
Http2Stream#_destroy, which prevents a re-entrant call into
Http2Session::SendPendingData when sending trailers after the
Http2Session has been shut down, allowing the trailer data to be flushed
properly before the socket is closed.
As a result of this change, writes can be initiated later in the
lifetime of the Http2Session. So, when a JSStreamSocket is used as the
underlying socket reference for an Http2Session, it needs to be able to
accept write calls after it is closed.
In addition, now that outgoing data can be flushed differently after a
session is closed, in two tests clients receive errors that they
previously did not receive. I believe the new errors are more correct,
so I changed the tests to match.
Fixes: https://github.com/nodejs/node/issues/42713
Refs: https://github.com/nodejs/node/issues/42713#issuecomment-1756140062
PR-URL: https://github.com/nodejs/node/pull/50202
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
A JS stream socket wraps a stream, exposing it as a socket for something
on top which needs a socket specifically (e.g. an HTTP server).
If the internal stream is closed in the same tick as the layer on top
attempts to close this stream, the race between doShutdown and doClose
results in an uncatchable exception. A similar race can happen with
doClose and doWrite.
It seems legitimate these can happen in parallel, so this resolves that
by explicitly detecting and handling that situation: if a close is in
progress, both doShutdown & doWrite allow doClose to run
finishShutdown/Write for them, cancelling the operation, without trying
to use this._handle (which will be null) in the meantime.
PR-URL: https://github.com/nodejs/node/pull/49400
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Do not delay the call to `stream.end()` too much.
PR-URL: https://github.com/nodejs/node/pull/42340
Refs: https://github.com/nodejs/node/pull/42340#issuecomment-1261163284
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Used an extra `checkReusedHandle()` helper function to
increase readability.
PR-URL: https://github.com/nodejs/node/pull/39649
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Signed-off-by: Darshan Sen <darshan.sen@postman.com>
PR-URL: https://github.com/nodejs/node/pull/38468
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Upcoming lint rule will require a blank line between consecutive
functions. Add it in the places where we don't have it already.
PR-URL: https://github.com/nodejs/node/pull/30696
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Leaving var in place of let for performance optimization
in short loops in hot paths. Added comments explaining why.
PR-URL: https://github.com/nodejs/node/pull/30415
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
In addition, use process.stderr instead of console.error when
there is no need to swallow the error.
PR-URL: https://github.com/nodejs/node/pull/27112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This improves our custom eslint rules to detect assertions to detect
assertions with only a single argument and fixes false negatives in
case unary expressions are used.
Some rules were extended to also lint our docs and tools and the lib
rule was simplified to prohibit most assertion calls.
PR-URL: https://github.com/nodejs/node/pull/26569
Refs: https://github.com/nodejs/node/pull/26565
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Its confusing to call a js class with a handle a "Wrap", usually it's
the C++ handle that is called a Wrap (tcp_wrap, tls_wrap, ...). Its
derived from Socket, and makes a JS stream look like a Socket, so call
it that. Also, remove use of lib/_stream_wrap.js so it can be deprecated
some time.
PR-URL: https://github.com/nodejs/node/pull/25153
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
