Skip to content
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.

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

avoid interleaving pyuwsgi threadstate #2660

Closed

Conversation

asottile
Copy link
Contributor

resolves #2659

so there's quite a lot going on here so I'll try to explain the change

  • 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 (for a reason I'm not completely understanding -- but my notes are below this)
    • 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 the new version of the code, once we call uwsgi_setup we never give control back to the original pyuwsgi threadstate avoiding the Swap dance entirely!

@asottile
Copy link
Contributor Author

I did end up figuring out why 3.12's subtle behaviour change caused the deadlock as well

  • 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 -- however I think this is exceedingly unlikely)
  • 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 -- though I think my patch is still an improvement on that idea even if it were possible
  • 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.

@asottile asottile changed the title avoid interleaving pywsgi threadstate avoid interleaving pyuwsgi threadstate Aug 22, 2024
@xrmx
Copy link
Collaborator

xrmx commented Sep 8, 2024

Thanks, added the explanation to the commit message in #2670

@xrmx xrmx closed this Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pyuwsgi + enable-threads + python3.12: workers do not recover after a reload
2 participants