Skip to content

fix: Wait for processing to finish before closing transport #2005

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

Merged
merged 8 commits into from
Apr 10, 2019

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Apr 9, 2019

Fixes #2000

There was an issue with flush that we have to wait for processing of the client to finish first before we ask the transport to drain.

cc @sheerun thanks for pointing this out

@HazAT HazAT self-assigned this Apr 9, 2019
@HazAT HazAT requested a review from kamilogorek as a code owner April 9, 2019 09:17
@getsentry-bot
Copy link
Contributor

getsentry-bot commented Apr 9, 2019

Messages
📖 ✅ TSLint passed
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 15.5459 kB) (ES6: 14.2676 kB)

Generated by 🚫 dangerJS against 9b7fff8

@HazAT HazAT force-pushed the fix/flush-close branch from d93e183 to 7ce67f1 Compare April 10, 2019 07:21
@@ -58,6 +58,9 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
/** Is the client still processing a call? */
protected _processing: boolean = false;

/** Processing interval */
protected _processingInterval?: NodeJS.Timeout;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this will be available in browser environments, which may cause same issus as with getCurrentHub and Window

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@kamilogorek
Copy link
Contributor

I'm not sure about the performance implications of this change, but the implementation looks fine

@HazAT
Copy link
Member Author

HazAT commented Apr 10, 2019

So I agree, but considering this only ever applies if you call close/flush I think it's fine.

@HazAT HazAT merged commit c3b7940 into master Apr 10, 2019
@HazAT HazAT deleted the fix/flush-close branch April 10, 2019 09:08
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.

3 participants