GH-110829: Ensure Thread.join() joins the OS thread by pitrou · Pull Request #110848 · python/cpython · GitHub

Issue: threading Thread.join should call the OS join API #110829

@gpshead This should work at least on Linux, hopefully on Windows if I'm lucky.

🤖 New build scheduled with the buildbot fleet by @pitrou for commit 6c594ce 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@zooba I think my use of the Windows APIs is correct here, but you might want to take a look.

I filed #110863 to ease this a bit.

Note the changes in this file are probably not necessary, it's just that without them the threads emulation of processes takes a very long time to test. The reason is simple and not related to this PR: while you can terminate a process early, you cannot do that on a thread, so joining a sleeping thread has to wait for the sleep to finish.

We can use a shorter wait time rather than skip the test entirely: #114186

We might also want to emit a ResourceWarning here, what do you think @vstinner ?

Thinking about it, I'd rather defer this to a later PR (if at all).

!buildbot wasm

🤖 New build scheduled with the buildbot fleet by @pitrou for commit 26e868c 🤖

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-emscripten node (pthreads) PR
wasm32-emscripten node (dynamic linking) PR
wasm32-wasi PR

Most buildbots are green and the few failures look unrelated:
https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F110848%2Fmerge

🤖 New build scheduled with the buildbot fleet by @pitrou for commit e2f440f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

I formally announce that there are no buildbot failures :-)

I realize that this PR lacks some docs for the _thread additions, but before that I would like someone to vet the overall approach. @gpshead Are you available for a review?

@gpshead Are you available for a review?

I'll take a look this week, I was on vacation.

🤖 New build scheduled with the buildbot fleet by @pitrou for commit 5cf2684 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

In an ironic (and depressing) twist, it seems that even after pthread_join returned successfully, the given thread can still appear in /proc/self/task. Forking should be safe in such a situation, but our fork() wrapper prints a warning nevertheless....

doh! I guess I can understand why, the thread won't be executing anymore so join succeeded but the in kernel data structure cleanup will take its own sweet time. :/

I still think it is a valuable addition that we wait for the OS join to happen (thanks for all this work, reviews & merge) but I understand it is annoying given one of the motivating reasons.

…rks on threadpools Threads can't be forced to terminate (without potentially corrupting too much state), so the expected behaviour of `ThreadPool.terminate` is to wait for the currently executing tasks to finish. The entire test was skipped in pythonGH-110848 (0e9c364). Instead of skipping it entirely, we should ensure the API eventually succeeds: use a shorter timeout. For the record: on my machine, when the test is un-skipped, the task manages to start in about 1.5% cases.

…dpools (#114186) gh-113205: test_multiprocessing.test_terminate: Test the API works on threadpools Threads can't be forced to terminate (without potentially corrupting too much state), so the expected behaviour of `ThreadPool.terminate` is to wait for the currently executing tasks to finish. The entire test was skipped in GH-110848 (0e9c364). Instead of skipping it entirely, we should ensure the API eventually succeeds: use a shorter timeout. For the record: on my machine, when the test is un-skipped, the task manages to start in about 1.5% cases.

… threadpools (pythonGH-114186) pythongh-113205: test_multiprocessing.test_terminate: Test the API works on threadpools Threads can't be forced to terminate (without potentially corrupting too much state), so the expected behaviour of `ThreadPool.terminate` is to wait for the currently executing tasks to finish. The entire test was skipped in pythonGH-110848 (0e9c364). Instead of skipping it entirely, we should ensure the API eventually succeeds: use a shorter timeout. For the record: on my machine, when the test is un-skipped, the task manages to start in about 1.5% cases. (cherry picked from commit c1db960) Co-authored-by: Petr Viktorin <encukou@gmail.com>

… threadpools (python#114186) pythongh-113205: test_multiprocessing.test_terminate: Test the API works on threadpools Threads can't be forced to terminate (without potentially corrupting too much state), so the expected behaviour of `ThreadPool.terminate` is to wait for the currently executing tasks to finish. The entire test was skipped in pythonGH-110848 (0e9c364). Instead of skipping it entirely, we should ensure the API eventually succeeds: use a shorter timeout. For the record: on my machine, when the test is un-skipped, the task manages to start in about 1.5% cases.

) Joining a thread now ensures the underlying OS thread has exited. This is required for safer fork() in multi-threaded processes. --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>

… threadpools (python#114186) pythongh-113205: test_multiprocessing.test_terminate: Test the API works on threadpools Threads can't be forced to terminate (without potentially corrupting too much state), so the expected behaviour of `ThreadPool.terminate` is to wait for the currently executing tasks to finish. The entire test was skipped in pythonGH-110848 (0e9c364). Instead of skipping it entirely, we should ensure the API eventually succeeds: use a shorter timeout. For the record: on my machine, when the test is un-skipped, the task manages to start in about 1.5% cases.

) Joining a thread now ensures the underlying OS thread has exited. This is required for safer fork() in multi-threaded processes. --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>

… threadpools (python#114186) pythongh-113205: test_multiprocessing.test_terminate: Test the API works on threadpools Threads can't be forced to terminate (without potentially corrupting too much state), so the expected behaviour of `ThreadPool.terminate` is to wait for the currently executing tasks to finish. The entire test was skipped in pythonGH-110848 (0e9c364). Instead of skipping it entirely, we should ensure the API eventually succeeds: use a shorter timeout. For the record: on my machine, when the test is un-skipped, the task manages to start in about 1.5% cases.

Nice

bedevere-app Bot mentioned this pull request Oct 13, 2023

threading Thread.join should call the OS join API #110829

Closed

pitrou force-pushed the gh-110829-join branch 2 times, most recently from ef411ef to 41b9cba Compare October 13, 2023 23:23

pitrou added type-feature A feature request or enhancement 🔨 test-with-buildbots Test PR w/ buildbots; report in status section interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Oct 14, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 14, 2023

pitrou marked this pull request as ready for review October 14, 2023 07:47

bedevere-app Bot added the awaiting core review label Oct 14, 2023

pitrou requested a review from gpshead October 14, 2023 07:47

pitrou commented Oct 14, 2023

View reviewed changes

vstinner reviewed Oct 14, 2023

View reviewed changes

Comment thread Include/pythread.h Outdated

vstinner mentioned this pull request Oct 14, 2023

Explain why the current state of the API is inconsistent capi-workgroup/api-evolution#30

Open

pitrou mentioned this pull request Oct 15, 2023

GH-109978: Allow multiprocessing finalizers to run on a separate thread #110510

Draft

zooba reviewed Oct 16, 2023

View reviewed changes

Comment thread Python/thread_nt.h

pitrou force-pushed the gh-110829-join branch from 26e868c to e2f440f Compare October 16, 2023 21:01

pitrou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 16, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 16, 2023

gpshead self-assigned this Oct 25, 2023

pitrou added 2 commits November 4, 2023 11:48

Introduce PyThread_ident_t, PyThread_handle_t

420ece6

Address compiler warnings

96849d1

pitrou force-pushed the gh-110829-join branch from 2546653 to 96849d1 Compare November 4, 2023 11:05

Fix merge error

5cf2684

pitrou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 4, 2023

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 4, 2023

📜🤖 Added by blurb_it.

fa11244

pitrou enabled auto-merge (squash) November 4, 2023 13:46

pitrou merged commit 0e9c364 into python:main Nov 4, 2023

bedevere-app Bot removed the awaiting merge label Nov 4, 2023

pitrou deleted the gh-110829-join branch November 4, 2023 14:14

encukou mentioned this pull request Jan 17, 2024

gh-113205: test_multiprocessing.test_terminate: Test the API on threadpools #114186

Merged

miss-islington mentioned this pull request Jan 18, 2024

[3.12] gh-113205: test_multiprocessing.test_terminate: Test the API on threadpools (GH-114186) #114222

Merged

miss-islington mentioned this pull request Jan 18, 2024

[3.11] gh-113205: test_multiprocessing.test_terminate: Test the API on threadpools (GH-114186) #114223

Merged

ericsnowcurrently mentioned this pull request Mar 15, 2024

gh-114271: Fix race in Thread.join() #114839

Merged

This was referenced Feb 17, 2025

Active thread list may be inaccurate due to data type mismatch #130115

Closed

gh-130115: fix thread identifiers for 32-bit musl #130391

Merged

rafa-rrayes reviewed Mar 18, 2025

View reviewed changes

+                  unsigned long long ull_handle = PyLong_AsUnsignedLongLong(handle_obj);
+                  if ((*ident == (unsigned long) -1 || ull_handle == (unsigned long long) -1)
+                      && PyErr_Occurred()) {
+                      // This should not occur as we control the contents of state->joinable_dict
+                      return -1;
+                  }
+                  *handle = (Py_uintptr_t) ull_handle;

                   def test_terminate(self):
+                      if self.TYPE == 'threads':
+                          self.skipTest("Threads cannot be terminated")

+                              } else {
+                                  PyThread_detach_thread((Py_uintptr_t) handle);
+                              }
+                          }

GH-110829: Ensure Thread.join() joins the OS thread by pitrou · Pull Request #110848 · python/cpython · GitHub

Uh oh!

Uh oh!

GH-110829: Ensure Thread.join() joins the OS thread - #110848

GH-110829: Ensure Thread.join() joins the OS thread#110848pitrou merged 19 commits intopython:mainpython/cpython:mainfrom pitrou:gh-110829-joinpitrou/cpython:gh-110829-joinCopy head branch name to clipboard

Conversation

pitrou commented Oct 13, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Oct 13, 2023

Uh oh!

bedevere-bot commented Oct 14, 2023

Uh oh!

pitrou commented Oct 14, 2023

Uh oh!

pitrou Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

encukou Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Oct 14, 2023

Uh oh!

bedevere-bot commented Oct 14, 2023

Uh oh!

pitrou commented Oct 14, 2023

Uh oh!

Uh oh!

bedevere-bot commented Oct 16, 2023

Uh oh!

pitrou commented Oct 17, 2023

Uh oh!

gpshead commented Oct 25, 2023

Uh oh!

bedevere-bot commented Nov 4, 2023

Uh oh!

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

Uh oh!

Uh oh!

gpshead commented Nov 4, 2023

Uh oh!

rafa-rrayes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

GH-110829: Ensure Thread.join() joins the OS thread#110848
pitrou merged 19 commits into
python:mainfrom
pitrou:gh-110829-join

pitrou commented Oct 13, 2023 •
edited by bedevere-app Bot
Loading

pitrou commented Nov 4, 2023 •
edited
Loading