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.

Sign up for GitHub

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

Jump to bottom

FLV.js Stream Release Issue with Sentry #13950

Closed
3 tasks done
Lei-k opened this issue Oct 11, 2024 · 4 comments · Fixed by #13951
Closed
3 tasks done

FLV.js Stream Release Issue with Sentry #13950

Lei-k opened this issue Oct 11, 2024 · 4 comments · Fixed by #13951
Assignees
@chargome
Labels
Package-Meta: CDN

Comments

@Lei-k
Copy link

Lei-k commented Oct 11, 2024 •
edited
Loading

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

Sentry Browser CDN bundle

SDK Version

via cdn (https://js.sentry-cdn.com/bfb3eb790cf5b7cc59d23d07aa079024.min.js)

Framework Version

No response

Link to Sentry event

No response

Reproduction Example/SDK Setup

Sentry.init({
  dsn: "<dsn>",

  // Alternatively, use `process.env.npm_package_version` for a dynamic release version
  // if your build tool supports it.
  release: "[email protected]",
  integrations: [
  ],

  // Set tracesSampleRate to 1.0 to capture 100%
  // of transactions for tracing.
  // We recommend adjusting this value in production
  tracesSampleRate: 1.0,

  // Set `tracePropagationTargets` to control for which URLs trace propagation should be enabled
  tracePropagationTargets: ["localhost", /^https:\/\/yourserver\.io\/api/],

  // Capture Replay for 10% of all sessions,
  // plus for 100% of sessions with an error
  replaysSessionSampleRate: 0.1,
  replaysOnErrorSampleRate: 1.0,
});

Hi Sentry team,

In my project, I am using flv.js to stream video, and it has been working well. However, after integrating Sentry.js, I encountered two issues:

Issue 1:
There was a memory leak, which I believe has been addressed in this issue.

Issue 2:
The stream cannot be released. When using flv.js's destroy() method, the connection is not properly closed, even though this API works fine without Sentry integration.

Since Issue 1 seems to have been resolved, I am focusing solely on Issue 2 in this report.

Steps to Reproduce

  1. Create an index.html with the following content:
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Flv.js With Sentry</title>
    <script
      src="https://js.sentry-cdn.com/bfb3eb790cf5b7cc59d23d07aa079024.min.js"
      crossorigin="anonymous"
    ></script>
    <script src="https://cdn.jsdelivr.net/npm/flv.js/dist/flv.min.js"></script>
</head>
<body>
    <h1>Hello Flv.js With Sentry!</h1>
    <video id="videoElement" controls width="640" height="360"></video>
    
    <script src="index.js"></script>
</body>
</html>
  1. Create an index.js with the following content:
Sentry.init({
  dsn: "<dsn>",
  release: "[email protected]",
  integrations: [],
  tracesSampleRate: 1.0,
  replaysSessionSampleRate: 0.1,
  replaysOnErrorSampleRate: 1.0,
});

if (flvjs.isSupported()) {
  var videoElement = document.getElementById('videoElement');
  var flvPlayer = flvjs.createPlayer({
      type: 'flv',
      url: 'http://10.0.0.6:8080/live?port=1935&app=live&stream=car_test'
  });
  flvPlayer.attachMediaElement(videoElement);
  flvPlayer.load();
  flvPlayer.play();

  setTimeout(() => {
    flvPlayer.destroy();
  }, 10 * 1000);
} else {
  console.error('FLV.js is not supported in this browser.');
}

Expected Result

The resources should be properly released after 10 seconds, as shown in below

Image

Actual Result

The connection is not released and stays open, as shown in below

Image

Additional Information:

I also noticed that in packages/utils/src/instrument/fetch.ts, response.clone() is used to split off a new ReadableStream. Since flv.js on Chrome does not use abort signals but instead cancels the stream using the cancel() API, flv.js is unaware of the Sentry-generated ReadableStream branch. As a result, flv.js cannot cancel Sentry's stream branch, causing Chrome to think there is still an active reader, preventing the release of resources.

Below is the related part of flv.js:

At line 174:

abort() {
    this._requestAbort = true;

    if (this._status !== LoaderStatus.kBuffering || !Browser.chrome) {
        if (this._abortController) {
            try {
                this._abortController.abort();
            } catch (e) {}
        }
    }
}

At line 212:

} else if (this._requestAbort === true) {
    this._status = LoaderStatus.kComplete;
    return reader.cancel();
}
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 11, 2024
@mydea
Copy link
Member

mydea commented Oct 11, 2024

Hey, thank you for the detailed report!

Just to be sure, you are not using replayIntegration, are you? Because I do not see the integration being added, but I see the replay sample rates being defined (which is fine, just wanting to double check to avoid confusion).

If replay is not added, then the problem is most likely what you alluded to already. We'll look into it!

@Lei-k
Copy link
Author

Lei-k commented Oct 11, 2024

Hi @mydea

I have actually tested both versions—one with replayIntegration and one without it—and the issue occurs in both cases.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 11, 2024
@chargome chargome self-assigned this Oct 11, 2024
@chargome chargome mentioned this issue Oct 11, 2024
Merged
@mydea
Copy link
Member

mydea commented Oct 11, 2024

Thanks for investigating. We will probably make the stream instrumentation opt-in then, as it appears that this is not 100% safe for us to patch and hard to handle all edge cases - see #13951

@Lei-k Lei-k mentioned this issue Oct 12, 2024
Open
@github-actions GitHub Actions
Copy link
Contributor

A PR closing this issue has just been released 🚀

This issue was referenced by PR #13951, which was included in the 8.35.0-beta.0 release.

@Lei-k Lei-k mentioned this issue Oct 16, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chargome chargome

Labels
Package-Meta: CDN
Projects
GitHub Issues with 👀 3
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(core): Make stream instrumentation opt-in getsentry/sentry-javascript
3 participants
@mydea @Lei-k @chargome