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

Processes seem to be able to kill themselves #628

Closed
evancz opened this Issue May 27, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@evancz
Member

evancz commented May 27, 2016

@alphalambda made a http://sscce.org/ in which you can trigger a runtime error if you click Pause and Resume a bunch of times.

https://gist.github.com/alphalambda/448099e95858b0ffc91fd41fdb1b437a

Looking at where the error happens, it seems that the currently running process is killed while it is running. This suggests to me that the process is somehow killing itself. I don't know how that can be possible.

Update: @jagare says

there are messages for the kill process left in the workerQueue

Suggesting that kill either needs to clear the messages to that node, or that there's some check needed when you start handling a node that isn't there already.

@evancz evancz added the bug label May 27, 2016

@alphalambda

This comment has been minimized.

Show comment
Hide comment
@alphalambda

alphalambda May 30, 2016

A workaround that stops the crashes is to change line 132 in src/Native/Scheduler.js from
while (numSteps < MAX_STEPS)
into
while (numSteps < MAX_STEPS && process.root)

alphalambda commented May 30, 2016

A workaround that stops the crashes is to change line 132 in src/Native/Scheduler.js from
while (numSteps < MAX_STEPS)
into
while (numSteps < MAX_STEPS && process.root)

@jagare

This comment has been minimized.

Show comment
Hide comment
@jagare

jagare Jun 13, 2016

I don't think the process is killing itself.
From what I can see in the debugger the process is killed from here:
https://github.com/elm-lang/animation-frame/blob/1.0.0/src/AnimationFrame.elm#L84
But it is not done from the process when it is running!
In my case, adding a few logs, the process with id 840 was killed by pid 3. However there where still a message for pid 840 in the workQueue. In fact after killing pid 840, pid 0 and pid 3 processed one message each before pid 840 was to process a message after it has been killed, which results in the exception.
So it appears to me as if the problem is that the problem comes from that there are messages for the kill process left in the workerQueue. I guess one solution can be to just replace

numSteps = step(numSteps, process);
with
if (process.root) { numSteps = step(numSteps, process); }
Since we want to continue processing the rest of the messages in the workQueue.

jagare commented Jun 13, 2016

I don't think the process is killing itself.
From what I can see in the debugger the process is killed from here:
https://github.com/elm-lang/animation-frame/blob/1.0.0/src/AnimationFrame.elm#L84
But it is not done from the process when it is running!
In my case, adding a few logs, the process with id 840 was killed by pid 3. However there where still a message for pid 840 in the workQueue. In fact after killing pid 840, pid 0 and pid 3 processed one message each before pid 840 was to process a message after it has been killed, which results in the exception.
So it appears to me as if the problem is that the problem comes from that there are messages for the kill process left in the workerQueue. I guess one solution can be to just replace

numSteps = step(numSteps, process);
with
if (process.root) { numSteps = step(numSteps, process); }
Since we want to continue processing the rest of the messages in the workQueue.

@w0rm

This comment has been minimized.

Show comment
Hide comment
@w0rm

w0rm Jul 6, 2016

I just ran into the same issue when trying to stop AnimationFrame. I need this to suspend a game, so that then it can be restored by a message from a port.

UPD: currently I'm patching generated elm.js in the way suggested by @jagare and it seems to be working.

w0rm commented Jul 6, 2016

I just ran into the same issue when trying to stop AnimationFrame. I need this to suspend a game, so that then it can be restored by a message from a port.

UPD: currently I'm patching generated elm.js in the way suggested by @jagare and it seems to be working.

@w0rm w0rm referenced this issue Jul 27, 2016

Closed

Runtime exception #15

@OvermindDL1

This comment has been minimized.

Show comment
Hide comment
@OvermindDL1

OvermindDL1 Aug 3, 2016

Similar issue (though with subscriptions being turned on/off) at: elm-lang/animation-frame#7

OvermindDL1 commented Aug 3, 2016

Similar issue (though with subscriptions being turned on/off) at: elm-lang/animation-frame#7

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 4, 2016

Member

I think @jagare has the right idea for the fix. I'll look into it a bit more now.

Member

evancz commented Aug 4, 2016

I think @jagare has the right idea for the fix. I'll look into it a bit more now.

@evancz evancz closed this in 9320969 Aug 4, 2016

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 4, 2016

Member

Can people check if elm-lang@9320969 fixes it for them?

If so, I'll do a patch release. Thanks again @jagare!

Member

evancz commented Aug 4, 2016

Can people check if elm-lang@9320969 fixes it for them?

If so, I'll do a patch release. Thanks again @jagare!

@OvermindDL1

This comment has been minimized.

Show comment
Hide comment
@OvermindDL1

OvermindDL1 Aug 4, 2016

That is precisely what I've been doing to my generated javascript (prior to finding this issue, would have saved me time if I'd found it before...) and it works, and based on that processes can be killed it looks like the right thing to do too. I'd love a patch release! ^.^

OvermindDL1 commented Aug 4, 2016

That is precisely what I've been doing to my generated javascript (prior to finding this issue, would have saved me time if I'd found it before...) and it works, and based on that processes can be killed it looks like the right thing to do too. I'd love a patch release! ^.^

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 4, 2016

Member

Cool, thanks for confirming this. I'll do a bit more testing myself and then do a patch.

Member

evancz commented Aug 4, 2016

Cool, thanks for confirming this. I'll do a bit more testing myself and then do a patch.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Aug 4, 2016

Member

Okay, core 4.0.4 should be out. Thanks again to everyone who helped sort this out!

Member

evancz commented Aug 4, 2016

Okay, core 4.0.4 should be out. Thanks again to everyone who helped sort this out!

@OvermindDL1

This comment has been minimized.

Show comment
Hide comment
@OvermindDL1

OvermindDL1 Aug 4, 2016

Upgraded, tried my code at work as well as tests and all passed without me needing to mess with the output javascript. Thanks much! ^.^

OvermindDL1 commented Aug 4, 2016

Upgraded, tried my code at work as well as tests and all passed without me needing to mess with the output javascript. Thanks much! ^.^

@w0rm

This comment has been minimized.

Show comment
Hide comment
@w0rm

w0rm Aug 4, 2016

Thanks!

w0rm commented Aug 4, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment