Proxy abort back to main thread - #14669
Conversation
| abort__sig: 'v', | ||
| #if !MINIMAL_RUNTIME | ||
| abort__proxy: 'sync', | ||
| #endif |
There was a problem hiding this comment.
I think it's still the right thing to proxy to the main thread in the minimal runtime, even if what runs in _abort() is different?
There was a problem hiding this comment.
@juj specifically suggested not doing that.
There was a problem hiding this comment.
I guess it doesn't make much difference, wither way MINIMAL_RUNTIME just throws here.. and doesn't do stuff like shut down all the other threads.
There was a problem hiding this comment.
The comment there is about not having onAbort, which is true. But still, it seems like a good idea to abort on the main thread if we abort at all? Anyhow, how about adding a TODO comment for minimal runtime, if we aren't sure.
There was a problem hiding this comment.
OK I agree we might as well proxy it for MINIMAL_RUNTIME too. The only reason not too would be if it increases code size for all threaded programs.
| // and this function _abort(). Remove one of these, importing two functions for the same purpose is wasteful. | ||
| abort__sig: 'v', | ||
| #if !MINIMAL_RUNTIME | ||
| abort__proxy: 'sync', |
There was a problem hiding this comment.
| abort__proxy: 'sync', | |
| // Proxy synchronously, which will also have the effect of halting the pthread. | |
| // (Async proxying would let the pthread continue until the main thread does | |
| // something.) | |
| abort__proxy: 'sync', |
There was a problem hiding this comment.
I'm not sure why that comment would be needed here by not anywhere else where we use abort__proxy. Its seem clear why we would want proxy this and all syscalls as "sync". In fact there is no current usage of "async" in the library code. Given that, I'm not sure why we even have that as an option for library code
There was a problem hiding this comment.
Interesting that we have no async proxying in library*.js, I thought we did... maybe it was removed.
While we do still support async proxying, though, I think the comment is important. I spent some time here puzzling out why this PR was valid, as it seemed to me like it would halt the main thread but not this thread. Then I realized the sync proxying replaces an abort on this thread with a hang. That seems good enough, but I think clarifying that is good, at least for me...
There was a problem hiding this comment.
The other reason this works is that, when abort it called on the main thread it will terminate all the worker threads (including this one). At least for normal runtime (I think MINIMAL_RUNTIME has a bug where PThread.terminateAllThreads is missing from exit/abort).
There was a problem hiding this comment.
I think MINIMAL_RUNTIME has a bug where PThread.terminateAllThreads is missing from exit/abort
I think PR #14481 will resolve that (it ensures that the PThread.terminateAllThreads() call is also added in postamble_minimal.js).
There was a problem hiding this comment.
@sbc100 True, but terminating the worker threads is not immediate, both because of the time it takes to proxy to the main thread (which is fire-and-forget if async), and also, there is no guarantee on when the browser actually terminates threads when requested to. So async proxying would give this thread time to continue forward for an unspecified time, which could be really weird.
There was a problem hiding this comment.
Yes clearly async proxying would make no sense at all here. (The same is true for all syscall though.. they are expected to by synchronous and would be completely broken in async mode).
There was a problem hiding this comment.
True. Outside of syscalls though we do use async proxying for several things, like WebGL. It's just by chance that none of that is in library*.js files.
| // TODO: There are currently two abort() functions that get imported to asm module scope: the built-in runtime function abort(), | ||
| // and this function _abort(). Remove one of these, importing two functions for the same purpose is wasteful. | ||
| abort__sig: 'v', | ||
| #if !MINIMAL_RUNTIME |
There was a problem hiding this comment.
| #if !MINIMAL_RUNTIME | |
| #if !MINIMAL_RUNTIME // TODO: Should minimal runtime also proxy, even if it has no onAbort()? |
This allows the `Module.onAbort` handler to be called. Fixes: #11345
It turns out proxying abort has several issues and doesn't really solve any real problems. The two main issues are: 1. If we do stuff like proxing during the call to abort it can allocate stack and heap space which might not be possible by the time abort is called. 2. All the other calls to abort that don't go via the library function (calls to abort directly from JS) are not proxied meaning we get inconsisten proxying. I added this proxying just a few week ago in #14669 but it was unnessary.
It turns out proxying abort has several issues and doesn't really solve any real problems. The two main issues are: 1. If we do stuff like proxing during the call to abort it can allocate stack and heap space which might not be possible by the time abort is called. 2. All the other calls to abort that don't go via the library function (calls to abort directly from JS) are not proxied meaning we get inconsisten proxying. I added this proxying just a few week ago in #14669 but it was unnessary.
3 participants
This allows the
Module.onAborthandler to be called.Fixes: #11345