fix(python-plugin): update internal interpreter state immediately upon forking #2388

majorgreys · 2021-12-26T18:50:23Z

This PR adds pre_uwsgi_fork and post_uwsgi_fork hooks to uwsgi_plugin in order to update the internal interpreter state of CPython immediately before and immediately after the call to uwsgi_fork. This follows the implementation of the standard library os.fork() in CPython.

While these two hooks effectively replace existing code to update the internal interpreter state during the lifecycle of master and worker processes, we preserve the current behavior for uWSGI LTS. We instead provide a new option py-call-uwsgi-fork-hooks to enable the new feature.

We add test coverage for the issue of deadlocked workers raised in #2387 parameterized for relevant configurations options.

unbit · 2021-12-27T07:54:52Z

uwsgi.h

@@ -1039,6 +1039,8 @@ struct uwsgi_plugin {
 	void *data;
 	void (*on_load) (void);
 	int (*init) (void);
+	void (*pre_uwsgi_fork) (void);


Can you move it at the end of the structure for maintaining ABI?

unbit · 2021-12-27T07:57:22Z

core/uwsgi.c

@@ -3338,14 +3338,6 @@ int uwsgi_start(void *v_argv) {
 		}
 	}

-	// master fixup


you should leave this part in for not breaking other plugins

@unbit I did not see the master_fixup hook used elsewhere in the repository but that doesn't mean it's not used by a plugin a user might have, so I can add this back. Are you aware of such a plugin?

unbit · 2021-12-27T07:57:38Z

core/mule.c

@@ -69,12 +69,6 @@ void uwsgi_mule(int id) {

 		uwsgi_close_all_sockets();

-		for (i = 0; i < 256; i++) {


you should leave this part in for not breaking other plugins

unbit · 2021-12-27T08:06:51Z

@majorgreys thanks a lot, my only doubt (in addition to the review comments) is about the old (and ugly, obviously) up.call_osafterfork. Basically it loses any meaning with your patch so i think we can safely remove this (at least in master). For LTS i would probably introduce this new behaviour (albeit definitely correct) as an option. I know it is annoying but LTS grants the user that we do not break/change old stuff (honestly i am not sure about the implications, very probably only app-managed signal management will be affected, and in a positive way). @xrmx any thought?

majorgreys · 2021-12-27T18:28:50Z

I've updated the change now to make it so the old behavior can be retained, specifically with regards to call_osafterfork.

Given the needs of LTS, I'm inclined to focus this PR on non-breaking changes so it can be easily backported to the 2.0 branch and open a separate PR for the breaking changes so that they can end up on master. @unbit Does that work for what you had in mind for how to introduce these changes?

…er state

majorgreys · 2021-12-29T05:36:29Z

@unbit @xrmx I have updated the PR to introduce this as an opt-in feature and avoid any breaking changes so it can be backported into the LTS 2.0 branch.

brettlangdon · 2022-01-24T19:59:02Z

@unbit or @xrmx have you had a chance to look over this PR again? Is there anything else you would like us to update?

majorgreys · 2022-03-21T19:52:40Z

👋🏽 @unbit Any update on this PR? I addressed the concerns you raised with regards to merging this into LTS.

xrmx · 2022-03-21T20:32:52Z

Merged and broke master CI :)
Search for "ERROR: uWSGI did not start"
https://github.com/unbit/uwsgi/runs/5633992704?check_suite_focus=true

majorgreys · 2022-03-21T20:58:46Z

@xrmx Thanks for the heads up. I'll see if I can reproduce this locally.

Superskyyy · 2022-11-23T18:32:08Z

@majorgreys Hello, thanks for fixing the issue, but I was trying the new option py-call-uwsgi-fork-hooks and the following ignored exception prints out if and only if the enable-threads option is given.

uwsgi --http :9090 --wsgi-file demo/uwsgi_app.py --py-call-uwsgi-fork-hooks --processes 4 works fine,
uwsgi --http :9090 --wsgi-file demo/uwsgi_app.py --enable-threads --py-call-uwsgi-fork-hooks --processes 4 prints the following errors.
`


Exception ignored in: <function _after_at_fork_child_reinit_locks at 0x7fd01e7ecdc0>
Traceback (most recent call last):
  File "/home/gitpod/.pyenv/versions/3.8.13/lib/python3.8/logging/__init__.py", line 264, in _after_at_fork_child_reinit_locks
    _releaseLock()  # Acquired by os.register_at_fork(before=.
  File "/home/gitpod/.pyenv/versions/3.8.13/lib/python3.8/logging/__init__.py", line 232, in _releaseLock
    _lock.release()
RuntimeError: cannot release un-acquired lock

This was discussed here but remains unresolved, can you help to see if it is my env problem or something else? Really appreciated.

majorgreys · 2022-11-29T22:17:00Z

@Superskyyy I need to still review the linked issue but I can confirm that I can reproduce this error. I don't see it when --py-call-uwsgi-fork-hooks is not used along with --enable-threads.

#2179 also reported the same exception when --py-call-osafterfork was used.

Code for minimal reproduction: https://gist.github.com/majorgreys/3ee1716b107ad80226b12145ae87276a

fix: add tests for python worker deadlocks

eafb025

unbit reviewed Dec 27, 2021

View reviewed changes

majorgreys changed the title ~~fix(cpython): update internal interpreter state immediately upon forking~~ fix(python-plugin): update internal interpreter state immediately upon forking Dec 27, 2021

majorgreys force-pushed the fix-worker-deadlock branch from b6815df to edd4c74 Compare December 27, 2021 18:13

fix(python-plugin): add uwsgi fork hooks to update internal interpret…

3b4dcdd

…er state

majorgreys force-pushed the fix-worker-deadlock branch from edd4c74 to 3b4dcdd Compare December 29, 2021 05:22

majorgreys requested a review from unbit December 29, 2021 05:35

brettlangdon mentioned this pull request Jan 7, 2022

ddtrace.internal.writer:trace buffer(209 traces 8378483b/8388608b) cannot fit trace of size 27505b, dropping DataDog/dd-trace-py#2834

Closed

xrmx merged commit b7a5cf1 into unbit:master Mar 21, 2022

majorgreys deleted the fix-worker-deadlock branch March 21, 2022 20:58

majorgreys mentioned this pull request Mar 22, 2022

Smoke tests failing to start uwsgi in CI #2416

Closed

This was referenced May 11, 2022

Deadlock in uwsgi_python_master_fixup #1149

Open

Listen queue reported full and all requests failing immediately after graceful reload #1369

Open

PeterJCLaw mentioned this pull request Jun 6, 2023

Clarify support for forking process models open-telemetry/opentelemetry-python#3307

Open

zaheerabbas-prodigal mentioned this pull request Mar 4, 2024

Fix #1978: Support Python 3.7's new pre- and post-fork handlers #1995

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(python-plugin): update internal interpreter state immediately upon forking #2388

fix(python-plugin): update internal interpreter state immediately upon forking #2388

majorgreys commented Dec 26, 2021 •

edited

Loading

unbit Dec 27, 2021

unbit Dec 27, 2021

majorgreys Dec 27, 2021

unbit Dec 27, 2021

unbit commented Dec 27, 2021

majorgreys commented Dec 27, 2021 •

edited

Loading

majorgreys commented Dec 29, 2021

brettlangdon commented Jan 24, 2022

majorgreys commented Mar 21, 2022

xrmx commented Mar 21, 2022

majorgreys commented Mar 21, 2022

Superskyyy commented Nov 23, 2022 •

edited

Loading

majorgreys commented Nov 29, 2022

		@@ -69,12 +69,6 @@ void uwsgi_mule(int id) {

		uwsgi_close_all_sockets();

		for (i = 0; i < 256; i++) {

fix(python-plugin): update internal interpreter state immediately upon forking #2388

fix(python-plugin): update internal interpreter state immediately upon forking #2388

Conversation

majorgreys commented Dec 26, 2021 • edited Loading

unbit Dec 27, 2021

Choose a reason for hiding this comment

unbit Dec 27, 2021

Choose a reason for hiding this comment

majorgreys Dec 27, 2021

Choose a reason for hiding this comment

unbit Dec 27, 2021

Choose a reason for hiding this comment

unbit commented Dec 27, 2021

majorgreys commented Dec 27, 2021 • edited Loading

majorgreys commented Dec 29, 2021

brettlangdon commented Jan 24, 2022

majorgreys commented Mar 21, 2022

xrmx commented Mar 21, 2022

majorgreys commented Mar 21, 2022

Superskyyy commented Nov 23, 2022 • edited Loading

majorgreys commented Nov 29, 2022

majorgreys commented Dec 26, 2021 •

edited

Loading

majorgreys commented Dec 27, 2021 •

edited

Loading

Superskyyy commented Nov 23, 2022 •

edited

Loading