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

Fix _processing flag in BaseClient. #2983

Merged

Conversation

marshall-lee
Copy link
Contributor

I had experienced some problems with flush() in a following scenario:

captureException(error); // Sets _processing=true. Internally, this one is asynchronous because of file I/O
captureMessage('foo');   // Sets _processing=true and immediately sets _processing=false because it's synchronous.
await flush(2000);       // Then, flush doesn't wait for captured error because _processing=false but the message is not even in a buffer yet.
os.exit();               // Finally, I miss that exception in Sentry.

I tried to change fs.readFile to fs.readFileSync in packages/node/src/parsers.ts but file I/O is not the only issue. beforeSend callback could result in asynchronous execution too.

So my proposal is to change _processing from boolean to number counter. Another option would be to introduce another PromiseBuffer and just drain it. What do you think?

Client processes events concurrently so it should be a counter, not a boolean.
@kamilogorek
Copy link
Contributor

I'm totally fine with a solution like that. And thankfully it works out of the box with my todays changes to processEvent 😅
Thanks!

@kamilogorek kamilogorek merged commit 7fc797b into getsentry:master Oct 20, 2020
@marshall-lee marshall-lee deleted the baseclient-processing-counter branch October 20, 2020 10:54
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.

None yet

2 participants