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 timer callback triggering after timer is cleared #865

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented May 22, 2018

Fixes elastic/kibana#19285

We could get fancy and fix this specific situation through more coordination, or we can be defensive and just not trigger the callback if it's been removed. I opted for the second because of its simplicity and it addresses any related bugs that may occur in the future.

This also fixes a second bug where the toast's countdown is not restarted for the correct amount of time after the second hover.

what causes the error When a toast managed by `EuiGlobalToastList` reaches the end of its timer, the timer self-clears and triggers the callback, beginning the process of fading out the element. The toast fade-and-remove can be laid out as
  • timer's timeout happens
    • timer.callback()
      • toast list tracks this toast as dismissed (it starts fading out)
      • toast list sets timeout X
    • timer.clear()
      • removes timer callback
  • timeout X happens
    • toast dismissal callback happens, which removes toast from the list
    • toast's timer is cleared, to be safe (allowing dismissals outside of this flow)

The bug as described in the kibana issue can happen by a 3rd event triggering between the timer's timeout and timeout X: EuiGlobalToastList's onMouseLeave handler cycles through the known timers and calls resume, re-creating their setTimeouts. The list of known timers at this point includes elements that are fading out, which have already completed their timers, and their callbacks have been removed.

Copy link

@rhoboat rhoboat left a comment

Choose a reason for hiding this comment

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

LGTM!

@chandlerprall chandlerprall merged commit 5d5d9ad into elastic:master May 22, 2018
this.callback();
if (this.callback) {
this.callback();
}
Copy link

Choose a reason for hiding this comment

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

This looks good to me, but I'm just wondering: these methods all look like they could benefit from some awaits or promise handling. Wherever Timer is used, there might be a need to chain events making sure the previous function finished executing. I'm just a little surprised looking through this class, that it doesn't await this.callback(). I guess nothing that calls finish needs to be chained with something else?

chandlerprall added a commit to chandlerprall/eui that referenced this pull request May 24, 2018
* ensure that a timer has not already been cleared when calling the callback

* changelog
@chandlerprall chandlerprall deleted the bug/kibana-19285-fatal-toast-error branch June 1, 2018 16:30
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.

Fatal callback error with multiple report generations / notifications
2 participants