Skip to content

Commit

Permalink
pyuwsgi: avoid interleaving pywsgi threadstate
Browse files Browse the repository at this point in the history
In all versions of pyuwsgi at the moment the first fork has a NULL threadstate
due to uwsgi_python_master_fixup which calls UWSGI_RELEASE_GIL (expanded to
 PyEval_SaveThread -- which drops the GIL and sets threadstate to NULL).
This is called during uwsgi_setup.
After uwsgi_setup was returning, PyThreadState_Swap was restoring the pyuwsgi
threadstate (in both the original and worker processes)

Future forks would have the pyuwsgi threadstate active (from the
restoration at PyThreadState_Swap) in python versions < 3.12 this wasn't an issue.
In 3.12+ the PyEval_RestoreThread would attempt to take_gil and then block forever
on the GIL mutex (despite it actually holding it? due to the fork state from the
parent process).

Bisecting cpython showed that python/cpython@92d8bff slightly changed behaviour
of PyThreadState_Swap (it now additionally manages GIL state: unlocking the
previous threadstate and locking the new threadstate).
Putting a log line in the PyThreadState_Swap showed a suspicious swapping from
oldts=123123 to newts=123123 (swapping from its own threadstate to itself?);
this is because after forking control would be given back to the original
threadstate (which mostly worked but was in UB territory given the GIL state).

In 3.11 the threadstate that was restored after the PyThreadState_Swap did not
have the GIL locked (technically this could have allowed a data race if threads
existed before starting uwsgi via pyuwsgi).

In 3.12 since PyThreadState_Swap was changed to release the old threadstate's GIL
and acquire the GIL in the new threadstate this meant that the saved threadstate
had ->locked = 1 (which is sort of an invalid state?).
As far as I can tell there aren't any public apis to undo this and "restore" the
3.11 behaviour precisely.
Then later it would try and lock (despite already being -> locked = 1) and
deadlock against itself this is actually called out on the docs:
  If the lock has been created, the current thread must not have acquired it,
  otherwise deadlock ensues.

To fix this once we call uwsgi_setup we never give control back to the original
pyuwsgi threadstate avoiding the Swap dance entirely.
  • Loading branch information
asottile authored and xrmx committed Sep 8, 2024
1 parent 52d858a commit 25dbb4b
Showing 1 changed file with 2 additions and 6 deletions.
8 changes: 2 additions & 6 deletions plugins/pyuwsgi/pyuwsgi.c
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,6 @@ pyuwsgi_setup(PyObject *self, PyObject *args, PyObject *kwds)
return NULL;
}

//TODO: ...???
// actually do the thing!
PyThreadState *_tstate = PyThreadState_Get();
uwsgi_setup(orig_argc, orig_argv, environ);
PyThreadState_Swap(_tstate);

Py_INCREF(self);
return self;
}
Expand All @@ -133,6 +127,7 @@ pyuwsgi_init(PyObject *self, PyObject *args, PyObject *kwds)
return NULL;
}

uwsgi_setup(orig_argc, orig_argv, environ);
int rc = uwsgi_run();

// never(?) here
Expand All @@ -149,6 +144,7 @@ pyuwsgi_run(PyObject *self, PyObject *args, PyObject *kwds)
return NULL;
}

uwsgi_setup(orig_argc, orig_argv, environ);
int rc = uwsgi_run();

// never(?) here
Expand Down

0 comments on commit 25dbb4b

Please sign in to comment.