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
Fix #508: Py37 Deadlock ThreadPoolExecutor #598
Fix #508: Py37 Deadlock ThreadPoolExecutor #598
Conversation
Python 3.7 and later implement queue.SimpleQueue in C, causing a deadlock when using ThreadPoolExecutor with eventlet. To avoid this deadlock we now replace the C implementation with the Python implementation on monkey_patch for Python versions 3.7 and higher.
CI test failures are caused by the incompatibility of the six version with the virtualenv version, as can be seen in the logs. Virtualenv version 20.0.2 requires six version 1.12.0 or higher, as it needs the patch that adds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! (And sorry it took so long to get eyes on it.) Patch seems to make sense -- one thought on scoping for when to do the swap, but it's probably safer this way. LGTM, but I'm no patching expert.
# causing a deadlock. Replace the C implementation with the Python one. | ||
if sys.version_info >= (3, 7): | ||
import queue | ||
queue.SimpleQueue = queue._PySimpleQueue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we only do this when on['thread']
is true? It shouldn't be a problem otherwise, right?
OTOH, we have precedent for a blanket use-more-python approach up just a few lines above...
|
||
|
||
def test_threadpoolexecutor(): | ||
tests.run_isolated('patcher_threadpoolexecutor.py') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a little worried about how tests still pass when I reverted the change to patcher.py
... but that's a problem in the test infrastructure, not this patch. Submitted #612 to address.
future = executor.submit(pow, 2, 3) | ||
res = future.result() | ||
assert res == 8, '2^3 should be 8, not %s' % res | ||
print('pass') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a bit of an indirect test; something like
import eventlet
eventlet.monkey_patch()
import threading
try:
import queue
except ImportError:
import Queue as queue # py2
def tgt(q):
print(q.get())
# SimpleQueue is sufficient, but was only added in 3.7
q = getattr(queue, 'SimpleQueue', queue.Queue)()
t = threading.Thread(target=tgt, args=(q,))
t.start()
q.put('pass')
t.join()
seems a little more obvious -- could even tighten it to just use eventlet.monkey_patch(all=False, thread=True)
.
OTOH, this more-clearly demonstrates the fact that we had a thing that was working when monkey-patched prior to 3.7 and stopped working after. *shrug*
Python 3.7 and later implement queue.SimpleQueue in C, causing a
deadlock when using ThreadPoolExecutor with eventlet.
To avoid this deadlock we now replace the C implementation with the
Python implementation on monkey_patch for Python versions 3.7 and
higher.
Fix #508