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

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

Closed
3 tasks done
soapproject opened this issue Sep 26, 2024 · 2 comments · Fixed by #13809
Closed
3 tasks done

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

soapproject opened this issue Sep 26, 2024 · 2 comments · Fixed by #13809
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK

Comments

@soapproject
Copy link
Contributor

Is there an existing issue for this?

How do you use Sentry?

Self-hosted/on-premise

Which SDK are you using?

@sentry/nextjs

SDK Version

8.30.0

Framework Version

next 14.2.13, react 18.2.0, video.js 7.21.1

Link to Sentry event

No response

Reproduction Example/SDK Setup

// next.config.js
const { withSentryConfig } = require('@sentry/nextjs');

module.exports = withSentryConfig(bundledConfig, {
  org: 'xxx',
  project: 'xxx',
  sentryUrl: 'https://xxx/',
  silent: !process.env.CI,
  widenClientFileUpload: true,
  reactComponentAnnotation: {
    enabled: false,
  },
  tunnelRoute: '/monitoring',
  hideSourceMaps: false,
  disableLogger: true,
  automaticVercelMonitors: true,
});

// sentry.client.config.ts
import * as Sentry from '@sentry/nextjs';

Sentry.init({
  dsn: 'https://xxx',
  integrations: [Sentry.replayIntegration()],
  tracesSampleRate: 1,
  replaysSessionSampleRate: 0.1,
  replaysOnErrorSampleRate: 1.0,
  debug: true,
});

// sentry.edge.config.ts
import * as Sentry from '@sentry/nextjs';

Sentry.init({
  dsn: 'https://xxx',
  tracesSampleRate: 1,
  debug: false,
});

// sentry.server.config.ts
import * as Sentry from '@sentry/nextjs';

Sentry.init({
  dsn: 'https://xxx',
  tracesSampleRate: 1,
  debug: false,
});

Steps to Reproduce

  1. Play an endless live stream (CCTV in FLV format).
  2. Memory usage keeps increasing over time.

Expected Result

I believe this is causing the issue:
Unresolved promises and their context are stacking up in the task queue.

// sentry-javascript/packages/utils/src/instrument/fetch.ts

async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
  if (res && res.body && res.body.getReader) {
    const responseReader = res.body.getReader();

    // eslint-disable-next-line no-inner-declarations
    async function consumeChunks({ done }: { done: boolean }): Promise<void> {
      if (!done) {
        try {
          // abort reading if read op takes more than 5s
          const result = await Promise.race([
            responseReader.read(),
            new Promise<{ done: boolean }>(res => {
              setTimeout(() => {
                res({ done: true });
              }, 5000);
            }),
          ]);
          await consumeChunks(result);
        } catch (error) {
          // handle error if needed
        }
      } else {
        return Promise.resolve();
      }
    }

    return responseReader
      .read()
      .then(consumeChunks)
      .then(onFinishedResolving)
      .catch(() => undefined);
  }
}

Is there any specific reason to use recursion in this case? If not, may I submit a PR with an alternative approach?

example:

async function resolveResponse(res, onFinishedResolving) {
  if (!res?.body?.getReader) return;

  const responseReader = res.body.getReader();

  while (true) {
    try {
      const { done } = await Promise.race([
        responseReader.read(),
        new Promise((resolve) => setTimeout(() => resolve({ done: true }), 5000)),
      ]);

      if (done) break;
    } catch (error) {
      // handle error if needed
      break;
    }
  }

  responseReader.releaseLock();
  onFinishedResolving();
}

Actual Result

Image

@github-actions github-actions bot added the Package: nextjs Issues related to the Sentry Nextjs SDK label Sep 26, 2024
@Lms24
Copy link
Member

Lms24 commented Sep 26, 2024

Hi @soapproject thanks for writing in! I don't have a lot of context on the particular code here but generally, PRs are always welcome. I believe our fetch instrumentation is well-tested so if the fix causes other problems, we should (lol famous last words) discover it. If you submit a PR I'll ask @lforst for a review when he's back.

soapproject added a commit to AI-RIDER/sentry-javascript that referenced this issue Sep 26, 2024
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
soapproject added a commit to soapproject/sentry-javascript that referenced this issue Sep 26, 2024
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
Copy link
Contributor

github-actions bot commented Oct 3, 2024

A PR closing this issue has just been released 🚀

This issue was referenced by PR #13809, which was included in the 8.33.1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants