[v8.x backport] async_hooks: add missing async_hooks destroys in AsyncReset #23410
Conversation
nodejs-github-bot
added
lib / src
| @@ -464,6 +464,9 @@ class Parser : public AsyncWrap { | |||
| static void Reinitialize(const FunctionCallbackInfo<Value>& args) { | |||
| Environment* env = Environment::GetCurrent(args); | |||
|
|
|||
| CHECK(args[0]->IsInt32()); | |||
There was a problem hiding this comment.
This check is actually new for the v8.x branch but since I was adding a new param here anyway (args[1]/isReused) I figured it wouldn't hurt to copy the existing check from master here.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/17737/ As a heads up, this won’t be released in v8.x until #23272 has been in a Current release for 2 weeks, unless this is deemed a critical bugfix (which I assume it won’t be). |
Just for my understanding - "Current release" in this case means a 10.x release, right? So the timeline would be
Is that how this works? As to the criticality: I don't know the criteria for critical bugfixes. The issues causes memory leaks and thus causes processes to die with out-out-of-memory. At least customers of some APM vendors (like my company) are affected. |
BethGriggs
added
the
baking-for-lts
This adds missing async_hooks destroy calls for sockets (in _http_agent.js) and HTTP parsers. We need to emit a destroy in AsyncWrap#AsyncReset before assigning a new async_id when the instance has already been in use and is being recycled, because in that case, we have already emitted an init for the "old" async_id. This also removes a duplicated init call for HTTP parser: Each time a new parser was created, AsyncReset was being called via the C++ Parser class constructor (super constructor AsyncWrap) and also via Parser::Reinitialize. PR-URL: nodejs#23272 Fixes: nodejs#19859 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
basti1302
force-pushed
the
backport-8-23272
branch
from
d9af7ec
to
5f22659
Compare
|
Rebased this on v8.x-staging to resolve conflict in src/async_wrap.cc. |
This adds missing async_hooks destroy calls for sockets (in _http_agent.js) and HTTP parsers. We need to emit a destroy in AsyncWrap#AsyncReset before assigning a new async_id when the instance has already been in use and is being recycled, because in that case, we have already emitted an init for the "old" async_id. This also removes a duplicated init call for HTTP parser: Each time a new parser was created, AsyncReset was being called via the C++ Parser class constructor (super constructor AsyncWrap) and also via Parser::Reinitialize. Backport-PR-URL: #23410 PR-URL: #23272 Fixes: #19859 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
|
landed in 26d145a |
MylesBorins
removed
the
baking-for-lts
|
@BethGriggs Any chance this could be included in v8.13.0, that seems to be scheduled for later this month? I'm not familiar with the LTS release process, but #23974 looks like the scope of 8.13.0 is already being defined and this is probably not included. |
This adds missing async_hooks destroy calls for sockets (in
_http_agent.js) and HTTP parsers. We need to emit a destroy in
AsyncWrap#AsyncReset before assigning a new async_id when the instance
has already been in use and is being recycled, because in that case, we
have already emitted an init for the "old" async_id.
This also removes a duplicated init call for HTTP parser: Each time a
new parser was created, AsyncReset was being called via the C++ Parser
class constructor (super constructor AsyncWrap) and also via
Parser::Reinitialize.
PR-URL: #23272
Fixes: #19859
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Checklist
make -j4 test(UNIX) passes