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

Added exception detection to ThreadFinish() #2205

Merged
merged 1 commit into from
Dec 30, 2017

Conversation

Dunbaratu
Copy link
Member

This sort of fixes one half of issue #2203.

Issue #2203 had two halves to it:

part 1 - When the problem happened, the exception got re-thrown
in a loop forever, never relinquishing control. Regardless
of the cause of the exception, throwing an exception shouldn't
cause kOS to get stuck in a loop like that. Something is wrong
with the error handling system.

part 2 - The problem that threw the exception shouldn't have
happened in the first place.

This PR fixes just part 1. Part 2 is in a separate PR. That is
deliberate because you cannot test the exception message
handling if the bug that caused the exception is fixed. That bug
needs to still be present to prove that the error message system
is handling it better.


The cause of the exception looping:

Compiling happens in a thread, using the YieldFinishedDetector system.

When the CPU wakes up each tick to see if a YieldFinishDetector thread
is done, it calls IsFinished() to do so. If IsFinished() notices that
ThreadExecute() is done, then it calls ThreadFinish() to do any cleanup the
class wants to do, and after that sets some flags that tells it the
thread is done and IsFinished() no longer has to be called.

Compiling consists of 2 steps, Compile and BuildProgram, and only
Compile was put under the ThreadExecute() control. The BuildProgram
step was being done inside ThreadFinish().

The problem is that if ThreadFinish() aborts with an exception, then IsFinished
aborts, never reaching the code that sets flags telling the CPU the thread is done.
So on the next tick, the CPU tries calling IsFinished() again, which notices
that ThreadExecute() is done (still) so it tries calling ThreadFinish(), and
triggers the whole thing again.


The fix: there are two possible fixes, and I chose the one that didn't
involve touching code I didn't write.

possible fix 1 - move the BuildProgram step inside the ThreadExecute().
This is more in keeping with the design intent of ThreadExecute(), but would
have involved touching code I don't want to touch.

possible fix 2 - Give ThreadFinish() the same exception checking that
ThreadExecute() has so an exception in ThreadFinished() won't prevent the
system from noticing that the thread indeed has finished.

I chose fix 2.

This sort of fixes one half of issue KSP-KOS#2203.

Issue KSP-KOS#2203 had two halves to it:

part 1 - When the problem happened, the exception got re-thrown
in a loop forever, never relinquishing control.  Regardless
of the cause of the exception, throwing an exception shouldn't
cause kOS to get stuck in a loop like that.  Something is wrong
with the error handling system.

part 2  - The problem that threw the exception shouldn't have
happened in the first place.

This PR fixes just part 1.  Part 2 is in a separate PR.  That is
deliberate because you cannot test the exception message
handling if the bug that caused the exception is fixed.  That bug
needs to still be present to prove that the error message system
is handling it better.

-----------
The cause of the exception looping:

Compiling happens in a thread, using the YieldFinishedDetector system.

When the CPU wakes up each tick to see if a YieldFinishDetector thread
is done, it calls IsFinished() to do so.  If IsFinished() notices that
`ThreadExecute()` is done, then it calls `ThreadFinish()` to do any cleanup the
class wants to do, and after that sets some flags that tells it the
thread is done and IsFinished() no longer has to be called.

Compiling consists of 2 steps, Compile and BuildProgram, and only
Compile was put under the `ThreadExecute()` control.  The BuildProgram
step was being done inside `ThreadFinish()`.

The problem is that if ThreadFinish() aborts with an exception, then IsFinished
aborts, never reaching the code that sets flags telling the CPU the thread is done.
So on the next tick, the CPU tries calling IsFinished() again, which notices
that ThreadExecute() is done (still) so it tries calling ThreadFinish(), and
triggers the whole thing again.

--------------
The fix: there are two possible fixes, and I chose the one that didn't
involve touching code I didn't write.

possible fix 1 - move the BuildProgram step inside the `ThreadExecute()`.
This is more in keeping with the design intent of ThreadExecute(), but would
have involved touching code I don't want to touch.

possible fix 2 - Give `ThreadFinish()` the same exception checking that
`ThreadExecute()` has so an exception in ThreadFinished() won't prevent the
system from noticing that the thread indeed has finished.

I chose fix 2.
@Dunbaratu Dunbaratu added bug Weird outcome is probably not what the mod programmer expected. Error Messages Feedback to the user explaining an error doesn't explain it properly. labels Dec 29, 2017
@hvacengi hvacengi merged commit 19f2de6 into KSP-KOS:develop Dec 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Weird outcome is probably not what the mod programmer expected. Error Messages Feedback to the user explaining an error doesn't explain it properly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants