GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http2: fix memory leak when nghttp2 hd threshold is reached by RafaelGSS · Pull Request #41502 · nodejs/node

Conversation

@RafaelGSS

@RafaelGSS RafaelGSS commented Jan 13, 2022

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss and badsyntax reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot

nodejs-github-bot commented Jan 13, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jan 13, 2022
@RafaelGSS
RafaelGSS force-pushed the fix/http2-memory-leak-nghttp2 branch from 40d5e54 to dc64638 Compare January 13, 2022 20:26
mcollina
mcollina approved these changes Jan 13, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 14, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 14, 2022
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Jan 14, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41869/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Jan 15, 2022
CI Reliability 2022-01-15 nodejs/reliability#174
Open
26 tasks
@nodejs-github-bot

nodejs-github-bot commented Jan 17, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41943/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Jan 18, 2022
CI Reliability 2022-01-18 nodejs/reliability#175
Open
16 tasks
Qard
Qard approved these changes Jan 18, 2022
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Jan 18, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42009/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Jan 19, 2022
CI Reliability 2022-01-19 nodejs/reliability#176
Open
13 tasks
@mcollina

mcollina commented Jan 19, 2022

Copy link
Copy Markdown
Member

@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS
RafaelGSS force-pushed the fix/http2-memory-leak-nghttp2 branch from dc64638 to e13dff6 Compare January 19, 2022 13:26
@nodejs-github-bot

nodejs-github-bot commented Jan 19, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42021/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS
RafaelGSS force-pushed the fix/http2-memory-leak-nghttp2 branch from e13dff6 to 13a144f Compare January 19, 2022 18:32
@github-actions github-actions Bot mentioned this pull request Jan 20, 2022
CI Reliability 2022-01-20 nodejs/reliability#177
Open
17 tasks
@nodejs-github-bot

nodejs-github-bot commented Jan 20, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42042/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS

RafaelGSS commented Jan 20, 2022

Copy link
Copy Markdown
Member Author

The ARM build produces a slighty different output. Instead of closing the connection on nghttp2 error, it propagates a frameError with NGHTTP2_HEADER_COMP error. I'll look at this at environment with more calm.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Jan 20, 2022

Copy link
Copy Markdown
Member

The ARM build produces a slighty different output. Instead of closing the connection on nghttp2 error, it propagates a frameError with NGHTTP2_HEADER_COMP error. I'll look at this at environment with more calm.

It might well be a difference that we would have to deal with. Maybe add an if depending on the environment?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Jan 21, 2022
CI Reliability 2022-01-21 nodejs/reliability#178
Open
19 tasks
@RafaelGSS

RafaelGSS commented Jan 21, 2022

Copy link
Copy Markdown
Member Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Jan 21, 2022

Copy link
Copy Markdown
Member

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Jan 22, 2022
CI Reliability 2022-01-22 nodejs/reliability#179
Open
17 tasks
@sxa

sxa commented Jan 22, 2022

Copy link
Copy Markdown
Member

nghttp2 behaves slightly different in x86 architectures (Windows and ARM).

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 ...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS

RafaelGSS commented Jan 22, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

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/

sxa reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jan 28, 2022
@nodejs-github-bot

nodejs-github-bot commented Jan 28, 2022

Copy link
Copy Markdown
Collaborator
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/.ncu
https://github.com/nodejs/node/actions/runs/1762462520
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina approved these changes Jan 28, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mcollina mcollina added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Jan 28, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 28, 2022
@nodejs-github-bot
nodejs-github-bot merged commit f98a785 into nodejs:master Jan 28, 2022
@nodejs-github-bot

nodejs-github-bot commented Jan 28, 2022

Copy link
Copy Markdown
Collaborator

Landed in f98a785

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jan 29, 2022
CI Reliability 2022-01-29 nodejs/reliability#186
Open
CI Reliability 2022-01-30 nodejs/reliability#187
Open
CI Reliability 2022-01-31 nodejs/reliability#188
Open
ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
@RafaelGSS @ruyadorno
http2: fix memory leak on nghttp2 hd threshold
2a18859
PR-URL: #41502
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Feb 8, 2022
v17.5.0 release proposal #41897
Merged
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
@RafaelGSS @danielleadams
http2: fix memory leak on nghttp2 hd threshold
63db692
PR-URL: #41502
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@danielleadams danielleadams mentioned this pull request Mar 3, 2022
v16.14.1 release proposal #42200
Merged
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
@RafaelGSS @danielleadams
http2: fix memory leak on nghttp2 hd threshold
42a45ef
PR-URL: #41502
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
@RafaelGSS @danielleadams
http2: fix memory leak on nghttp2 hd threshold
7bf2be5
PR-URL: #41502
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Apr 24, 2022
Problematic http2 header causes an assertion fail: Diagnosis and Fix #39951
Closed
@ywave620

ywave620 commented Nov 3, 2022

Copy link
Copy Markdown
Contributor

@RafaelGSS Hey, I wonder is there any plan to backport this commit to v14.x?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ywave620 ywave620 mentioned this pull request Nov 3, 2022
Request to backport "fix memory leak when nghttp2 hd threshold is reached" into v14.x #45295
Closed
@aduh95 aduh95 added the lts-watch-v14.x label Nov 3, 2022
@ywave620 ywave620 mentioned this pull request Nov 4, 2022
[v14.x backport] http2: fix memory leak when nghttp2 hd threshold is reached #45310
Closed
ywave620 added a commit to ywave620/node that referenced this pull request Nov 4, 2022
@ywave620
http2: fix memory leak on nghttp2 hd threshold
2291969
PR-URL: nodejs#41502
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
richardlau pushed a commit that referenced this pull request Nov 23, 2022
@ywave620 @richardlau
http2: fix memory leak when nghttp2 hd threshold is reached
c3a90c4
Refs: #45295
PR-URL: #41502
Backport-PR-URL: #45310
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
@richardlau richardlau added backported-to-v14.x and removed lts-watch-v14.x labels Nov 24, 2022
@richardlau richardlau mentioned this pull request Dec 7, 2022
v14.21.2 proposal #45775
Merged
mwalbeck pushed a commit to mwalbeck/docker-jellyfin-livestream that referenced this pull request Dec 14, 2022
renovate-bot @mwalbeck
Update Node.js to v14.21.2 (#210)
02fcc17
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [node](https://github.com/nodejs/node) | stage | patch | `14.21.1-bullseye-slim` -> `14.21.2-bullseye-slim` |

---

### Release Notes

<details>
<summary>nodejs/node</summary>

### [`v14.21.2`](https://github.com/nodejs/node/releases/tag/v14.21.2): 2022-12-13, Version 14.21.2 &#x27;Fermium&#x27; (LTS), @&#8203;richardlau

[Compare Source](nodejs/node@v14.21.1...v14.21.2)

##### Notable Changes

##### OpenSSL 1.1.1s

This update is a bugfix release and does not address any security
vulnerabilities.

##### Root certificates updated to NSS 3.85

Certificates added:

-   Autoridad de Certificacion Firmaprofesional CIF [`A626340`](nodejs/node@A62634068)
-   Certainly Root E1
-   Certainly Root R1
-   D-TRUST BR Root CA 1 2020
-   D-TRUST EV Root CA 1 2020
-   DigiCert TLS ECC P384 Root G5
-   DigiCert TLS RSA4096 Root G5
-   E-Tugra Global Root CA ECC v3
-   E-Tugra Global Root CA RSA v3
-   HiPKI Root CA - G1
-   ISRG Root X2
-   Security Communication ECC RootCA1
-   Security Communication RootCA3
-   Telia Root CA v2
-   vTrus ECC Root CA
-   vTrus Root CA

Certificates removed:

-   Cybertrust Global Root
-   DST Root CA X3
-   GlobalSign Root CA - R2
-   Hellenic Academic and Research Institutions RootCA 2011

##### Time zone update to 2022f

Time zone data has been updated to 2022f. This includes changes to Daylight
Savings Time (DST) for Fiji and Mexico. For more information, see
<https://mm.icann.org/pipermail/tz-announce/2022-October/000075.html>.

##### Commits

-   \[[`436a596e99`](nodejs/node@436a596e99)] - **crypto**: update root certificates (Luigi Pinca) [#&#8203;45490](nodejs/node#45490)
-   \[[`4b422d34af`](nodejs/node@4b422d34af)] - **deps**: V8: cherry-pick [`d2db7fa`](nodejs/node@d2db7fa7f786) (Richard Lau) [#&#8203;45785](nodejs/node#45785)
-   \[[`625f4bf3a9`](nodejs/node@625f4bf3a9)] - **deps**: update corepack to 0.15.1 (Node.js GitHub Bot) [#&#8203;45331](nodejs/node#45331)
-   \[[`48a9810de8`](nodejs/node@48a9810de8)] - **deps**: update corepack to 0.15.0 (Node.js GitHub Bot) [#&#8203;45235](nodejs/node#45235)
-   \[[`9f4e64b603`](nodejs/node@9f4e64b603)] - **deps**: update timezone to 2022f (Richard Lau) [#&#8203;45521](nodejs/node#45521)
-   \[[`f297b6bd21`](nodejs/node@f297b6bd21)] - **deps**: update archs files for OpenSSL-1.1.1s (RafaelGSS) [#&#8203;45272](nodejs/node#45272)
-   \[[`11629fef15`](nodejs/node@11629fef15)] - **deps**: upgrade openssl sources to 1.1.1s (RafaelGSS) [#&#8203;45272](nodejs/node#45272)
-   \[[`c3a90c4b44`](nodejs/node@c3a90c4b44)] - **http2**: fix memory leak when nghttp2 hd threshold is reached (rogertyang) [#&#8203;41502](nodejs/node#41502)
-   \[[`785dc3efee`](nodejs/node@785dc3efee)] - **module**: cjs-module-lexer WebAssembly fallback (Guy Bedford) [#&#8203;43612](nodejs/node#43612)
-   \[[`2dbeb889f6`](nodejs/node@2dbeb889f6)] - **node-api**: handle no support for external buffers (Michael Dawson) [#&#8203;45181](nodejs/node#45181)
-   \[[`5b2ea124f3`](nodejs/node@5b2ea124f3)] - **test**: add test to validate changelogs for releases (Richard Lau) [#&#8203;45325](nodejs/node#45325)
-   \[[`f13f889956`](nodejs/node@f13f889956)] - **test**: add a test to ensure the correctness of timezone upgrades (Darshan Sen) [#&#8203;45299](nodejs/node#45299)
-   \[[`5608e6fa72`](nodejs/node@5608e6fa72)] - **tools**: update certdata.txt (Luigi Pinca) [#&#8203;45490](nodejs/node#45490)
-   \[[`d6f1d7107b`](nodejs/node@d6f1d7107b)] - **tools**: have test-asan use ubuntu-20.04 (Filip Skokan) [#&#8203;45581](nodejs/node#45581)
-   \[[`370a00f737`](nodejs/node@370a00f737)] - **tools**: make license-builder.sh comply with shellcheck 0.8.0 (Rich Trott) [#&#8203;41258](nodejs/node#41258)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC41NS4wIiwidXBkYXRlZEluVmVyIjoiMzQuNTUuMCJ9-->

Reviewed-on: https://git.walbeck.it/mwalbeck/docker-jellyfin-livestream/pulls/210
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@Qard Qard Qard approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues and PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@RafaelGSS @nodejs-github-bot @mcollina @Trott @sxa @ywave620 @Qard @richardlau @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.