-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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 the handling of runtimeKeepalivePush() and runtimeKeepalivePop() in emscripten_set_immediate_loop() #18053
Fix the handling of runtimeKeepalivePush() and runtimeKeepalivePop() in emscripten_set_immediate_loop() #18053
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A push is still needed, I think, as in the other PR? But maybe I'm mixed up. @sbc100 ?
This looks good to me. The idea is that we want to keep the runtime alive as long as there is an outstanding timeout. Here we call I won't if we can make a test case for this that fails with the old/broken code? |
Actually looking more closely I don't see a bug in the old code. The new code maybe avoids the extra Pop/Push for each iterations, but the old code seems to be perfectly correct. I do see one difference in that, under the old code, the value of the keepalive counter will be zero while the callback itself is run . This means that if the user called With the new code the loop is still considered alive for the duration of the callback, so calls to I think perhaps locally the new code make more sense, since it requires and explicit cancellation of the loop to exit the runtime. |
But there is no push in the new code (the push is deleted, and the pop is moved around). Am I reading it wrong? |
The push is below on line 106. This push now remains active until one of the callbacks returns 0, which is this is the desired effect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sorry, I missed it was out of the visible diff...
…in emscripten_set_immediate_loop() to be watertight.
58b35df
to
68eb026
Compare
Fix the handling of runtimeKeepalivePush() and runtimeKeepalivePop() in emscripten_set_immediate_loop() to be watertight.