GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-104690 Disallow thread creation and fork at interpreter finalization by chgnrdv · Pull Request #104826 · python/cpython

Conversation

@chgnrdv

@chgnrdv chgnrdv commented May 24, 2023 •
edited by gpshead
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Fixes #104690

In the following functions, check if interpreter is finalizing and in this case raise RuntimeError with appropriate message:

  • _thread.start_new_thread
  • posix.fork
  • posix.fork1
  • posix.forkpty
  • _posixsubprocess.fork_exec when a preexec_fn is used.
  • Issue: Threads started in exit handler are still running after their thread states are destroyed #104690

Sorry, something went wrong.

Uh oh!

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

All reactions
@chgnrdv
Disallow thread creation and fork at interpreter finalization
15ecad5
in the following functions, check if interpreter is finalizing and raise `RuntimeError` with appropriate message:
* `_thread.start_new_thread`
* `posix.fork`
* `posix.fork1`
* `posix.forkpty`
* `_posixsubprocess.fork_exec`
@bedevere-bot bedevere-bot mentioned this pull request May 24, 2023
Threads started in exit handler are still running after their thread states are destroyed #104690
Closed
chgnrdv and others added 4 commits May 24, 2023 12:16
@chgnrdv
improved error messages
72095f3
@chgnrdv
added tests, changed test_threading.ThreadTests.test_fork_at_exit to …
77443a0
…fit new behaviour
@blurb-it
📜🤖 Added by blurb_it.
3134fbe
@chgnrdv
Merge branch 'main' into disallow-thread-creation-and-fork-at-interp-…
530f785
…exit
@chgnrdv
chgnrdv marked this pull request as ready for review May 24, 2023 12:46
@chgnrdv
chgnrdv requested a review from gpshead as a code owner May 24, 2023 12:46
@bedevere-bot bedevere-bot added the awaiting review label May 24, 2023
@gpshead gpshead self-assigned this May 26, 2023
gpshead
gpshead requested changes May 26, 2023
View reviewed changes
Comment thread Lib/test/test_threading.py Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Core and Builtins/2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst Resolved

Uh oh!

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

Comment thread Modules/_posixsubprocess.c Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels May 26, 2023
@bedevere-bot

bedevere-bot commented May 26, 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.

And if you don't make the requested changes, you will be put in the comfy chair!

All reactions

Sorry, something went wrong.

Uh oh!

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

@gpshead
gpshead requested a review from vstinner May 26, 2023 04:23
@gpshead gpshead added the needs backport to 3.12 only security fixes label May 26, 2023
chgnrdv added 4 commits May 26, 2023 12:07
@chgnrdv
Merge branch 'main' into disallow-thread-creation-and-fork-at-interp-…
d26b8db
…exit
@chgnrdv
Merge branch 'disallow-thread-creation-and-fork-at-interp-exit' of gi…
1c51471
…thub.com:chgnrdv/cpython into disallow-thread-creation-and-fork-at-interp-exit
@chgnrdv
Suggested changes:
bbc8ca4
* removed excess test case from `test_threading`
* changed NEWS entry to not mention internal function
* allowed `subprocess.Popen` at shutdown if `preexec_fn` is `None`
@chgnrdv
fixed test case in test_subprocess
bcbcb75
@chgnrdv

chgnrdv commented May 26, 2023

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels May 26, 2023
@bedevere-bot

bedevere-bot commented May 26, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot
bedevere-bot requested a review from gpshead May 26, 2023 09:36
@gpshead

gpshead commented May 26, 2023

Copy link
Copy Markdown
Member

FYI - if you want an example of real world code that was spawning threads from atexit (I'm not claiming that this was a good idea... just that it inadvertently happened): See the example given in #86813.

chgnrdv reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

@gpshead
Reword and clarify the NEWS entry.
0926f6f
gpshead
gpshead approved these changes May 26, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels May 26, 2023
@gpshead
gpshead enabled auto-merge (squash) June 3, 2023 15:45
@gpshead
gpshead merged commit ce558e6 into python:main Jun 4, 2023
@miss-islington

miss-islington commented Jun 4, 2023

Copy link
Copy Markdown
Contributor

Thanks @chgnrdv for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting merge label Jun 4, 2023
@miss-islington

miss-islington commented Jun 4, 2023

Copy link
Copy Markdown
Contributor

Sorry @chgnrdv and @gpshead, I had trouble checking out the 3.12 backport branch.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker ce558e69d4087dd3653207de78345fbb8a2c7835 3.12

All reactions

Sorry, something went wrong.

Uh oh!

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

@gpshead gpshead added needs backport to 3.12 only security fixes and removed needs backport to 3.12 only security fixes labels Jun 4, 2023
@miss-islington

miss-islington commented Jun 4, 2023

Copy link
Copy Markdown
Contributor

Thanks @chgnrdv for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 4, 2023
@chgnrdv @blurb-it
@gpshead @miss-islington
pythongh-104690 Disallow thread creation and fork at interpreter fina…
b31e6ae
…lization (pythonGH-104826)

Disallow thread creation and fork at interpreter finalization.

in the following functions, check if interpreter is finalizing and raise `RuntimeError` with appropriate message:
* `_thread.start_new_thread` and thus `threading`
* `posix.fork`
* `posix.fork1`
* `posix.forkpty`
* `_posixsubprocess.fork_exec` when a `preexec_fn=` is supplied.

---------

(cherry picked from commit ce558e6)

Co-authored-by: chgnrdv <[email protected]>
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Co-authored-by: Gregory P. Smith <[email protected]>
@bedevere-bot

bedevere-bot commented Jun 4, 2023

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 4, 2023
gpshead added a commit that referenced this pull request Jun 4, 2023
@miss-islington @chgnrdv @gpshead
[3.12] gh-104690 Disallow thread creation and fork at interpreter fin…
c7a9d96
…alization (GH-104826) (#105277)

gh-104690 Disallow thread creation and fork at interpreter finalization (GH-104826)

Disallow thread creation and fork at interpreter finalization.

in the following functions, check if interpreter is finalizing and raise `RuntimeError` with appropriate message:
* `_thread.start_new_thread` and thus `threading`
* `posix.fork`
* `posix.fork1`
* `posix.forkpty`
* `_posixsubprocess.fork_exec` when a `preexec_fn=` is supplied.

---------

(cherry picked from commit ce558e6)

Co-authored-by: chgnrdv <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
@erlend-aasland erlend-aasland mentioned this pull request Jun 6, 2023
3.12 backport gh 105236 #105358
Closed
AdamWill added a commit to AdamWill/anaconda that referenced this pull request Jul 5, 2023
@AdamWill
Handle subprocess disallowing preexec during shutdown
f8311b7
In Python 3.12, you cannot pass a preexec_fn to subprocess.Popen
during interpreter shutdown:

python/cpython#104826

This avoids the problem by giving startProgram an arg to not
use a preexec_fn, and passing that all the way from anaconda's
exitHandler through execWithRedirect and _run_program.

Signed-off-by: Adam Williamson <[email protected]>
@AdamWill AdamWill mentioned this pull request Jul 5, 2023
Handle subprocess disallowing preexec during shutdown rhinstaller/anaconda#4879
Merged
AdamWill added a commit to AdamWill/anaconda that referenced this pull request Jul 5, 2023
@AdamWill
Handle subprocess disallowing preexec during shutdown
6f14a16
In Python 3.12, you cannot pass a preexec_fn to subprocess.Popen
during interpreter shutdown:

python/cpython#104826

This avoids the problem by giving startProgram an arg to not
use a preexec_fn, and passing that all the way from anaconda's
exitHandler through execWithRedirect and _run_program.

Signed-off-by: Adam Williamson <[email protected]>
@vstinner

vstinner commented Sep 7, 2023

Copy link
Copy Markdown
Member

This change introduced a test_concurrent_futures regression: issue #109047. Well, right now I'm not sure about the root cause analysis.

All reactions

Sorry, something went wrong.

Uh oh!

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

@mabdinur mabdinur mentioned this pull request Sep 8, 2023
chore(telemetry): ensure instrumentation telemetry is compatible with python 3.12 DataDog/dd-trace-py#6859
Merged
18 tasks done
mabdinur added a commit to DataDog/dd-trace-py that referenced this pull request Sep 8, 2023
@mabdinur @Yun-Kim
@majorgreys @P403n1x87 @emmettbutler @ZStriker19
chore(telemetry): ensure instrumentation telemetry is compatible with…
dad4d9d
… python 3.12 (#6859)

## Motivation

- Make the instrumentation telemetry client compatible with python3.12:
python/cpython#104826

## Description

 - Start telemetry worker thread as early as possible.
 - Delays sending all telemetry events until app-started is queued.
 - Refactors tests to align with this new logic.
 

## Risk 

- Telemetry events (metrics/logs/integrations) are queued as early as
possible but these events are only sent when the trace agent writer is
started. This **may** result in a memory leak if high cardinality
telemetry metrics and logs are added in the future. This is not a
concern right now.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

---------

Co-authored-by: Yun Kim <[email protected]>
Co-authored-by: Tahir H. Butt <[email protected]>
Co-authored-by: Gabriele N. Tornetta <[email protected]>
Co-authored-by: Yun Kim <[email protected]>
Co-authored-by: Emmett Butler <[email protected]>
Co-authored-by: ZStriker19 <[email protected]>
mabdinur added a commit to DataDog/dd-trace-py that referenced this pull request Sep 11, 2023
@mabdinur @Yun-Kim
@majorgreys @P403n1x87 @emmettbutler @ZStriker19
chore(telemetry): ensure instrumentation telemetry is compatible with…
9c2b6d4
… python 3.12 (#6859)

- Make the instrumentation telemetry client compatible with python3.12:
python/cpython#104826

 - Start telemetry worker thread as early as possible.
 - Delays sending all telemetry events until app-started is queued.
 - Refactors tests to align with this new logic.

- Telemetry events (metrics/logs/integrations) are queued as early as
possible but these events are only sent when the trace agent writer is
started. This **may** result in a memory leak if high cardinality
telemetry metrics and logs are added in the future. This is not a
concern right now.

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

---------

Co-authored-by: Yun Kim <[email protected]>
Co-authored-by: Tahir H. Butt <[email protected]>
Co-authored-by: Gabriele N. Tornetta <[email protected]>
Co-authored-by: Yun Kim <[email protected]>
Co-authored-by: Emmett Butler <[email protected]>
Co-authored-by: ZStriker19 <[email protected]>
majorgreys added a commit to DataDog/dd-trace-py that referenced this pull request Sep 13, 2023
@mabdinur @Yun-Kim
@majorgreys @P403n1x87 @emmettbutler @ZStriker19
chore(telemetry): ensure instrumentation telemetry is compatible with…
51f3bac
… python 3.12 (#6859)

## Motivation

- Make the instrumentation telemetry client compatible with python3.12:
python/cpython#104826

## Description

 - Start telemetry worker thread as early as possible.
 - Delays sending all telemetry events until app-started is queued.
 - Refactors tests to align with this new logic.
 

## Risk 

- Telemetry events (metrics/logs/integrations) are queued as early as
possible but these events are only sent when the trace agent writer is
started. This **may** result in a memory leak if high cardinality
telemetry metrics and logs are added in the future. This is not a
concern right now.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

---------

Co-authored-by: Yun Kim <[email protected]>
Co-authored-by: Tahir H. Butt <[email protected]>
Co-authored-by: Gabriele N. Tornetta <[email protected]>
Co-authored-by: Yun Kim <[email protected]>
Co-authored-by: Emmett Butler <[email protected]>
Co-authored-by: ZStriker19 <[email protected]>
@sentrivana sentrivana mentioned this pull request Nov 2, 2023
Officially support Python 3.12 getsentry/sentry-python#2480
Closed
4 tasks done
@pavyamsiri pavyamsiri mentioned this pull request Jan 19, 2024
RuntimeErrors upon interpreter shutdown due to garbage collection pynbody/pynbody#762
Closed
@tacaswell tacaswell mentioned this pull request Feb 9, 2024
Can not communicate with subprocess during interpreter shutdown on windows #115219
Closed
@vstinner vstinner mentioned this pull request Feb 22, 2024
3.12 regression: interpreter shouldn't shut down when there are non-daemon threads left #115533
Closed
@gvanrossum gvanrossum mentioned this pull request Feb 22, 2024
3.12 regression: "can't create new thread at interpreter shutdown" from non-daemon threads or atexit handlers #113964
Open
@robhudson robhudson mentioned this pull request Apr 3, 2024
Migrate to gunicorn + gevent mozmeao/basket#1350
Closed
ppigazzini added a commit to ppigazzini/fishtest that referenced this pull request Apr 4, 2024
@ppigazzini
Mitigate Python 3.12 shutdown RuntimeError
a24b9d4
Python 3.12 introduced a bug during the interpreter shutdown, see:
- issue python/cpython#104826
- bugfix python/cpython#117029

With Python 3.12.3, this change could potentially be reverted (after testing).
@ppigazzini ppigazzini mentioned this pull request Apr 4, 2024
Mitigate Python 3.12 Shutdown RuntimeError official-stockfish/fishtest#1925
Merged
ppigazzini added a commit to official-stockfish/fishtest that referenced this pull request Apr 4, 2024
@ppigazzini
Mitigate Python 3.12 shutdown RuntimeError
c8173d2
Python 3.12 introduced a bug during the interpreter shutdown, see:
- issue python/cpython#104826
- bugfix python/cpython#117029

With Python 3.12.3, this change could potentially be reverted (after testing).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes
@sunmy2019 sunmy2019 sunmy2019 approved these changes
@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

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

Assignees

@gpshead gpshead

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Threads started in exit handler are still running after their thread states are destroyed

Uh oh!

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

6 participants

@chgnrdv @bedevere-bot @gpshead @miss-islington @vstinner @sunmy2019
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.