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 crash if a Context is stopped immediately after creation #120

Merged
merged 1 commit into from
Apr 13, 2024

Conversation

rcahoon
Copy link
Member

@rcahoon rcahoon commented Apr 7, 2024

Description

Context waits in the initial waitForControl in threadFunction from when it first is created until it is first allowed to run. If stop() is called on the Context during this time, ContextStoppedException will be thrown and nothing will catch it. This leads the Thread to crash, which also brings down the JVM, due to our default exception handler (NB: if it didn't bring down the JVM, it would cause a dead-lock, which wouldn't be any better).

ContextStoppedException is supposed to be called in a Context's thread when the Context is stopped, but it is also supposed to be caught in the try-catch in threadFunction. This PR fixes the issue by moving the initial call to waitForControl into the try-catch.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Be detailed so that your code reviewer can understand exactly how much and what kinds of testing were done, and which might still be worthwhile to do.

  • Unit tests: New unit test included in this PR. Test fails in main but passes on this branch.
  • Simulator testing: [Add your description here]
  • On-robot bench testing: [Add your description here]
  • On-robot field testing: [Add your description here]

@rcahoon rcahoon force-pushed the rcahoon/fix-context-crash-on-immediate-stop branch from bfae3fb to e418779 Compare April 7, 2024 07:05
@dejabot
Copy link
Contributor

dejabot commented Apr 7, 2024

for future PRs (post comp), we may also want to:

  • reduce severity of log message when this happens
  • make Logger.'s LogUncaughtException very clearly indicate that it is exiting (with a preceding **** or similar?) so this jumps out in logs vs other usages of LoggerExceptionUtils

if this sg, happy to make these changes.

thank you again!

@rcahoon rcahoon force-pushed the rcahoon/fix-context-crash-on-immediate-stop branch from e418779 to ca4c052 Compare April 13, 2024 20:39
@rcahoon rcahoon merged commit 5aaaabc into main Apr 13, 2024
3 checks passed
@rcahoon rcahoon deleted the rcahoon/fix-context-crash-on-immediate-stop branch April 13, 2024 20:45
@rcahoon
Copy link
Member Author

rcahoon commented Apr 13, 2024

for future PRs (post comp), we may also want to:

  • reduce severity of log message when this happens
  • make Logger.'s LogUncaughtException very clearly indicate that it is exiting (with a preceding **** or similar?) so this jumps out in logs vs other usages of LoggerExceptionUtils

if this sg, happy to make these changes.

These sound great. I'd be fully in support of these improvements

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.

2 participants