"Fix" fork() so it "works" on Python 3.13, and "works" better on older Python versions by itamarst · Pull Request #1047 · eventlet/eventlet

itamarst · 2025-06-11T15:39:36Z

CAVEAT: fork() is a terrible idea and should never be used with Python programs.

The root cause here is that:

Python assumes threads die when fork() runs, so it has a post-fork handler that cleans up the Python objects representing those threads.
Eventlet monkey-patches threading.py so it's actually tracking greenlets, and greenlets do survive forks.
The solution is therefore to disable threading.py's post-fork handler.

In addition, I discovered that the way fork() affected older versions of Python was buggy, e.g. threading.enumerate() didn't list all threads because of item 1 above. So I added tests and fixed that too.

This has a behavior change insofar as eventlet.green.thread.Thread is now tracked and visible in threading.py APIs, but that seems... fine? I think?

itamarst · 2025-06-11T15:42:52Z

    for t in threads:
        t.start()

-    check(2, threading, 'pre-fork patched')


What's going on here is that previously threads created by threading and threads created by eventlet.green.threading were tracked separately, and now they're just tracked in both.

Specifically:

threading tracked the main thread and the thread it started (see earlier in the test), for a total of 2.

eventlet.green.threading tracked the main thread the 3 threads it started, for a total of 4.

So in total there are 5 threads; main thread is counted by both, so it's 1 + (2 - 1) + (4 - 1) = 5.

To put another way: this is a semantic change, but seems fine to me.

I think we have to strongly highlight this behavioural change in documentation and also somewhere in the release note, do you mind adding some words about that to explain to users what they can expect or not.
I suppose this patch will potentially break lot of unit tests outside of Eventlet where people are checking for similar counting for some reasons.

Apparently, according to comments, Swift and Neutron, at least, seems to rely in some ways on pre-fork and both are Eventlet compliant https://codesearch.opendev.org/?q=pre-fork&i=nope&literal=nope&files=&excludeFiles=&repos=

Is this based on just the phrasing, or do you know which libraries they're using to start the subprocesses? Since for a long time fork() was the only way to create a subprocess, and so "forking" was synonymous with starting a subprocess.

(After the fork(), you would then execve() or friends to effectively start a new process. So that is fine. The real issue is fork() without execve(), where the memory copied from the parent process is persisted in the child process.)

I suppose they use oslo.service to achieve that.

itamarst · 2025-06-11T15:43:25Z

-    check(4, eventlet.green.threading, 'pre-fork green')
+    check(5, eventlet.green.threading, 'pre-fork green')

    if os.fork() == 0:


This was asserting that the child lost all its threads. Which would be the case for real threads, but green threads do survive fork() so these assertions were wrong.

Ack, thanks for the heads up. Make sense.

itamarst · 2025-06-11T15:54:54Z

Note that unit tests didn't run! Going to open issue about that if there isn't one already.

itamarst · 2025-06-11T15:56:28Z

Blocked on #1048

4383

Hey Itamar, great job as always!

Your changes LGTM, I would just suggest to highlight the asyncio/forking changes into the documentations to explain that to users. See my inline comments.

Feel free to merge it or to amend it with doc changes, or to address the doc change in a follow up PR, as you prefer.

4383 · 2025-06-12T09:00:42Z

    for t in threads:
        t.start()

-    check(2, threading, 'pre-fork patched')


I think we have to strongly highlight this behavioural change in documentation and also somewhere in the release note, do you mind adding some words about that to explain to users what they can expect or not.
I suppose this patch will potentially break lot of unit tests outside of Eventlet where people are checking for similar counting for some reasons.

4383 · 2025-06-12T09:03:43Z

    for t in threads:
        t.start()

-    check(2, threading, 'pre-fork patched')


Apparently, according to comments, Swift and Neutron, at least, seems to rely in some ways on pre-fork and both are Eventlet compliant https://codesearch.opendev.org/?q=pre-fork&i=nope&literal=nope&files=&excludeFiles=&repos=

4383 · 2025-06-12T09:04:50Z

-    check(4, eventlet.green.threading, 'pre-fork green')
+    check(5, eventlet.green.threading, 'pre-fork green')

    if os.fork() == 0:


Ack, thanks for the heads up. Make sense.

codecov · 2025-06-12T13:07:13Z

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 56%. Comparing base (8e81dd9) to head (9eb16bc).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
eventlet/__init__.py	80%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1047   +/-   ##
======================================
- Coverage      56%     56%   -1%     
======================================
  Files          89      89           
  Lines        9847    9852    +5     
  Branches     1643    1645    +2     
======================================
  Hits         5560    5560           
- Misses       3920    3924    +4     
- Partials      367     368    +1

Flag	Coverage Δ
ipv6	`23% <18%> (-1%)`	⬇️
py310asyncio	`53% <81%> (-1%)`	⬇️
py310epolls	`53% <56%> (-1%)`	⬇️
py310poll	`53% <56%> (-1%)`	⬇️
py310selects	`53% <56%> (-1%)`	⬇️
py311asyncio	`53% <81%> (-1%)`	⬇️
py311epolls	`53% <56%> (-1%)`	⬇️
py312asyncio	`50% <81%> (-1%)`	⬇️
py312epolls	`51% <56%> (-1%)`	⬇️
py313asyncio	`50% <81%> (-1%)`	⬇️
py313epolls	`51% <56%> (-1%)`	⬇️
py39asyncio	`51% <81%> (-1%)`	⬇️
py39dnspython1	`51% <56%> (-1%)`	⬇️
py39epolls	`53% <56%> (-1%)`	⬇️
py39openssl	`52% <56%> (-1%)`	⬇️
py39poll	`53% <56%> (-1%)`	⬇️
py39selects	`53% <56%> (-1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

itamarst · 2025-06-12T13:09:50Z

Note that tests didn't run because of #1048 and now that I reran them they're all red 😀 So more work to be done.

…fork-python3.13

4383 · 2025-06-12T15:45:05Z

Note that tests didn't run because of #1048 and now that I reran them they're all red 😀 So more work to be done.

😭 😂

4383 · 2025-06-12T15:46:56Z

I didn't checked properly, I was thinking they were all green because you re-enabled them and because this patch had fixed the situation

itamarst · 2025-06-12T18:07:05Z

Some progress made (the test was too brittle), still slightly more to fix plus addressing review comments.

4383 · 2025-06-13T09:24:37Z

Some progress made (the test was too brittle), still slightly more to fix plus addressing review comments.

Ack, thanks for the update.
Ping me when you think you are done and then I'll review it again.

itamarst · 2025-06-16T19:55:52Z

What the changelog should say:

Behavior change: threads created by eventlet.green.threading.Thread and threading.Thead will be visible across both modules if monkey patching was used. Previously each module would only list threads created in that module.

Bug fix: after fork(), greenlet threads are correctly listed in threading.enumerate() if monkey patching was used. You should not use fork()-without-execve().

itamarst · 2025-06-16T19:56:27Z

Changelog happens in release PRs? But there's text above.

The test failure is the unrelated TLS failure we get on macOS sometimes, looks like.

4383 · 2025-06-17T07:48:02Z

Concerning the changelog, I'm gonna go to release a new version, so I'll simply copy and paste your sentence into NEWS

pythonspeed added 11 commits June 9, 2025 16:59

WIP trying to solve this

8ebf4d4

Attempt 2

acd568c

Attempt 3.

6557cb2

Make it reflect new, less buggy reality

13ed1d1

New tests.

cb1a9e3

Make new tests pass.

9f00e3d

Allow asyncio event loop to survive fork(). This is a bad idea.

7aed73b

Make the test more robust.

700e4c2

Not all Python versions have post-fork handler.

6e58cc2

Nicer errors.

5915331

Don't need this on older Pythons either.

caf8314

itamarst commented Jun 11, 2025

View reviewed changes

pep8 fixes.

d04bef4

itamarst marked this pull request as ready for review June 11, 2025 15:54

4383 mentioned this pull request Jun 12, 2025

os.fork() doesn't work with Eventlet on Python 3.13 #1030

Closed

4383 approved these changes Jun 12, 2025

View reviewed changes

4383 mentioned this pull request Jun 12, 2025

Handle ReferenceError except while count rlock #1042

Merged

Merge branch 'master' into 1030-fork-python3.13

b9c1bb3

pythonspeed added 2 commits June 12, 2025 09:16

Correct comment.

1ba88e0

Merge remote-tracking branch 'origin/1030-fork-python3.13' into 1030-…

94cd2dd

…fork-python3.13

itamarst marked this pull request as draft June 12, 2025 16:07

pythonspeed added 2 commits June 12, 2025 12:23

More robust tests.

74417e9

Pacify code style

88b20e9

"Fix" Python 3.9-3.11.

373ed9b

pythonspeed added 2 commits June 16, 2025 15:41

Documentation

faccdb8

Warn users.

9eb16bc

itamarst marked this pull request as ready for review June 16, 2025 19:55

itamarst requested a review from 4383 June 16, 2025 19:56

4383 approved these changes Jun 17, 2025

View reviewed changes

4383 merged commit a4dcd4d into master Jun 17, 2025
28 of 29 checks passed

4383 deleted the 1030-fork-python3.13 branch June 17, 2025 07:49

4383 mentioned this pull request Jun 17, 2025

Garbage collection happens on thread switch, freeing in-use reference on 3.13 #1032

Closed

Conversation

itamarst commented Jun 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itamarst commented Jun 11, 2025

Uh oh!

itamarst commented Jun 11, 2025

Uh oh!

4383 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

itamarst commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

4383 commented Jun 12, 2025

Uh oh!

4383 commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamarst commented Jun 12, 2025

Uh oh!

4383 commented Jun 13, 2025

Uh oh!

itamarst commented Jun 16, 2025

Uh oh!

itamarst commented Jun 16, 2025

Uh oh!

4383 commented Jun 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jun 12, 2025 •

edited

Loading

itamarst commented Jun 12, 2025 •

edited

Loading

4383 commented Jun 12, 2025 •

edited

Loading