Fix Asyncify.handleAsync conflict with PROXY_SYNC_ASYNC#26513
Fix Asyncify.handleAsync conflict with PROXY_SYNC_ASYNC#26513thiblahute wants to merge 1 commit intoemscripten-core:mainfrom
Conversation
|
Thanks! Yes, I just saw #26482 landed ppoll support - I've rebased and dropped the ppoll commit. The remaining change here fixes a pre-existing bug in |
thiblahute
changed the title
| (end.tv_nsec - begin.tv_nsec) / 1000000; | ||
| assert(elapsed_ms >= 195); | ||
|
|
||
| printf("done\n"); |
There was a problem hiding this comment.
Do we need a new test here or can we just re-used the existing test test_poll_blocking_asyncify.c but with -sPROXY_TO_PTHREAD?
If there are things we are not covering in the existing test maybe better to add to that?
There was a problem hiding this comment.
I tried reusing test_poll_blocking_asyncify.c with -sPROXY_TO_PTHREAD but it hangs because it uses emscripten_set_timeout to schedule a write, which doesn't work when main runs on a worker thread.
The pthread variant needs actual cross thread writes to exercise the proxied poll path, so I think a separate test file is needed here. Or do you have a better idea?
There was a problem hiding this comment.
emscripten_set_timeout doesn't work when run from a pthread?
Is that because the pthread being blocked during poll rather than unwinding to the even loop? I guess the SYNC_ASYNC proxying more is taking precidence of asyncify in this case and poll is not causing and unwind?
There was a problem hiding this comment.
That is what I understand yes.
thiblahute
force-pushed
the
ppoll_proxy
branch
6 times, most recently
from
5089fe8 to
4c3d00b
Compare
| return NULL; | ||
| } | ||
|
|
||
| int main(void) { |
There was a problem hiding this comment.
This code looks a lot like the existing test_unblock_poll + write_after_sleep code in test_poll_blocking.c.
Would it be enough to run test_poll_blocking.c in PROXY_TO_PTHREAD+ASYNCIFY mode? (rather than add this new test code)
There was a problem hiding this comment.
You are right, I could reproduce the exact error using that test, I removed the file and added a test in the testsuite reusing this one, you first intuition was right, sorry for the noise.
thiblahute
force-pushed
the
ppoll_proxy
branch
2 times, most recently
from
bd9180c to
c4ceddb
Compare
When a JS library function has both __proxy:'sync' and __async:'auto', the compiler generates an Asyncify.handleAsync wrapper. When called from the PROXY_SYNC_ASYNC path on the main thread, handleAsync triggers an Asyncify unwind instead of returning a Promise, causing "rtn.then is not a function" in the proxy infrastructure. Fix by generating a PThread.currentProxiedOperationCallerThread check in handleAsyncFunction (jsifier.mjs): when in a proxied context, call the inner function directly and skip the Asyncify unwind, letting the proxy mechanism handle the async return.
| // mechanism (PROXY_SYNC_ASYNC) handles the async return itself. In that | ||
| // case, skip the Asyncify unwind and call the inner function directly so | ||
| // the proxy can use .then() on the returned Promise. | ||
| const skipHandleAsync = PTHREADS && ASYNCIFY == 1 && proxyingMode === 'sync'; |
There was a problem hiding this comment.
Can / should this also be true for ASYNCIFY == 2? I don't see any reason to call handleAsync in that case either.
| @requires_pthreads | ||
| def test_poll_blocking_asyncify_pthread(self): | ||
| # Only testing ASYNCIFY=1: JSPI's handleAsync is a plain async function | ||
| # and doesn't have this bug. Also, with_asyncify_and_jspi can't be |
There was a problem hiding this comment.
I think we should still test both versions of ASYNCIFY here.
| # skipping). | ||
| self.set_setting('ASYNCIFY') | ||
| self.do_runf('core/test_poll_blocking.c', 'done\n', | ||
| cflags=['-sPROXY_TO_PTHREAD', '-sEXIT_RUNTIME']) |
There was a problem hiding this comment.
You can put this all one one line (we allow longer-than-normal lines here in the test code).
2 participants
When in a proxied context, skip the Asyncify unwind and call
startAsync()directly, letting the proxy mechanism handle theasync return.
This already affects
__syscall_polland would also affect the new__syscall_ppoll.