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

pitrou · 2023-10-13T22:45:05Z

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

pitrou · 2023-10-13T22:46:14Z

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

bedevere-bot · 2023-10-14T07:47:10Z

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

pitrou · 2023-10-14T07:48:34Z

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

pitrou · 2023-10-14T07:49:30Z

+    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;


I filed #110863 to ease this a bit.

pitrou · 2023-10-14T07:51:40Z


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


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

pitrou · 2023-10-14T07:59:40Z

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


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

pitrou · 2023-10-14T09:57:11Z

!buildbot wasm

bedevere-bot · 2023-10-14T09:57:13Z

🤖 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

pitrou · 2023-10-14T13:02:14Z

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

bedevere-bot · 2023-10-16T21:07:50Z

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

pitrou · 2023-10-17T07:53:49Z

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 · 2023-10-25T19:00:28Z

@gpshead Are you available for a review?

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

bedevere-bot · 2023-11-04T11:20:16Z

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

pitrou · 2023-11-04T15:18:54Z

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

gpshead · 2023-11-04T23:25:52Z

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.

rafa-rrayes

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

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

This was referenced Jan 18, 2024

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

Merged

[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

Uh oh!

Uh oh!

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

pitrou commented Oct 13, 2023 •

edited by bedevere-app Bot

Loading

pitrou commented Nov 4, 2023 •

edited

Loading