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

kOS 1.1.4: kOS.Safe.Exceptions.KOSCompileException: ProgramBuilder.ReplaceLabels #2203

Closed
pellinor0 opened this issue Dec 23, 2017 · 6 comments · Fixed by #2206
Closed

kOS 1.1.4: kOS.Safe.Exceptions.KOSCompileException: ProgramBuilder.ReplaceLabels #2203

pellinor0 opened this issue Dec 23, 2017 · 6 comments · Fixed by #2206
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.

Comments

@pellinor0
Copy link

pellinor0 commented Dec 23, 2017

I got this error when upgrading to kOS 1.1.4.

I have boiled it down to 3 functions that are defined in a file tmp.ks. When I try to run that file, kOS gets caught in a loop of errors, and KSP becomes unresponsive. The only way to get out is by closing the window running KSP.

When I remove one of those functions, the script runs without error.

( Links removed, since they seem to be expired )

Reproduction: in KSP 1.3.1 with only kOS and ModuleManager

  • launch ship with kOS core on it
  • switch to 0.

  • run tmp.ks.

Edit: the file compiles fine, and when running the compiled version, I get the error once without having to restart the game every time.

@pellinor0
Copy link
Author

pellinor0 commented Dec 23, 2017

I could boil it down further: there seems to be a problem with the symbol "d". The code below triggers the errors. Once I declare the first d as a "local function", things work again (defining it locally was the intention anyway).

@lazyglobal off.
function timeToDist {
    parameter dist.
    function d {parameter t. return dist-(PositionAt(Ship,t)-PositionAt(Target,t)):Mag.}
}

function findClosestApproach {
    local d is 0.
    local t is 0.
    set d to Abs( (PositionAt(Ship,t)-PositionAt(Target,t)):Mag ).
    return 0.
}

@hvacengi
Copy link
Member

hvacengi commented Dec 28, 2017

Apologies, this issue got lost in the holiday shuffle. I think that this is probably related to #2204 (which was posted after yours). As you correctly found, the local keyword is the key. The new version apparently introduced a bug where nested functions are not properly being compiled with local scope and instead are being defined in global scope.

This also points out however that we appear to have an issue in the error reporting code, and that we need to make the error more clear.

Unfortunately your two uploaded files appear to have expired, at least they are returning only the text {"success":false,"error":404,"message":"Not Found"} for me.

@hvacengi hvacengi 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 28, 2017
@Dunbaratu
Copy link
Member

@pellinor0 - I was trying to re-create this problem to test it, and I cannot find your code examples. The links you provided in the original post lead to 404 Not Found errors.

@pellinor0
Copy link
Author

Sorry for the confusion. The second post was meant as an update of the first one, so the links are not needed anymore.

You should only need the few lines of code from the second post. I got the error just by running a file with that content. It happened once when running the compiled file, and in a game-breaking loop when running it uncompiled.

@Dunbaratu
Copy link
Member

I suspect the PR I'm making to fix issue #2204 will also fix this problem. I'll test afterward to see.

@Dunbaratu
Copy link
Member

Okay I was able to reproduce it, and verify that my fix to issue 2204 also fixes this.

But I also want to make the error message better in case it happens again - so it doesn't get stuck in a loop printing the error again and again like it did here.

The irony is that this means I have to fix the error message first as a separate PR without fixing the root problem, because once I fix this problem, the error message won't be triggered.

Dunbaratu added a commit to Dunbaratu/KOS-1 that referenced this issue Dec 29, 2017
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 added a commit to Dunbaratu/KOS-1 that referenced this issue Dec 29, 2017
Now the comment that claims function scope is
only global at file scope is actually telling the truth!

(The code contained a comment that described the correct
behaviour, but the code itself didn't actually implement
what the comment said it was doing.)
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 a pull request may close this issue.

3 participants