gh-104690 Disallow thread creation and fork at interpreter finalization #104826

chgnrdv · 2023-05-24T00:25:59Z

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

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`

…fit new behaviour

…exit

Lib/test/test_threading.py

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

Modules/_posixsubprocess.c

bedevere-bot · 2023-05-26T04:23:46Z

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!

…exit

…thub.com:chgnrdv/cpython into disallow-thread-creation-and-fork-at-interp-exit

* 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 · 2023-05-26T09:36:27Z

I have made the requested changes; please review again

bedevere-bot · 2023-05-26T09:36:31Z

Thanks for making the requested changes!

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

gpshead · 2023-05-26T17:19:26Z

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.

miss-islington · 2023-06-04T04:06:49Z

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

miss-islington · 2023-06-04T04:06:51Z

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

miss-islington · 2023-06-04T04:07:50Z

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

…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 · 2023-06-04T04:08:01Z

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

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

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 · 2023-09-07T02:33:30Z

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

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

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

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

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

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

improved error messages

72095f3

added tests, changed test_threading.ThreadTests.test_fork_at_exit to …

77443a0

…fit new behaviour

📜🤖 Added by blurb_it.

3134fbe

Merge branch 'main' into disallow-thread-creation-and-fork-at-interp-…

530f785

…exit

chgnrdv marked this pull request as ready for review May 24, 2023 12:46

chgnrdv requested a review from gpshead as a code owner May 24, 2023 12:46

bedevere-bot added the awaiting review label May 24, 2023

gpshead self-assigned this May 26, 2023

gpshead requested changes May 26, 2023

View reviewed changes

Lib/test/test_threading.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Core and Builtins/2023-05-24-12-10-54.gh-issue-104690.HX3Jou.rst Outdated Show resolved Hide resolved

Modules/_posixsubprocess.c Outdated Show resolved Hide resolved

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

gpshead requested a review from vstinner May 26, 2023 04:23

gpshead added the needs backport to 3.12 bug and security fixes label May 26, 2023

chgnrdv added 4 commits May 26, 2023 12:07

Merge branch 'main' into disallow-thread-creation-and-fork-at-interp-…

d26b8db

…exit

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

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`

fixed test case in test_subprocess

bcbcb75

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

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

Reword and clarify the NEWS entry.

0926f6f

gpshead approved these changes May 26, 2023

View reviewed changes

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

gpshead enabled auto-merge (squash) June 3, 2023 15:45

gpshead merged commit ce558e6 into python:main Jun 4, 2023

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

gpshead added needs backport to 3.12 bug and security fixes and removed needs backport to 3.12 bug and security fixes labels Jun 4, 2023

bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jun 4, 2023

erlend-aasland mentioned this pull request Jun 6, 2023

3.12 backport gh 105236 #105358

Closed

AdamWill mentioned this pull request Jul 5, 2023

Handle subprocess disallowing preexec during shutdown rhinstaller/anaconda#4879

Merged

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

sentrivana mentioned this pull request Nov 2, 2023

Officially support Python 3.12 getsentry/sentry-python#2480

Closed

4 tasks

pavyamsiri mentioned this pull request Jan 19, 2024

RuntimeErrors upon interpreter shutdown due to garbage collection pynbody/pynbody#762

Closed

tacaswell mentioned this pull request Feb 9, 2024

Can not communicate with subprocess during interpreter shutdown on windows #115219

Closed

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 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 mentioned this pull request Apr 3, 2024

Migrate to gunicorn + gevent mozmeao/basket#1350

Closed

ppigazzini mentioned this pull request Apr 4, 2024

Mitigate Python 3.12 Shutdown RuntimeError official-stockfish/fishtest#1925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-104690 Disallow thread creation and fork at interpreter finalization #104826

gh-104690 Disallow thread creation and fork at interpreter finalization #104826

chgnrdv commented May 24, 2023 •

edited by gpshead

Loading

bedevere-bot commented May 26, 2023

chgnrdv commented May 26, 2023

bedevere-bot commented May 26, 2023

gpshead commented May 26, 2023

miss-islington commented Jun 4, 2023

miss-islington commented Jun 4, 2023

miss-islington commented Jun 4, 2023

bedevere-bot commented Jun 4, 2023

vstinner commented Sep 7, 2023

gh-104690 Disallow thread creation and fork at interpreter finalization #104826

gh-104690 Disallow thread creation and fork at interpreter finalization #104826

Conversation

chgnrdv commented May 24, 2023 • edited by gpshead Loading

bedevere-bot commented May 26, 2023

chgnrdv commented May 26, 2023

bedevere-bot commented May 26, 2023

gpshead commented May 26, 2023

miss-islington commented Jun 4, 2023

miss-islington commented Jun 4, 2023

miss-islington commented Jun 4, 2023

bedevere-bot commented Jun 4, 2023

vstinner commented Sep 7, 2023

chgnrdv commented May 24, 2023 •

edited by gpshead

Loading