Fix threading monkey patching for Python 3.13 fork handling#1044
Fix threading monkey patching for Python 3.13 fork handling#1044SeanMooney wants to merge 1 commit into
Conversation
This resolves an assertion failure in threading._after_fork() when using monkey-patched threads on Python 3.13. The root cause was the stricter thread identity verification in _make_thread_handle() that didn't account for how Python 3.13 reinitializes threads after forking. Key changes: 1. Replaced the problematic identity assertion with a conditional check 2. Return a safe "done" thread handle during fork detection 3. Add graceful handling for cases with no current greenlet The new implementation: - Maintains thread safety while preventing crashes during forking - Works with Python 3.13's revised thread initialization after fork - Preserves backwards compatibility with older Python versions - Allows stdlib's _after_fork() to properly recreate thread objects Closes eventlet#1030 Assisted-By: aider (openrouter/deepseek/deepseek-r1-0528:free) <noreply@aider.chat> Assisted-By: aider (openrouter/google/gemini-2.0-flash-exp:free) <noreply@aider.chat> Signed-off-by: Sean Mooney <work@seanmooney.info>
|
not i have only tested this by running eventlet's unit tests and the reproducer under python 3.13 |
| return _ThreadHandle() | ||
| except greenlet.error: | ||
| # Handle cases with no current greenlet | ||
| return _ThreadHandle() |
There was a problem hiding this comment.
thinking about this more. this will fix the reproducer script but im not convinced its correct. with that said i am hoping to find time to actully deploy with debian 13 or ubuntu 25.04 to try and repoduce the poblems in the next week or so.
this might work but without actully testing it with openstack and seing if it fixes the porblem in a real end to end usecase it s hard to determin if this really is the direction to take.
the repoducer uses the _at_fork hook to demonstrate teh probelm but that may have biased the soltuion so we may need other repoducers too to be able to fully determin why this failes the way it id.
|
So here is the thing: fork() without exec() is fundamentally broken. See e.g. writeup I made about If you are seeing problems with fork() you should stop using fork()`. Even if we "fix" eventlet there will be other mysterious intermittent bugs. So separate from this PR I would treat any use of fork() on your side as something that needs to be ripped out ASAP. It's just broken. There is no way to make it not broken. |
|
I am inclined to say we shouldn't accept anything that is intended to fix fork() without a lot of thought and understanding of impacts. Given this was generated by an LLM I am also inclined to reject it without even spending anytime thinking about it. Eventlet is a very brittle system and changes we've made have had unexpected consequences, I would like to actually have code written by an entity capable of having some understanding of state. |
|
the repoducer is form #1030 is that is not actully forking it just show that the way we are doing the tread identity is broken i think that repoducer may have partly lead my ai analasys in the wrong direction and we need more examples of the failrue that do not invovle calling _after_fork(). with that said oslo service is callifn fork in teh eventlet mode when there are multiple workers so until the eventlet backend of oslo.service is updated to use spawn this is still a problem we need to adress. the right fix is undobable to update how the eventlest backend of oslo service work to not use fork, but dont discount this as jsut a llm generated patch. i spent 3-4 hours working with several different tools to try and isolate a potential cause for the issue. and i explicitly called out that while this passes all of eventlets test, i was hoping to repoduce the actual issue in a real deployment to confirm if this actully adress the probelm before we go any future. with the patch |
|
I am discounting this because it was written by an LLM, specifically because LLMs aren't by their nature going to be good at modeling massively stateful systems, especially ones with hideous hidden state like this one. They can look at patterns in text. In some situations that's good enough. Here's the hideous mess we're dealing with here, though:
A plausible solution is therefore to disable the stdlib threading.py post-fork handler in some limited cases. I have this working in a branch, so will close this and submit once I've written a bunch of tests. |
|
thanks for taking the time to explain this and how you intend to proceed. |
Explanation of the fix:
after forking
_make_thread_handle()
get_ident(greenthread)
• The thread identities change
• Green thread mapping isn't preserved across forks
• The main thread recreation requires custom handling
• Only perform identity verification when NOT in a fork scenario
• Return a safe "done" thread handle during fork situations
• Handle cases where no current greenlet exists
Python 3.13's revised fork behavior
Note i have created this patch using aider, a terminal based AI coding tool
with my personal open-router key to access the deepseak-r1 and Gemini
flash LLMs.
deepseak drove the analysis of the the trackback and code base and
gemini applied the proposed changes to the files.
Closes #1030