Fix wrapping/modifying of single line JS library functions #20265
Conversation
sbc100
force-pushed
the
fix_js_arrow_funcs
branch
from
8696741 to
020f363
Compare
|
The tsst looks good but I'll defer to someone else to review the actual change. |
test/test_other.py
Outdated
| addToLibrary({ | ||
| foo__proxy: 'sync', | ||
| foo: () => setTimeout(() => { | ||
| console.log('should not see this'); |
There was a problem hiding this comment.
Why should this not be seen? foo is called.
There was a problem hiding this comment.
But the timeout here is 1000 seconds.. and the program exits before we even return to the event loop.
The bug here was that the outer setTimeout(() => { was being stripped by the jsifier code.. resulting in the console.log running directly rather than on a timeout.
There was a problem hiding this comment.
It's 1000 ms, not seconds, so I can see a VM stall cause it to print.
I guess I'd be ok with 1000*60*15 (15 minutes) or such, as that is the timeout of CI anyhow...
There was a problem hiding this comment.
(lgtm with that, or more edit: with a comment)
There was a problem hiding this comment.
Ah.. well in that case maybe we should make the value zero, assuming the test passes.
I removed the timeout completely and test still passes. Also added a comment.
There was a problem hiding this comment.
(Was that change pushed? I still see a timeout, and don't see a comment about the timeout duration of 1000)
There was a problem hiding this comment.
Oops, I'm switching to using my own emscripten branch as origin and it can be confusing.
|
ping.. |
sbc100
force-pushed
the
fix_js_arrow_funcs
branch
from
020f363 to
2b41ea6
Compare
|
Should be good to land now. |
| foo__proxy: 'sync', | ||
| foo: () => setTimeout(() => { | ||
| console.log('should not see this before "done"'); | ||
| }), |
There was a problem hiding this comment.
Isn't this a little racey? A timeout of 0 means it will run in the next event loop iteration, but the runtime might exit before that? That is, we might get only "main" in the output, sometimes.
There was a problem hiding this comment.
Oh, wait, I misread this, done is not from here but from below.
There was a problem hiding this comment.
I think the problem is that the main thread which calls foo could hit its even loop at print "should not see this before "done"" from the setTimeout before the secondary thread sees that foo is complete and allow itself to continue executing and print "done"
There was a problem hiding this comment.
The key here is that "foo" is proxied to the main thread.
There was a problem hiding this comment.
Ah, yes... it must be the proxying, yeah, good point.
|
This errors on Mac and Windows: Not sure why it's different on linux here on github? Is this fixed by #20303? |
|
I don't understand how it could be flakey, since "done" was printed synchronously... strange. |
Fixes: #20264