-
Notifications
You must be signed in to change notification settings - Fork 639
fix: Reduce from error to warning for message when failing to terminate #3621
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: Reduce from error to warning for message when failing to terminate #3621
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3621 +/- ##
=======================================
Coverage 98.28% 98.28%
=======================================
Files 417 417
Lines 11772 11772
Branches 1832 1832
=======================================
Hits 11570 11570
Misses 202 202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I had some questions about how we're handling this scenario, but regardless of the answers there, this seems like an appropriate change.
| ); | ||
|
|
||
| if (result === hasTimedOut || result !== 'OK') { | ||
| // We tried to shutdown gracefully but failed. This probably means the Snap is in infinite loop and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment accurate? Does a failure to terminate gracefully always (or usually) indicate a bug in the snap? If so, what impact would we expect this to have on users if any?
I ask because we may want to consider forcibly disabling the snap and/or warning the user that it's misbehaving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the assumption originally, but I removed it specifically because I didn't think it was accurate anymore.
The Snap gets very limited time to gracefully terminate (1s), so on lower-end systems it seems likelier now that we may see this as a "false-postive" of sorts. Especially since we see this on our own Snaps in production which shouldn't be hogging CPU by running a while-loop.
It may still warrant some investigation into whether we can better guarantee graceful termination and whether there is any issues in our own Snaps that cause this to happen more often.
When failing to terminate gracefully we currently log an error, it can be a warning instead since it doesn't prevent the Snap from terminating.
Intentionally also removing the logging of
resultwhich is almost always a symbol that clutters the error message.