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

fix(instrumentation-http): resume responses when there is no response… #4332

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Nov 29, 2023

Fixes #4316

There is a memory leak in node 20 which is a manifestation of a problem that has been around for a while. If a user makes an http request and doesn't add a response listener to the request, the response body is typically dropped by the node runtime. The HTTP instrumentation adds a listener, which prevents the runtime from dropping the body. We should resume the response in order to put the stream back into flowing mode, which will cause the runtime to have the correct behavior after our callback returns.

This is fixed in the next branch by #4331 but if this merges first I will remove it from there.

… listener

Fixes a memory leak where unhandled response bodies pile up in node 20
@dyladan dyladan requested a review from a team November 29, 2023 21:16
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

Merging #4332 (3f96f1f) into main (f654636) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4332   +/-   ##
=======================================
  Coverage   92.31%   92.31%           
=======================================
  Files         330      330           
  Lines        9424     9426    +2     
  Branches     1998     1999    +1     
=======================================
+ Hits         8700     8702    +2     
  Misses        724      724           
Files Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts 93.35% <100.00%> (+0.04%) ⬆️

@Flarna
Copy link
Member

Flarna commented Nov 29, 2023

Well, it's actually not a leak in node.js and it's not new in node.js 20.
Caller is responsible to consume the data according to docs.
Instrumentation adds a response listener and this avoids the automatic data discard. So it's the instrumentation which causes the leak here in case user doesn't set a response listener and consumes the data. Data is kept in the incoming socket queue.

The reason why this was hidden in node 18 is that node 20 uses connection keep alive on default. As a result the socked data queue got more and more data.

@dyladan dyladan merged commit cc4ff2d into open-telemetry:main Nov 30, 2023
19 checks passed
@dyladan dyladan deleted the node-20-leak branch November 30, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible memory leak in @opentelemetry/instrumentation-http on node 20
4 participants