Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-111569: Fix critical sections test on WebAssembly #111897

Merged
merged 3 commits into from
Nov 9, 2023
Merged

gh-111569: Fix critical sections test on WebAssembly #111897

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aef9180
gh-111569: Fix critical sections test on WebAssembly
colesbury Nov 9, 2023
fe38ff9
Use HAVE_PTHREAD_STUBS instead
colesbury Nov 9, 2023
2ac2321
Go back to Py_CAN_START_THREADS
colesbury Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions Include/pyport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,13 @@ extern "C" {
# define WITH_THREAD
#endif

/* Some WebAssembly platforms do not provide a working pthread implementation.
* Thread support is stubbed and any attempt to create a new thread fails.
*/
#if !defined(__wasi__) && (!defined(__EMSCRIPTEN__) || defined(__EMSCRIPTEN_PTHREADS__))
# define Py_CAN_START_THREADS 1
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cpython/configure.ac

Lines 2123 to 2127 in 6f09f69

AS_VAR_IF([enable_wasm_pthreads], [yes], [
AS_VAR_APPEND([CFLAGS_NODIST], [" -pthread"])
AS_VAR_APPEND([LDFLAGS_NODIST], [" -sUSE_PTHREADS"])
AS_VAR_APPEND([LINKFORSHARED], [" -sPROXY_TO_PTHREAD"])
])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

elif sys.platform == "wasi":
return False

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 if the HAVE_PTHREADS_STUBS solution works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think the buildbots have calmed down: https://buildbot.python.org/all/#/builders?tags=wasm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... HAVE_PTHREAD_STUBS did not seem to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that when built without thread support WASI uses HAVE_PTHREAD_STUBS, but Emscripten stubs pthreads internally.


#ifdef WITH_THREAD
# ifdef Py_BUILD_CORE
# ifdef HAVE_THREAD_LOCAL
Expand Down
4 changes: 4 additions & 0 deletions Modules/_testinternalcapi/test_critical_sections.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ thread_critical_sections(void *arg)
}
}

#ifdef Py_CAN_START_THREADS
static PyObject *
test_critical_sections_threads(PyObject *self, PyObject *Py_UNUSED(args))
{
Expand All @@ -194,12 +195,15 @@ test_critical_sections_threads(PyObject *self, PyObject *Py_UNUSED(args))
Py_DECREF(test_data.obj1);
Py_RETURN_NONE;
}
#endif

static PyMethodDef test_methods[] = {
{"test_critical_sections", test_critical_sections, METH_NOARGS},
{"test_critical_sections_nest", test_critical_sections_nest, METH_NOARGS},
{"test_critical_sections_suspend", test_critical_sections_suspend, METH_NOARGS},
#ifdef Py_CAN_START_THREADS
{"test_critical_sections_threads", test_critical_sections_threads, METH_NOARGS},
#endif
{NULL, NULL} /* sentinel */
};

Expand Down