Skip to content

Conversation

@Lei-k
Copy link
Contributor

@Lei-k Lei-k commented Aug 29, 2025

Which problem is this PR solving?

What

When using FetchInstrumentation, if a user cancels the Response.body reader, the underlying HTTP connection is not released.

Why

The instrumentation clones the Response and continuously reads from the clone to track span completion. Even if the user cancels the original response, the clone reader continues reading, preventing the browser from freeing the HTTP connection. This breaks integrations such as flv.js where player.destroy() is expected to close network connections.

Fixes #5883

Short description of the changes

Introduce a withCancelPropagation helper that wraps the original response body. When the user cancels, the cancel signal is propagated to the clone's reader, ensuring both readers stop and the HTTP connection is released properly.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  1. Start a flv.js player with @opentelemetry/instrumentation-fetch enabled.
  2. Call player.destroy().
  3. Verify in Chrome DevTools that the HTTP request is marked as finished and the connection is released.

Checklist:

  • Followed the style guidelines of this project

@Lei-k Lei-k requested a review from a team as a code owner August 29, 2025 17:30
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 29, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@Lei-k
Copy link
Contributor Author

Lei-k commented Aug 29, 2025

For reference, here’s a patch that applies withCancelPropagation. Applying this resolves the issue locally for me.

@opentelemetry+instrumentation-fetch+0.203.0.patch

@Lei-k
Copy link
Contributor Author

Lei-k commented Aug 29, 2025

All tests passed locally:

Lerna (powered by Nx) Successfully ran target test for 37 projects (2m)

@Lei-k Lei-k changed the title fix(fetch-instrumentation): release HTTP connection when response body is cancelled fix(instrumentation-fetch): release HTTP connection when response body is cancelled Aug 29, 2025
@codecov
Copy link

codecov bot commented Sep 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.07%. Comparing base (4dca17f) to head (832b061).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5894   +/-   ##
=======================================
  Coverage   95.07%   95.07%           
=======================================
  Files         308      308           
  Lines        8037     8037           
  Branches     1626     1626           
=======================================
  Hits         7641     7641           
  Misses        396      396           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! Could you please add a test for us to see that the behavior is correct now and also for us to prevent any regressions in the future :)

@Lei-k
Copy link
Contributor Author

Lei-k commented Sep 4, 2025

Hi @pichlermarc,

I've added a test case that simulates fetch in a long connection scenario where reader.cancel() should properly release the connection. The test case is outlined below:

  1. Use a continuously pushing ReadableStream to simulate a long connection
  2. After calling fetch, read the first chunk and confirm the stream hasn't ended
  3. After calling reader.cancel(), check if traceFetch() completes within one second. If it doesn't complete, it indicates that cloneReader.cancel() wasn't triggered to clean up resources

I'm testing this way because cloneRes exists internally within FetchInstrumentation, and there's no direct way to detect whether cloneRes has ended, so this approach is used to handle the situation.

@Lei-k Lei-k requested a review from pichlermarc September 10, 2025 17:17
@Lei-k Lei-k requested a review from legendecas September 18, 2025 12:08
@Lei-k
Copy link
Contributor Author

Lei-k commented Sep 18, 2025

Hi @legendecas , I’ve addressed the feedback. Could you please let me know if this PR meets the criteria for merging, or if there’s anything else you’d like me to change?

@pichlermarc pichlermarc added this pull request to the merge queue Sep 22, 2025
Merged via the queue into open-telemetry:main with commit a7a3649 Sep 22, 2025
25 checks passed
@otelbot
Copy link
Contributor

otelbot bot commented Sep 22, 2025

Thank you for your contribution @Lei-k! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

@Lei-k Lei-k deleted the fix/instrumentation-fetch branch September 22, 2025 12:14
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.

HTTP Connections Not Released After OpenTelemetry Integration

3 participants