Clean up threading book-keeping at fork when monkey-patched #611
Conversation
| @@ -312,6 +312,26 @@ def monkey_patch(**on): | |||
| for attr_name in deleted: | |||
| if hasattr(orig_mod, attr_name): | |||
| delattr(orig_mod, attr_name) | |||
|
|
|||
| if name == 'threading' and sys.version_info >= (3, 7): | |||
There was a problem hiding this comment.
Or maybe this should say ... and hasattr(_os, 'register_at_fork'):?
There was a problem hiding this comment.
i think yes, they may remove register_at_fork in future version; checking py version in test is the right move though
tipabu
force-pushed
the
active-thread-tracking
branch
from
9d1897f
to
3df6c27
Compare
Codecov Report
@@ Coverage Diff @@
## master #611 +/- ##
=====================================
Coverage 46% 46%
=====================================
Files 81 81
Lines 7977 7977
Branches 1366 1366
=====================================
Hits 3700 3700
Misses 4019 4019
Partials 258 258
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I can't imagine anything that makes this change any better - amazing work @tipabu
the register_at_fork handling seems very inline with the cpython change that prompted all this and having a clean slate after fork seems like a very reasonable state to struggle towards; so even if it's complicated at least we're all pulling in the same direction to try and reduce complexity.
| _global_dict=original('threading').current_thread.__globals__, | ||
| _patched=orig_mod | ||
| ): | ||
| _prefork_active = [None] |
There was a problem hiding this comment.
py2/3 is SO annoying, even tho this code is py3 only you can't use nonlocal because it's a SyntaxError on py2 🙄
tests/patcher_test.py
Outdated
| # but old pythons make it difficult to ensure | ||
| if sys.version_info >= (3, 7): | ||
| check(1, threading, 'child post-fork patched') | ||
| check(1, _threading, 'child post-fork original') |
There was a problem hiding this comment.
so this wasn't expected - and it may be the crux of the new 3.7 feature that's applying some cleaning.
This is closer to what I expected from my experience with python and forking with threads:
diff --git a/tests/patcher_test.py b/tests/patcher_test.py
index 0bebe39..ff8dc24 100644
--- a/tests/patcher_test.py
+++ b/tests/patcher_test.py
@@ -326,6 +326,9 @@ if os.fork() == 0:
if sys.version_info >= (3, 7):
check(1, threading, 'child post-fork patched')
check(1, _threading, 'child post-fork original')
+ else:
+ check(2, threading, 'child post-fork patched')
+ check(3, _threading, 'child post-fork original')
check(1, eventlet.green.threading, 'child post-fork green')
exit()
else:
But I don't have enough experience outside of python to say if that's "reasonable"
There was a problem hiding this comment.
IDK -- seems pretty consistent with how (unpatched) stdlib threading works across a variety of versions:
$ cat test_threads_and_forking.py
import os
import threading
import time
threads = [threading.Thread(target=time.sleep, args=(1,)) for _ in range(2)]
for t in threads: t.start()
time.sleep(0.1) # Give py2 threads a chance to actually start
print('pre-fork: {}'.format(len(threading._active)))
label = 'child' if os.fork() == 0 else 'parent'
print('post-fork ({}): {}'.format(label, len(threading._active)))
print('liveness ({}): {}'.format(label, [t.is_alive() for t in threads]))
for t in threads: t.join()
if label == 'child':
exit()
else:
os.wait()
$ for p in python2.7 python3.5 python3.6 python3.7 python3.8; do $p --version ; $p test_threads_and_forking.py ; echo ; done
Python 2.7.17
pre-fork: 3
post-fork (parent): 3
liveness (parent): [True, True]
post-fork (child): 1
liveness (child): [False, False]
Python 3.5.9
pre-fork: 3
post-fork (parent): 3
liveness (parent): [True, True]
post-fork (child): 1
liveness (child): [False, False]
Python 3.6.9
pre-fork: 3
post-fork (parent): 3
liveness (parent): [True, True]
post-fork (child): 1
liveness (child): [False, False]
Python 3.7.7
pre-fork: 3
post-fork (parent): 3
liveness (parent): [True, True]
post-fork (child): 1
liveness (child): [False, False]
Python 3.8.3
pre-fork: 3
post-fork (parent): 3
liveness (parent): [True, True]
post-fork (child): 1
liveness (child): [False, False]
| _global_dict=original('threading').current_thread.__globals__, | ||
| _patched=orig_mod | ||
| ): | ||
| _prefork_active = [None] |
There was a problem hiding this comment.
py2/3 is SO annoying - even tho this code is py3 only you can't use nonlocal because it's a SyntaxError on py2 🙄
|
Is there any time frame as to when this PR will be merged. I have high hopes this will resolve a block on supporting Python 3.7+ on a project that depends on eventlet. |
Codecov Report
@@ Coverage Diff @@
## master #611 +/- ##
=======================================
- Coverage 52% 46% -7%
=======================================
Files 87 81 -6
Lines 9784 7977 -1807
Branches 1719 1366 -353
=======================================
- Hits 5159 3700 -1459
+ Misses 4275 4019 -256
+ Partials 350 258 -92
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
temoto
force-pushed
the
active-thread-tracking
branch
from
0d46519
to
7041c84
Compare
|
@tipabu thanks for your work; please say if i'm pushing hospitality with rebasing your branch. Extracted test code into isolated/ file. Otherwise LGTM and ready to merge if you are fine with these changes. |
|
By all means, push away! I'm always a fan of whatever gets fixes mergeable faster 👍 The extraction makes sense; was just looking at the precedent around it. I may have set those sleeps too short to be robust in CI, but other than that, |
Previously, if we patched threading then forked (or, in some cases, used
the subprocess module), Python would log an ignored exception like
Exception ignored in: <function _after_fork at 0x7f16493489d8>
Traceback (most recent call last):
File "/usr/lib/python3.7/threading.py", line 1335, in _after_fork
assert len(_active) == 1
AssertionError:
This comes down to threading in Python 3.7+ having an import side-effect
of registering an at-fork callback. When we re-import threading to patch
it, the old (but still registered) callback still points to the old
thread-tracking dict, rather than the new dict that's actually doing the
tracking.
Now, register our own at_fork hook that will fix up the dict reference
before threading's _at_fork runs and put it back afterwards.
Closes eventlet#592
tipabu
force-pushed
the
active-thread-tracking
branch
from
7041c84
to
115103d
Compare
Codecov Report
@@ Coverage Diff @@
## master #611 +/- ##
======================================
- Coverage 44% 44% -1%
======================================
Files 87 87
Lines 11813 11831 +18
Branches 1773 1774 +1
======================================
Hits 5269 5269
- Misses 6150 6168 +18
Partials 394 394
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
v0.27.0 on PyPI |
Previously, if we patched
threadingthen forked (or, in some cases, used thesubprocessmodule), Python would log an ignored exception likeThis comes down to threading in Python 3.7+ having an import side-effect of registering an at-fork callback. When we re-import threading to patch it, the old (but still registered) callback still points to the old thread-tracking dict, rather than the new dict that's actually doing the tracking.
Now, register our own at_fork hook that will fix up the dict reference before
threading's_at_forkruns and put it back afterwards.Closes #592