Skip to content
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

http2: fix memory leak caused by premature listener removing #55966

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ywave620
Copy link
Contributor

@ywave620 ywave620 commented Nov 23, 2024

Http2Session should always call ondone into JS to detach the handle. In some case, ondone is defered to be called by the StreamListener through WriteWrap, we should be careful of this before getting rid of the StreamListener.

The test case destroys the socket governed by http2. This is not a good practice, but nodejs internal in some case does this too (e.g. when a TCP RESET is received, nodejs internal destroys the socket without bothering it is consumed by a http2 session or not) and it is the simplest code that I can come up with to produce this issue.

I stongly believe that this fixs #42710 based on the issue's description

Http2Session should always call ondone into JS to detach the
handle. In some case, ondone is defered to be called by the
StreamListener through WriteWrap, we should be careful of this
before getting rid of the StreamListener.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Nov 23, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.00%. Comparing base (58a8eb4) to head (f9f55cf).
Report is 75 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55966      +/-   ##
==========================================
- Coverage   88.40%   88.00%   -0.41%     
==========================================
  Files         654      653       -1     
  Lines      187815   187852      +37     
  Branches    36136    35882     -254     
==========================================
- Hits       166045   165323     -722     
- Misses      15001    15713     +712     
- Partials     6769     6816      +47     
Files with missing lines Coverage Δ
src/node_http2.cc 85.06% <100.00%> (+0.34%) ⬆️

... and 116 files with indirect coverage changes

---- 🚨 Try these New Features:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants