http2: fix memory leak when nghttp2 hd threshold is reached #41502
Conversation
|
Review requested: |
RafaelGSS
force-pushed
the
fix/http2-memory-leak-nghttp2
branch
from
40d5e54
to
dc64638
Compare
This comment has been hidden.
This comment has been hidden.
|
@RafaelGSS apparently a relevant test is failing on some platforms: https://ci.nodejs.org/job/node-test-commit-arm/40343/nodes=ubuntu2004-armv7l/testReport/junit/(root)/test/parallel_test_http2_options_max_headers_exceeds_nghttp2/. |
RafaelGSS
force-pushed
the
fix/http2-memory-leak-nghttp2
branch
from
dc64638
to
e13dff6
Compare
RafaelGSS
force-pushed
the
fix/http2-memory-leak-nghttp2
branch
from
e13dff6
to
13a144f
Compare
|
The ARM build produces a slighty different output. Instead of closing the connection on nghttp2 error, it propagates a |
It might well be a difference that we would have to deal with. Maybe add an if depending on the environment? |
|
@nodejs/build Looks like the nghttp2 behaves slightly different in x86 architectures (Windows and ARM). However, I do not have a machine with these architectures to debug it further. Could somebody help me? By the way, I have tried with Windows 10 x64 and Ubuntu 20.04 x64, and works fine. |
If all else fails, there is a process to get temporary access to a machine: https://github.com/nodejs/build/blob/09308290d8401e15fcd3f7f5c6610a6ea13df75a/GOVERNANCE.md#temporary-access You can search for open/closed issues with "temporary access" in the subject/title in the https://github.com/nodejs/build to see examples of requests. |
I'm slightly confused as to what you mean there. Is the failing difference happening on more than just ARM? It sounds like you have a pass on Windows/x86 at least. Looking at https://ci.nodejs.org/job/node-test-commit-arm/40343/ it looks like it's not failing on 64-bit ARM builds (aarch64/arm64) and is only occurring on 32-bit arm (armv7l). It's also worth noting that the machine it's running on in Matteo's log referenced above is a 32-bit docker container running on a 64-bit OS+kernel, although it's also failing on the "real" 32-bit OSs at https://ci.nodejs.org/job/node-test-binary-arm-12+/14026/ I'll try running a build+tests on one of my personal 32-bit ARM32 hosts and see how it goes ... |
I may be wrong, but looks like it is failing in Windows 2012 R2 x86 as well https://ci.nodejs.org/job/node-test-binary-windows-js-suites/13225/RUN_SUBSET=2,nodes=win2012r2-COMPILED_BY-vs2019-x86/testReport/junit/(root)/test/parallel_test_http2_options_max_headers_exceeds_nghttp2/ |
Commit Queue failed- Loading data for nodejs/node/pull/41502 ✔ Done loading data for nodejs/node/pull/41502 ----------------------------------- PR info ------------------------------------ Title http2: fix memory leak when nghttp2 hd threshold is reached (#41502) Author Rafael Silva (@RafaelGSS) Branch RafaelGSS:fix/http2-memory-leak-nghttp2 -> nodejs:master Labels c++, http2, needs-ci Commits 1 - http2: fix memory leak on nghttp2 hd threshold Committers 1 - RafaelGSS PR-URL: https://github.com/nodejs/node/pull/41502 Reviewed-By: Matteo Collina Reviewed-By: Stephen Belanger ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/41502 Reviewed-By: Matteo Collina Reviewed-By: Stephen Belanger -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - http2: fix memory leak on nghttp2 hd threshold ℹ This PR was created on Thu, 13 Jan 2022 19:41:46 GMT ✔ Approvals: 2 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41502#pullrequestreview-852212678 ✔ - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/41502#pullrequestreview-855912162 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-01-27T17:14:01Z: https://ci.nodejs.org/job/node-test-pull-request/42196/ - Querying data for job/node-test-pull-request/42196/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/1762462520 |
|
Landed in f98a785 |
PR-URL: nodejs#41502 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #41502 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #41502 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #41502 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #41502 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Address: #35233
nghttp2 contains a header limit as you can see here https://github.com/nghttp2/nghttp2/blob/20079b4c2f688385ba9ecf723f958d0448894879/lib/nghttp2_hd.h#L45 and when this limit is reached, the nghttp2 stops the pipelining with https://github.com/nghttp2/nghttp2/blob/master/lib/nghttp2_hd.c#L1658.
However, due to the fact of headers were processed before the error is thrown, the session still holds the memory reference for those headers and in this situation, it's never free causing the memory leak.