gh-111569: Fix critical sections test on WebAssembly #111897
Conversation
This adds a macro `Py_CAN_START_THREADS` that corresponds to the Python function `test.support.threading_helper.can_start_thread()`. WASI and some Emscripten builds do not have a working pthread implementation. This macro is used to guard the critical sections C API tests that require a working threads implementation.
|
!buildbot wasm |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit aef9180 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
|
!buildbot wasm |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit aef9180 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
Include/pyport.h
Outdated
| #if !defined(__wasi__) && (!defined(__EMSCRIPTEN__) || defined(__EMSCRIPTEN_PTHREADS__)) | ||
| # define Py_CAN_START_THREADS 1 | ||
| #endif |
There was a problem hiding this comment.
Unfortunately it's trickier than that because you can do a WASI build with experimental threading support (the "experimental" bit is on the WASI side, not our side); see the --with-wasm-pthreads for turning the support on.
Lines 2123 to 2127 in 6f09f69
There was a problem hiding this comment.
@brettcannon, do you think the general approach here is a reasonable? Previously, I had been just checking for thread support on the Python side, but that's a bit awkward for these C API tests.
I'm a bit confused by your comment that you can do a WASI build with threading support. I see that there is Emscripten support for pthreads. That's already handled by the __EMSCRIPTEN_PTHREADS__ check on line 476.
The Python equivalent assumes that WASI never supports threads:
cpython/Lib/test/support/threading_helper.py
Lines 232 to 233 in 6f09f69
I see that there is a WASI proposal for threads (https://github.com/WebAssembly/wasi-threads), but that looks like it's still marked "phase 1".
There was a problem hiding this comment.
The Python equivalent assumes that WASI never supports threads
The threading support isn't well-tested because I haven't set up a buildbot yet and made it priority to more thoroughly test it. 😅
I see that there is a WASI proposal for threads (https://github.com/WebAssembly/wasi-threads), but that looks like it's still marked "phase 1".
Yep, and it's staying at that phase because threading is being taken up by the WebAssembly CG at the W3C which acts as an upstream to WASI. That proposal hit I think phase 4 last month (it's definitely moving forward regardless of whether I got the number right), hence why this whole "threads in WASI" thing continues to be experimental both in WASI and for us.
I think we need to make a decision about free-threading and WASI. Do we want to try and make it work with the experimental threading support that's there, or do we say it's not worth it and wait on the more official support to land?
There was a problem hiding this comment.
I think we need to make a decision about free-threading and WASI.
My 2¢ is "no, not for now", but I also think that's only tangentially related to this issue. The test breakage is not happening in a free-threaded (--disable-gil) build. Even if we don't support the combination of --disable-gil and wasm, I'd still like a way to determine at compile time (in C code) if PyThread_start_new_thread will actually launch a thread (or is just a no-op stub).
I think we might be able to just use HAVE_PTHREAD_STUBS here instead of defining a new Py_CAN_START_THREADS.
I'm hoping to run this on the wasm buildbots, but I think the runner is backed up... probably because the test_critical_sections_threads test hangs until the test suite times out.
There was a problem hiding this comment.
👍 if the HAVE_PTHREADS_STUBS solution works.
There was a problem hiding this comment.
And I think the buildbots have calmed down: https://buildbot.python.org/all/#/builders?tags=wasm
There was a problem hiding this comment.
Hmmm... HAVE_PTHREAD_STUBS did not seem to work.
There was a problem hiding this comment.
It appears that when built without thread support WASI uses HAVE_PTHREAD_STUBS, but Emscripten stubs pthreads internally.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Ok, I think this is ready for review now. I went back to the |
|
!buildbot wasm |
|
🤖 New build scheduled with the buildbot fleet by @brettcannon for commit 2ac2321 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
|
Thanks, @colesbury ! |
…111897) This adds a macro `Py_CAN_START_THREADS` that corresponds to the Python function `test.support.threading_helper.can_start_thread()`. WASI and some Emscripten builds do not have a working pthread implementation. This macro is used to guard the critical sections C API tests that require a working threads implementation.
…111897) This adds a macro `Py_CAN_START_THREADS` that corresponds to the Python function `test.support.threading_helper.can_start_thread()`. WASI and some Emscripten builds do not have a working pthread implementation. This macro is used to guard the critical sections C API tests that require a working threads implementation.
This adds a macro
Py_CAN_START_THREADSthat corresponds to the Python function test.support.threading_helper.can_start_thread(). Some WebAssembly platforms do not have a working pthread implementation.This macro is used to guard the critical sections C API tests that require a working threads implementation.