Add more testing for pthread_kill.#26663
Conversation
| return EINVAL; | ||
| } | ||
| if (t == emscripten_main_runtime_thread_id()) { | ||
| if (sig == 0) return 0; // signal == 0 is a no-op. |
There was a problem hiding this comment.
Was this a perf optimization perhaps?
There was a problem hiding this comment.
Just simplifying the code a little.
There was a problem hiding this comment.
Oh.. also I'm removing this limitation. For some reason we were disallowing using pthread_kill to send signals to the main runtime thread.. I'm not sure why.
There was a problem hiding this comment.
Worth a changelog mention perhaps, if this is a breaking change? Though maybe not if there are hardly any users of this.
Though I would suspect this limitation was there for a reason... maybe git history helps?
There was a problem hiding this comment.
This is very niche.. and its changing from non-standard + restrictive behavior to standard + less restrictive behaviour. I tend to think that moving things towards standard behaviour and removing restrictions normally mean less reason to add ChangeLog entry?
There was a problem hiding this comment.
Looks like this was inherited from the JS version in #17041.
The JS version had "err('Main thread (id=' + ptrToString(thread) + ') cannot be killed with pthread_kill!');"
The "cannot be killed" message dates all the way back to 4f29d30 in 2015.
I imagine it was logical to thing that the main browser thread cannot be killed, but kill and also be used to send all kinds of signal. Its doesn't actaully kill anything necessarily.. confusing name. In any case raise (which is what kill really does) works fine on the main browser thread.
sbc100
force-pushed
the
pthread_kill
branch
2 times, most recently
from
969a8aa to
cde49d9
Compare
- Test using `pthread_kill` on yourself. - Test using `pthread_kill` from a background thread to the main thread.
This special handling for SIGCANCEL should not be needed. If we need to do something like `_emscripten_runtime_keepalive_clear` during pthread cancelation it would be best done when the cancellation in processed in `__testcancel`/`__cancel`. The comment and code I'm removing here was added back in emscripten-core#22467 along with testing in the form of `test/pthread/test_pthread_kill.c` and the pthread_kill tests in posixtest. I recently expanded the testing in emscripten-core#26663, and this change doesn't break any of those tests.
This special handling for `SIGCANCEL` should not be needed. If we need to do something like `_emscripten_runtime_keepalive_clear` during pthread cancelation it would be best done when the cancellation in processed in `__testcancel`/`__cancel`. The comment & code I'm removing here was added back in #22467 along with testing in the form of `test/pthread/test_pthread_kill.c` and the pthread_kill tests in posixtest. I recently expanded the testing in #26663, and this change doesn't break any of those tests.
2 participants
pthread_killon yourself.pthread_killfrom a background thread to the main thread. For some reason this was disallowed before.pthread_killalong with a test.