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.

Sign up for GitHub

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

Jump to bottom

Bug 1838453 - Shutdown timeout: Gracefully handle the waiting thread going away #2503

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Bug 1838453 - Shutdown timeout: Gracefully handle the waiting thread going away #2503

merged 1 commit into from
Jun 20, 2023

Conversation

badboy
Copy link
Member

@badboy badboy commented Jun 15, 2023

Bringing this back. Not sure how to best test this.

There's in theory now an edge case where other things could crash because the dispatcher could continue to run, but Glean already being gone.

@badboy badboy force-pushed the 1838453/prevent-crash-on-shutdown branch from 9be0ca5 to ba31a44 Compare June 20, 2023 14:41
@badboy badboy marked this pull request as ready for review June 20, 2023 14:50
@badboy badboy force-pushed the 1838453/prevent-crash-on-shutdown branch from ba31a44 to dcb1bb1 Compare June 20, 2023 14:50
@badboy badboy requested a review from a team as a code owner June 20, 2023 14:50
@badboy badboy requested review from chutten and removed request for a team June 20, 2023 14:50
@badboy
Copy link
Member Author

badboy commented Jun 20, 2023 •
edited
Loading

I ran this locally using the prototype example, modified to put a very long task on the dispatcher before shutdown. This does avoid the panic then as expected.

It does have the side-effect that the dispatcher then would still continue, but I filed a follow-up bug for that.

chutten
chutten approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@chutten chutten left a comment

Choose a reason for hiding this comment

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

r+wc (the c stands for Changelog)

I'm guessing the manual test doesn't translate to an automated one?

@badboy
Copy link
Member Author

badboy commented Jun 20, 2023

r+wc (the c stands for Changelog)

I'm guessing the manual test doesn't translate to an automated one?

Unfortuantely not. We hard-code the timeout to 10s and the only way to actually observe the behavior right now is through the logs.

@badboy badboy force-pushed the 1838453/prevent-crash-on-shutdown branch from dcb1bb1 to d7cb68c Compare June 20, 2023 15:57
@badboy badboy enabled auto-merge (rebase) June 20, 2023 16:02
@badboy badboy merged commit 2439105 into main Jun 20, 2023
@badboy badboy deleted the 1838453/prevent-crash-on-shutdown branch June 20, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chutten chutten chutten approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@badboy @chutten