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

[do not land] handle runtime shutdown more gracefully #6875

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Oct 15, 2024

In #6505 we identified a situation where panics were occurring because the
Tokio runtime was shutting down. Making panics work better requires essentially
making all of the database code and its dependents be aware of the situation
where runtime shutdown is happening.

This is incomplete work that builds on top of oxidecomputer/async-bb8-diesel#77,
but I wanted to show the general direction we would
take here. The next step from here would probably be to detect the runtime
shutdown situation in background tasks, and change BackgroundTask::activate
to cleanly exit in those situations.

I don't think this is worth pursuing to be honest -- I'd consider this an
interesting experiment but ultimately a negative result.

Created using spr 1.3.6-beta.1
@@ -276,6 +276,12 @@ impl Error {
Error::ServiceUnavailable { internal_message: message.to_owned() }
}

pub fn runtime_shutdown() -> Error {
Error::ServiceUnavailable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd make this another variant so it can be detected by callers.

@sunshowers
Copy link
Contributor Author

This would be nice and I think there's a path here, but ultimately the juice doesn't seem worth the squeeze for now. Going to close this in favor of oxidecomputer/qorb#45.

@sunshowers sunshowers closed this Oct 15, 2024
@sunshowers sunshowers deleted the sunshowers/spr/do-not-land-handle-runtime-shutdown-more-gracefully branch October 15, 2024 19:17
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.

1 participant