Rewrite IPC to use multiprocessing.connection #6010
Conversation
|
I think I will likely defer this till we figure out what is wrong with the daemon on Windows. |
| @@ -200,8 +200,8 @@ def serve(self) -> None: | |||
| json.dump({'pid': os.getpid(), 'connection_name': server.connection_name}, f) | |||
| f.write('\n') # I like my JSON with a trailing newline | |||
| while True: | |||
| with server: | |||
| data = receive(server) | |||
| with server as connection: | |||
There was a problem hiding this comment.
These semantics of __enter__ are a little too magical. I'd prefer to see something like with server.accept_connection() as connection here.
| @@ -232,12 +232,9 @@ def serve(self) -> None: | |||
| # simplify the logic and always remove the file, since | |||
| # that could cause us to remove a future server's | |||
| # status file.) | |||
| server.close() | |||
There was a problem hiding this comment.
Sully had a specific reason for the ordering of the closing of the socket and the unlinking of the status file; if you close the socket first, it's possible for a new client to start a new server before we get to remove the status file, and then we'd end up removing the wrong status file. See #5956.
There was a problem hiding this comment.
I think in this case it is just cleanliness. The bug fix was doing the file removal in cmd_stop, before any response is sent.
|
@ethanhs How important is this PR? In particular, do you think that it's okay to release mypy with official Windows dmypy support without this change, or should we wait until this is in? |
|
Ah, sorry I probably should have marked this WIP last night (but thank you for the feedback Guido). @JukkaL I don't think this needs to be in the release. I am a bit concerned with releasing with #5983 dangling over our heads, but I'm also a bit stumped at the moment because I am having difficulty reproducing it and gaining any extra information. |
|
I think it isn't worth it to do this after all. We have to re-implement enough that I think it is easier to just keep our own implementation. |
This re-writes the IPC module to use the communication primitives in
multiprocessing.connectionso we don't duplicate as much work. It also means we can replace out JSON serialization to passing objects (which should probably go in a separate PR).In short, this makes IPC much more robust.
There are a few missing definitions and other issues in typeshed for
multiprocessing.connection, I am planning on making a PR for that momentarily.