Fix glfw usage of callUserCallback #17441
Conversation
sbc100
force-pushed
the
simplify_callUserCallback
branch
from
e5cba5a to
584a0bf
Compare
sbc100
force-pushed
the
fix_glfw
branch
2 times, most recently
from
f6f23dd to
fda49b7
Compare
src/library.js
Outdated
| #if ASSERTIONS | ||
| assert(!inUserCode, 'callUserCallback called when already running user code'); | ||
| inUserCode = true; | ||
| #endif |
There was a problem hiding this comment.
The idea is that callUserCallback is only ever called from the event loop when entering user code.
This is important because callUserCallback take care of shutting down the runtime after the user code is done. If we allow callUserCallback to be called while we are already inside of user code then it could exit while running that user code.
Sadly, my initial is of doing runtimeKeepalivePush() before calling main doesn't work because we actually do want to be able to exit the runtime (i.e. call exit()) when running user code. runtimeKeepalivePush() means that something other than the current callstack is keeping the runtime alive. So keepRuntimeAlive() need to return false during main() (or during any callback).
Another way of putting it: callUserCallback is assuming that when the user code is done it is returning to the event loop (and its safe moment to shut down the runtime) .
There was a problem hiding this comment.
Is that behavior of callUserCallback documented anywhere? I had never thought of it like that, fwiw... I thought it could be used also not from the event loop, like directly in running code, and I think we have such uses of it?
There was a problem hiding this comment.
If we did have such uses, then they would show up as test failures here, right?
If you are already in a user stack then you can just invoke the callback yourself.. what would be the point of using callUserCallback in that case? You wouldn't want it to do any of the things it normally does... which is specifically why @dschuff added the, now-removed, syncronous argument.. which bypassed what calllUserCallback was actually doing.
Anyone assuming they can use callUserCallback when they are already in a user stack would already be broken, as in this glfw bug.
There was a problem hiding this comment.
The first line of the docs seem pretty clear to me: Used to call user callbacks from the embedder / event loop.
If you are not coming from the event loop you simply don't need to call this function.
This new assertion just helps such users know that they are making a mistake.
There was a problem hiding this comment.
How about if I made this into a warnOnce rather than an assertion? I think that might be a better way to go.
sbc100
force-pushed
the
fix_glfw
branch
2 times, most recently
from
a5aace4 to
7f9ec3b
Compare
Detect cases where `callUserCallback` is called while already running user code. This doesn't change the behaviour, but warns about (currently) incorrect usage of `callUserCallback`. Split out from #17441 which fixes a real bug in glfw where `callUserCallback` could cause the runtime to exit mid-program. This bug only effects programs that are built with `EXIT_RUNTIME` but its serious enough we probably want to look at more robust solution. For now, issuing a warning seems like a good first step.
Detect cases where `callUserCallback` is called while already running user code. This doesn't change the behaviour, but warns about (currently) incorrect usage of `callUserCallback`. Split out from #17441 which fixes a real bug in glfw where `callUserCallback` could cause the runtime to exit mid-program. This bug only effects programs that are built with `EXIT_RUNTIME` but its serious enough we probably want to look at more robust solution. For now, issuing a warning seems like a good first step.
Detect cases where `callUserCallback` is called while already running user code. This doesn't change the behaviour, but warns about (currently) incorrect usage of `callUserCallback`. Split out from #17441 which fixes a real bug in glfw where `callUserCallback` could cause the runtime to exit mid-program. This bug only effects programs that are built with `EXIT_RUNTIME` but its serious enough we probably want to look at more robust solution. For now, issuing a warning seems like a good first step.
Detect cases where `callUserCallback` is called while already running user code. This doesn't change the behaviour, but warns about (currently) incorrect usage of `callUserCallback`. Split out from #17441 which fixes a real bug in glfw where `callUserCallback` could cause the runtime to exit mid-program. This bug only effects programs that are built with `EXIT_RUNTIME` but its serious enough we probably want to look at more robust solution. For now, issuing a warning seems like a good first step.
sbc100
changed the title
|
I reverted all the code to check for misused of callUserCallback. We can leave futher discussion of that to #17476. That should make this change a lot less controversial as it simply fixes a bug. |
Detect cases where `callUserCallback` is called while already running user code. This doesn't change the behaviour, but warns about (currently) incorrect usage of `callUserCallback`. Split out from #17441 which fixes a real bug in glfw where `callUserCallback` could cause the runtime to exit mid-program. This bug only effects programs that are built with `EXIT_RUNTIME` but its serious enough we probably want to look at more robust solution. For now, issuing a warning seems like a good first step.
Supersedes #17438 and #17376.