fix(fetch): Fix memory leak when handling endless streaming #13809

soapproject · 2024-09-26T10:58:55Z

This is a quick fix to address a memory overflow issue caused by the recursive approach when handling endless streams (e.g., CCTV).

However, this is not a perfect solution, as this approach still does not trigger the onFinishedResolving callback for streams that never terminate.

Fixes GH-13806

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

If you've added code that should be tested, please add tests.
[] Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

The lints didn't pass but they are unrelated to the changes in this commit?

This is a quick fix to address a memory overflow issue caused by the recursive approach when handling endless streams (e.g., CCTV). However, this is not a perfect solution, as this approach still does not trigger the onFinishedResolving callback for streams that never terminate. Fixes getsentryGH-13806

adjust format

lforst · 2024-09-30T10:14:09Z

Hi, thanks for opening the PR! Would you mind outlining the thought process behind the releaseLock() call? I think I get it but want to be sure we all know what's going on because this is a super hot path in the SDK.

soapproject · 2024-09-30T11:32:42Z

Hi, actually the improvement was referenced from GTP.
But I think it is correct to use releaseLock, after read the ReadableStream doc:
https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream/getReader

getReader locks the stream and returns either a ReadableStreamDefaultReader or a ReadableStreamBYOBReader.
https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader/releaseLock
https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamBYOBReader/releaseLock

So we need to release the lock and de-activate the reader to ensure that res.body is not locked.

Maybe we can experiment with calling resolveResponse multiple times without releasing the lock to see what happens?

soapproject · 2024-09-30T11:51:58Z

btw, we've encountered another issue when working with CCTV streaming. Even after leaving the page and calling videojs.dispose(), the stream continues to fetch data in the background.

I think this might require opening a new issue, but could this behavior be related to the code in fetch.ts?

lforst · 2024-10-02T14:01:21Z

I have updated this PR to:

not update the span duration in every case we cancel the operation
add a maximum timeout of 90s (this is arbitrary)
remove some unnecessary async/awaits
cancel the body (and implicitly the reader) to stop putting stuff into memory as it streams in

chargome · 2024-10-02T15:40:24Z

packages/utils/src/instrument/fetch.ts

@@ -158,7 +175,8 @@ async function streamHandler(response: Response): Promise<void> {
    return;
  }

-  await resolveResponse(clonedResponseForResolving, () => {
+  // eslint-disable-next-line @typescript-eslint/no-floating-promises
+  resolveResponse(clonedResponseForResolving, () => {


Why do we want leave this floating?

It is fine to have floating promises if you are certain that they cannot reject - i think this is the case here!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@sentry/node](https://github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.32.0` -> `8.33.1`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.32.0/8.33.1) | | [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.32.0` -> `8.33.1`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.32.0/8.33.1) | --- ### Release Notes <details> <summary>getsentry/sentry-javascript (@sentry/node)</summary> ### [`v8.33.1`](https://github.com/getsentry/sentry-javascript/releases/tag/8.33.1) [Compare Source](getsentry/sentry-javascript@8.33.0...8.33.1) - fix(core): Update trpc middleware types ([#13859](getsentry/sentry-javascript#13859)) - fix(fetch): Fix memory leak when handling endless streaming ([#13809](getsentry/sentry-javascript#13809)) Work in this release was contributed by [@soapproject](https://github.com/soapproject). Thank you for your contribution! ##### Bundle size 📦 | Path | Size | | ---------------------------------------------------------------- | ----------------- | | [@sentry/browser](https://github.com/sentry/browser) | 22.64 KB | | [@sentry/browser](https://github.com/sentry/browser) - with treeshaking flags | 21.42 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing) | 34.87 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay) | 71.38 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay) - with treeshaking flags | 61.81 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay with Canvas) | 75.73 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback) | 88.5 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Tracing, Replay, Feedback, metrics) | 90.38 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. metrics) | 26.91 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. Feedback) | 39.78 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. sendFeedback) | 27.3 KB | | [@sentry/browser](https://github.com/sentry/browser) (incl. FeedbackAsync) | 32.08 KB | | [@sentry/react](https://github.com/sentry/react) | 25.39 KB | | [@sentry/react](https://github.com/sentry/react) (incl. Tracing) | 37.86 KB | | [@sentry/vue](https://github.com/sentry/vue) | 26.8 KB | | [@sentry/vue](https://github.com/sentry/vue) (incl. Tracing) | 36.77 KB | | [@sentry/svelte](https://github.com/sentry/svelte) | 22.77 KB | | CDN Bundle | 23.95 KB | | CDN Bundle (incl. Tracing) | 36.66 KB | | CDN Bundle (incl. Tracing, Replay) | 71.15 KB | | CDN Bundle (incl. Tracing, Replay, Feedback) | 76.45 KB | | CDN Bundle - uncompressed | 70.17 KB | | CDN Bundle (incl. Tracing) - uncompressed | 108.68 KB | | CDN Bundle (incl. Tracing, Replay) - uncompressed | 220.58 KB | | CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed | 233.79 KB | | [@sentry/nextjs](https://github.com/sentry/nextjs) (client) | 37.82 KB | | [@sentry/sveltekit](https://github.com/sentry/sveltekit) (client) | 35.45 KB | | [@sentry/node](https://github.com/sentry/node) | 125.13 KB | | [@sentry/node](https://github.com/sentry/node) - without tracing | 93.58 KB | | [@sentry/aws-serverless](https://github.com/sentry/aws-serverless) | 103.28 KB | ### [`v8.33.0`](https://github.com/getsentry/sentry-javascript/blob/HEAD/CHANGELOG.md#8330) [Compare Source](getsentry/sentry-javascript@8.32.0...8.33.0) ##### Important Changes - **feat(nextjs): Support new async APIs (`headers()`, `params`, `searchParams`) ([#13828](getsentry/sentry-javascript#13828 Adds support for [new dynamic Next.js APIs](vercel/next.js#68812). - **feat(node): Add `lru-memoizer` instrumentation ([#13796](getsentry/sentry-javascript#13796 Adds integration for lru-memoizer using [@opentelemetry/instrumentation-lru-memoizer](https://github.com/opentelemetry/instrumentation-lru-memoizer). - **feat(nuxt): Add `unstable_sentryBundlerPluginOptions` to module options ([#13811](getsentry/sentry-javascript#13811 Allows passing other options from the bundler plugins (vite and rollup) to Nuxt module options. ##### Other Changes - fix(browser): Ensure `wrap()` only returns functions ([#13838](getsentry/sentry-javascript#13838)) - fix(core): Adapt trpc middleware input attachment ([#13831](getsentry/sentry-javascript#13831)) - fix(core): Don't return trace data in `getTraceData` and `getTraceMetaTags` if SDK is disabled ([#13760](getsentry/sentry-javascript#13760)) - fix(nuxt): Don't restrict source map assets upload ([#13800](getsentry/sentry-javascript#13800)) - fix(nuxt): Use absolute path for client config ([#13798](getsentry/sentry-javascript#13798)) - fix(replay): Stop global event handling for paused replays ([#13815](getsentry/sentry-javascript#13815)) - fix(sveltekit): add url param to source map upload options ([#13812](getsentry/sentry-javascript#13812)) - fix(types): Add jsdocs to cron types ([#13776](getsentry/sentry-javascript#13776)) - fix(nextjs): Loosen [@sentry/nextjs](https://github.com/sentry/nextjs) webpack peer dependency ([#13826](getsentry/sentry-javascript#13826)) Work in this release was contributed by [@joshuajaco](https://github.com/joshuajaco). Thank you for your contribution! </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/186 Reviewed-by: Alexandre Soro <code@soro.dev> Co-authored-by: renovate <renovate@git.tristess.app> Co-committed-by: renovate <renovate@git.tristess.app>

mydea requested a review from lforst September 26, 2024 11:03

Update fetch.ts

335adcc

adjust format

AbhiPrasad self-requested a review September 26, 2024 12:59

lforst self-assigned this Sep 30, 2024

lforst added 3 commits October 2, 2024 12:51

Free usage of reader and and define max timeout of 90s

d20c03b

Update tests

91396d0

cancel body instead of reader and add comment

1846cd4

lforst requested a review from chargome October 2, 2024 14:30

chargome approved these changes Oct 2, 2024

View reviewed changes

lforst merged commit 0b739c5 into getsentry:develop Oct 3, 2024
127 checks passed

This was referenced Oct 3, 2024

ref: Add external contributor to CHANGELOG.md #13858

Closed

Memory leak with endless streaming (example: CCTV) #13806

Closed

chargome mentioned this pull request Oct 11, 2024

feat(core): Make stream instrumentation opt-in #13951

Merged

q1blue mentioned this pull request Oct 24, 2024

[Snyk] Upgrade @sentry/react from 7.16.0 to 8.33.1 https-quantumblockchainai-atlassian-net/ossinsight#952

Open

kenn7575 mentioned this pull request Oct 27, 2024

[Snyk] Upgrade @sentry/sveltekit from 8.32.0 to 8.33.1 kenn7575/kenni-kollemorten-v2#78

Merged

tristanrobert mentioned this pull request Oct 28, 2024

[Snyk] Upgrade @sentry/react from 8.30.0 to 8.33.1 MTES-MCT/carbure#61

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fetch): Fix memory leak when handling endless streaming #13809

fix(fetch): Fix memory leak when handling endless streaming #13809

soapproject commented Sep 26, 2024

lforst commented Sep 30, 2024

soapproject commented Sep 30, 2024

soapproject commented Sep 30, 2024

lforst commented Oct 2, 2024

chargome Oct 2, 2024

lforst Oct 3, 2024

fix(fetch): Fix memory leak when handling endless streaming #13809

fix(fetch): Fix memory leak when handling endless streaming #13809

Conversation

soapproject commented Sep 26, 2024

lforst commented Sep 30, 2024

soapproject commented Sep 30, 2024

soapproject commented Sep 30, 2024

lforst commented Oct 2, 2024

chargome Oct 2, 2024

Choose a reason for hiding this comment

lforst Oct 3, 2024

Choose a reason for hiding this comment