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

Sentry.captureException doesn't close readable stream opened by createReadStream in getContextLinesFromFile #14892

Closed
3 tasks done
mstrokin opened this issue Jan 2, 2025 · 9 comments · Fixed by #14995
Closed
3 tasks done
Assignees
Labels
Package: node Issues related to the Sentry Node SDK

Comments

@mstrokin
Copy link

mstrokin commented Jan 2, 2025

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.47.0

Framework Version

No response

Link to Sentry event

No response

Reproduction Example/SDK Setup

https://github.com/mstrokin/sentry-openfiles-bug

Steps to Reproduce

Running this in a Node Express server opens same files repeatedly without ever closing them, since 8.10.0 (8.9.0 works fine).

for (var i = 0; i < 1000; i++) {
Sentry.captureException('fake exception to test the files opening');
}

This is the cause of it: #12221

const stream = createReadStream(path);

this stream never gets closed (even though it supposed to use autoClose I think)

Expected Result

There are no open files in lsof output after sending the exceptions.

Actual Result

New files are opened every time captureException is called.

@mstrokin
Copy link
Author

mstrokin commented Jan 2, 2025

@mstrokin
Copy link
Author

mstrokin commented Jan 2, 2025

Removed Express from the reproducible example, It's a Sentry bug after all.

@mstrokin
Copy link
Author

mstrokin commented Jan 2, 2025

The worst scenario happens if there's a loop doing captureException - it leads to same files being reopened & not closed.

I.e. If we call captureException using an Express route it doesn't open extra files with subsequent HTTP requests, it only reads files once it seems.

@AbhiPrasad
Copy link
Member

Hey @mstrokin thanks for the repro and PR!

What version of node did you run the repro with? Want to see what test matrix to add to the PR.

Assigning myself this issue, will try to update the PR tomorrow and get it merged in!

@mstrokin
Copy link
Author

mstrokin commented Jan 3, 2025

Node v23.5.0 locally and Node v20 alpine on the server

Tested it again on all of these, works the same everywhere.
lts/fermium -> v14.21.3
lts/gallium -> v16.20.2
lts/hydrogen -> v18.20.5
lts/iron -> v20.18.1
lts/jod -> v22.12.0

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Jan 3, 2025
@mstrokin
Copy link
Author

mstrokin commented Jan 3, 2025

Also, fun fact that I didn't debug - if you move the code into a single file (i.e. move App code into main.ts) - it stops happening, I guess it already has a file in the memory in that case.

@mstrokin
Copy link
Author

mstrokin commented Jan 3, 2025

I've debugged it a bit more and added new commit to repro repo, the bug is here - if we use lineReaded.removeAllListeners() it never closes the stream, not sure if it's a Node bug.

if (currentRangeIndex === ranges.length - 1) {
          // We need to close the file stream and remove listeners, else the reader will continue to run our listener;
          lineReaded.close();
          lineReaded.removeAllListeners();
          return;
}

@mstrokin
Copy link
Author

mstrokin commented Jan 3, 2025

nodejs/node#9002 Node says it's not a bug

@mstrokin mstrokin changed the title Sentry.captureException doesn't close readable stream createReadStream in getContextLinesFromFile Sentry.captureException doesn't close readable stream opened by createReadStream in getContextLinesFromFile Jan 3, 2025
@AbhiPrasad
Copy link
Member

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