Skip to content

Concurrent flush calls never ends (does not reject/return/throw) #2131

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

Closed
1 task done
enapupe opened this issue Jun 24, 2019 · 3 comments · Fixed by #2143
Closed
1 task done

Concurrent flush calls never ends (does not reject/return/throw) #2131

enapupe opened this issue Jun 24, 2019 · 3 comments · Fixed by #2143

Comments

@enapupe
Copy link

enapupe commented Jun 24, 2019

Package + Version

  • @sentry/core

Version:

v5.0.8

Description

Flush never resolves/rejects (hangs forever) when called concurrently.
When called concurrently, the second (or is it the first?) flush call never resolves. It seems the second call to flush kills the setInterval happening for the first on:

clearInterval(this._processingInterval);

(clearing the interval here ^^^^ kills other possibly running calls, it should resolve all instances instead.)

In other words, it seems _isClientProcessing relies on setInterval in order to resolve/reject the promise, but this interval is being cleared on the referenced line above. This makes _isClientProcessing promise to HANG forever.

One easy (but kinda ugly) workaround, would be adding a Promise.race using that timeout - I've tested this change locally and it works OK:

    /** Waits for the client to be done with processing. */
    _isClientProcessing(timeout) {
        return Promise.race([new Promise(resolve => {
            let ticked = 0;
            const tick = 1;
            if (this._processingInterval) {
                clearInterval(this._processingInterval);
            }
            this._processingInterval = setInterval(() => {
                if (!this._processing) {
                    resolve(true);
                }
                else {
                    ticked += tick;
                    if (timeout && ticked >= timeout) {
                        resolve(false);
                    }
                }
            }, tick);
        }), new Promise((resolve) => {
            setTimeout(() => {
                if (!this._processing){
                    return resolve(true)
                }
                return resolve (false)
            }, timeout)
        })])
@esetnik
Copy link

esetnik commented Jul 1, 2019

@enapupe one major downside of the Promise.race solution is that the concurrent flush will take a minimum of 2s to complete. This is not the case for a real flush with a small or empty queue so it's artificially slowing down the speed of concurrent flush operations.

@enapupe
Copy link
Author

enapupe commented Jul 1, 2019

Absolutely, this is just a hack to illustrate what's going on, having it default to that timeout would be really bad.

@kamilogorek
Copy link
Contributor

Thanks for the detailed repro @enapupe! Fixed in #2143

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 a pull request may close this issue.

3 participants