8000 gh-109746: Make `_thread.start_new_thread` delete state of new thread on its startup failure by chgnrdv · Pull Request #109761 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-109746: Make _thread.start_new_thread delete state of new thread on its startup failure #109761

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
Merged
merged 9 commits into from
Nov 22, 2024
Merged

gh-109746: Make _thread.start_new_thread delete state of new thread on its startup failure #109761

merged 9 commits into from
Nov 22, 2024

Conversation

chgnrdv
Copy link
Contributor
@chgnrdv chgnrdv commented Sep 22, 2023 •
edited by bedevere-app bot
Loading

Fixes #109746.

If Python fails to start newly created thread due to failure of underlying PyThread_start_new_thread() call, its state should be removed from interpreter' thread states list to avoid its double cleanup.

  • make PyThreadState_Delete accept unbound thread state
  • in thread_PyThread_start_new_thread, delete state of new thread from interpreter if PyThread_start_new_thread call fails

@chgnrdv
Delete state of new thread on its startup failure
788b0b6 
If Python fails to start newly created thread
due to failure of underlying PyThread_start_new_thread() call,
its state should be removed from interpreter' thread states list
to avoid its double cleanup

* make 'PyThreadState_Delete' accept unbound thread state
* in 'thread_PyThread_start_new_thread', delete state of new thread from interpreter if 'PyThread_start_new_thread' call fails
@chgnrdv chgnrdv changed the title Make _thread.start_new_thread delete state of new thread on its startup failure gh-109746: Make _thread.start_new_thread delete state of new thread on its startup failure Sep 22, 2023
@bedevere-app bedevere-app bot mentioned this pull request Sep 22, 2023
Closed
serhiy-storchaka
serhiy-storchaka reviewed Jan 18, 2024
View reviewed changes
@serhiy-storchaka
Copy link
Member

The test did not fail on non-patched code, failed on Windows, and hanged on macOS. I rewrote it. According to a comment in a similar test, setrlimit() does not work on macOS. Will see if it works for this test.

@serhiy-storchaka
Copy link
Member

!buildbot bsd

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 1f3ecd9 🤖

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

The builders matched are:

  • AMD64 FreeBSD14 PR
  • AMD64 FreeBSD15 PR
  • AMD64 FreeBSD PR

@devdanzin
Copy link
Contributor

Applying a version of the fix in this PR solves an issue I had where all processes with large thread counts would abort at finalization (particularly annoying when you're trying to fuzz CPython).

@serhiy-storchaka
Copy link
Member

!buildbot bsd

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 185d5a6 🤖

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

The builders matched are:

  • AMD64 FreeBSD Refleaks PR
  • AMD64 FreeBSD14 PR
  • AMD64 FreeBSD15 PR
  • AMD64 FreeBSD PR

@serhiy-storchaka
Copy link
Member

!buildbot bsd

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 55a8237 🤖

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

The builders matched are:

  • AMD64 FreeBSD Refleaks PR
  • AMD64 FreeBSD14 PR
  • AMD64 FreeBSD15 PR
  • AMD64 FreeBSD PR

@serhiy-storchaka serhiy-storchaka merged commit ca3ea9a into python:main Nov 22, 2024
42 checks passed
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.13 bugs and security fixes label Nov 22, 2024
@miss-islington-app
Copy link

Thanks @chgnrdv for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington-app
Copy link

Sorry, @chgnrdv and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker ca3ea9ad05c3d876a58463595e5b4228fda06936 3.13

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Nov 22, 2024
@chgnrdv @serhiy-storchaka
[3.13] pythongh-109746: Make _thread.start_new_thread delete state of…
66444b0 
… new thread on its startup failure (pythonGH-109761)

If Python fails to start newly created thread
due to failure of underlying PyThread_start_new_thread() call,
its state should be removed from interpreter' thread states list
to avoid its double cleanup.

(cherry picked from commit ca3ea9a)

Co-authored-by: Radislav Chugunov <52372310+chgnrdv@users.noreply.github.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link
bedevere-app bot commented Nov 22, 2024

GH-127171 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Nov 22, 2024
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Nov 22, 2024
@chgnrdv @serhiy-storchaka
[3.12] pythongh-109746: Make _thread.start_new_thread delete state of…
dc67cfb 
… new thread on its startup failure (pythonGH-109761)

If Python fails to start newly created thread
due to failure of underlying PyThread_start_new_thread() call,
its state should be removed from interpreter' thread states list
to avoid its double cleanup.

(cherry picked from commit ca3ea9a)

Co-authored-by: Radislav Chugunov <52372310+chgnrdv@users.noreply.github.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link
bedevere-app bot commented Nov 22, 2024

GH-127173 is a backport of this pull request to the 3.12 branch.

serhiy-storchaka added a commit that referenced this pull request Nov 22, 2024
@serhiy-storchaka @chgnrdv
[3.13] gh-109746: Make _thread.start_new_thread delete state of new t…
75ef92d 
…hread on its startup failure (GH-109761) (GH-127171)

If Python fails to start newly created thread
due to failure of underlying PyThread_start_new_thread() call,
its state should be removed from interpreter' thread states list
to avoid its double cleanup.

(cherry picked from commit ca3ea9a)

Co-authored-by: Radislav Chugunov <52372310+chgnrdv@users.noreply.github.com>
serhiy-storchaka added a commit that referenced this pull request Nov 22, 2024
@serhiy-storchaka @chgnrdv
[3.12] gh-109746: Make _thread.start_new_thread delete state of new t…
f1e7424 
…hread on its startup failure (GH-109761) (GH-127173)

If Python fails to start newly created thread
due to failure of underlying PyThread_start_new_thread() call,
its state should be removed from interpreter' thread states list
to avoid its double cleanup.

(cherry picked f
8000
rom commit ca3ea9a)

Co-authored-by: Radislav Chugunov <52372310+chgnrdv@users.noreply.github.com>
@vstinner
Copy link
Member

!buildbot bsd

The test is unstable on macOS and FreeBSD: it fails randomly. Examples:

Example of failure on FreeBSD:

FAIL: test_start_new_thread_failed (test.test_threading.ThreadTests.test_start_new_thread_failed)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/buildbot/buildarea/3.x.ware-freebsd/build/Lib/test/test_threading.py", line 1205, in test_start_new_thread_failed
    self.assertEqual(out, b'ok')
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^
AssertionError: b"skipshouldn't be printed" != b'ok'

@vstinner
Copy link
Member

On Linux, RLIMIT_NPROC is the maximum number of threads. On FreeBSD, RLIMIT_NPROC is the maximum number of processes.

@vstinner vstinner mentioned this pull request Nov 26, 2024
Merged
@vstinner
Copy link
Member

I suggest to make the test specific to Linux, or use another approach than RLIMIT_NPROC.

@serhiy-storchaka
Copy link
Member
6D40

Ah, there is a race condition here. Should be fixed by #127299.

ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@chgnrdv @serhiy-storchaka @ebonnal
pythongh-109746: Make _thread.start_new_thread delete state of new th…
3c8fce6 
…read on its startup failure (pythonGH-109761)

If Python fails to start newly created thread
due to failure of underlying PyThread_start_new_thread() call,
its state should be removed from interpreter' thread states list
to avoid its double cleanup.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Assignees

@serhiy-storchaka serhiy-storchaka

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash at finalization after fail to start new thread
5 participants
@chgnrdv @serhiy-storchaka @bedevere-bot @devdanzin @vstinner
0