GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-111569: Fix critical sections test on WebAssembly by colesbury · Pull Request #111897 · python/cpython

Conversation

colesbury commented Nov 9, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

This adds a macro Py_CAN_START_THREADS that 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.

Issue: Implement Python Critical Sections from PEP 703 #111569

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-111569: Fix critical sections test on WebAssembly

aef9180

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.

colesbury added tests

Tests in the Lib/test dir

OS-wasi topic-free-threading labels

Nov 9, 2023

colesbury requested a review from brettcannon

November 9, 2023 15:43

bedevere-app Bot mentioned this pull request

Nov 9, 2023

Implement Python Critical Sections from PEP 703 #111569

Closed

colesbury commented Nov 9, 2023

Copy link

Copy Markdown

Contributor Author

!buildbot wasm

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot commented Nov 9, 2023

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @colesbury for commit aef9180 🤖

The command will test the builders whose names match following regular expression: wasm

The builders matched are:

wasm32-emscripten browser (dynamic linking, no tests) PR
wasm32 WASI PR
wasm32-emscripten node (dynamic linking) PR
wasm32-emscripten node (pthreads) PR
wasm32-wasi PR

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury added the skip news label

Nov 9, 2023

colesbury commented Nov 9, 2023

Copy link

Copy Markdown

Contributor Author

!buildbot wasm

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot commented Nov 9, 2023

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @colesbury for commit aef9180 🤖

The command will test the builders whose names match following regular expression: wasm

The builders matched are:

wasm32-emscripten browser (dynamic linking, no tests) PR
wasm32 WASI PR
wasm32-emscripten node (dynamic linking) PR
wasm32-emscripten node (pthreads) PR
wasm32-wasi PR

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brettcannon requested changes

Nov 9, 2023

View reviewed changes

brettcannon left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unfortunately there's more subtlety to thread support under WASI as it's possible via --with-wasm-pthreads, but it's opt-in (until threads fully land in the WebAssembly standard and support trickles down through the tooling).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Include/pyport.h Outdated

Comment on lines +476 to +478

		#if !defined(__wasi__) && (!defined(__EMSCRIPTEN__) \|\| defined(__EMSCRIPTEN_PTHREADS__))
		# define Py_CAN_START_THREADS 1
		#endif

brettcannon Nov 9, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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"])
	])

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

colesbury Nov 9, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

brettcannon Nov 9, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury reacted with thumbs up emoji

All reactions

👍 1 reaction

colesbury Nov 9, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

brettcannon Nov 9, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

brettcannon Nov 9, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

colesbury Nov 9, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

colesbury Nov 9, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added the awaiting changes label

Nov 9, 2023

bedevere-app Bot commented Nov 9, 2023

Copy link

Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Use HAVE_PTHREAD_STUBS instead

fe38ff9

colesbury marked this pull request as ready for review

November 9, 2023 19:14

bedevere-app Bot added awaiting review and removed awaiting changes labels

Nov 9, 2023

colesbury marked this pull request as draft

November 9, 2023 19:25

bedevere-app Bot removed the awaiting review label

Nov 9, 2023


          Go back to Py_CAN_START_THREADS

2ac2321

colesbury marked this pull request as ready for review

November 9, 2023 20:15

bedevere-app Bot added the awaiting review label

Nov 9, 2023

colesbury commented Nov 9, 2023

Copy link

Copy Markdown

Contributor Author

Ok, I think this is ready for review now. I went back to the Py_CAN_START_THREADS approach, but check for HAVE_PTHREAD_STUBS and the Emscripten combiation. WASI support for threads is indicated by the absence/presence of HAVE_PTHREAD_STUBS. Emscripten support for threads is indicated by __EMSCRIPTEN_PTHREADS__.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury requested a review from brettcannon

November 9, 2023 20:16

brettcannon approved these changes

Nov 9, 2023

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels

Nov 9, 2023

brettcannon commented Nov 9, 2023

Copy link

Copy Markdown

Member

!buildbot wasm

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot commented Nov 9, 2023

Copy link

Copy Markdown

🤖 New build scheduled with the buildbot fleet by @brettcannon for commit 2ac2321 🤖

The command will test the builders whose names match following regular expression: wasm

The builders matched are:

wasm32-emscripten browser (dynamic linking, no tests) PR
wasm32 WASI PR
wasm32-emscripten node (dynamic linking) PR
wasm32-emscripten node (pthreads) PR
wasm32-wasi PR

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brettcannon merged commit 289af86 into python:main

Nov 9, 2023

bedevere-app Bot removed the awaiting merge label

Nov 9, 2023

brettcannon commented Nov 9, 2023

Copy link

Copy Markdown

Member

Thanks, @colesbury !

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury deleted the critical_sections-wasm branch

November 9, 2023 23:38

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-111569: Fix critical sections test on WebAssembly (pythonGH-…

41586cd

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

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-111569: Fix critical sections test on WebAssembly (pythonGH-…

97032d5

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

Reviewers

brettcannon

brettcannon approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

OS-wasi skip news tests

Tests in the Lib/test dir

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

colesbury commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesbury commented Nov 9, 2023

Uh oh!

bedevere-bot commented Nov 9, 2023

Uh oh!

colesbury commented Nov 9, 2023

Uh oh!

bedevere-bot commented Nov 9, 2023

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Nov 9, 2023

Uh oh!

colesbury commented Nov 9, 2023

Uh oh!

brettcannon commented Nov 9, 2023

Uh oh!

bedevere-bot commented Nov 9, 2023

Uh oh!

brettcannon commented Nov 9, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

colesbury commented Nov 9, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.