support gevent-using apps when workers>2 after #2183#2853
Conversation
Kludex#2796 shows the issue for gevent usage including a demo for the issue I am trying to solve. This change updated multi-processing to use `socket` instead of `pipe` because it is patchable! Thanks for the project, let me know if this patch is worthwhile to you!
f799112 to
adcb8f1
Compare
hrv-dys
left a comment
There was a problem hiding this comment.
Interesting approach. Replacing multiprocessing.Pipe with socket.socketpair makes the IPC mechanism compatible with gevent's monkey-patching, since gevent patches socket but not multiprocessing.Connection.
Some thoughts:
-
The
ping/pongprotocol change is straightforward —sendall(b"ping")/recv(4)instead ofconn.send(b"ping")/conn.recv(). The explicit 4-byte recv matches the 4-byte "ping"/"pong" messages, which is correct. -
Error handling in
ping()— catching(TimeoutError, OSError)is right. Thefinallyblock resetting to blocking mode with its owntry/except OSErroris defensive — if the socket was closed between the recv and the reset, it won't crash. Solid. -
The
__del__method onProcess— this is a safety net for socket cleanup. It's fine as a fallback, but__del__in CPython has known issues with reference cycles and GC ordering. The explicit cleanup injoin()andstart()(closing the unused end) is the primary cleanup mechanism, which is good. -
always_pong()now catchesOSError— needed becausesocket.recvraisesOSErrorwhen the socket is closed, unlikeConnection.recvwhich raisesEOFError. The testtest_process_ping_after_socket_closedvalidates this. -
The
with_running_supervisorcontext manager in tests is a nice cleanup — ensures the supervisor thread gets joined even if the test fails. Theshould_exit.set()in the finally block is a good fallback. Reduces the chance of zombie threads leaking between tests. -
One concern:
socket.socketpair()creates AF_UNIX sockets on Unix. On Windows, Python'ssocket.socketpair()creates a TCP loopback pair, which should still work but is slightly different in terms of buffering behavior. Since the messages are tiny (4 bytes), this shouldn't matter in practice, but it's a behavioral difference frommultiprocessing.Pipeon Windows.
All 14 multiprocess tests pass (1 skipped — SIGBREAK on macOS, pre-existing).
|
Interesting. I'll see if I have time to check it soon. |
|
thanks! let me know if you need anything from me! |
|
This doesn't seem enough - can you try with gevent monkeypatching to see if it is? @markjm |
Summary
#2796 shows the issue for gevent usage including a demo for the issue I am trying to solve.
This change updated multi-processing to use
socketinstead ofpipebecause it is patchable! Feel free to try out the demo script i included in the discussion and see that it fails without this change and passes with it. FWIW we have been running with this patch since Jan with no issueThanks for the project, let me know if this patch is worthwhile to you!
Checklist
I am missing a few lines of coverage - can come back to work on those if this is a change you would be interested in pursuing!