From e0490f73483d61c7cbf33525249088e7b8dd6b34 Mon Sep 17 00:00:00 2001 From: "Michael J. Sullivan" Date: Mon, 26 Nov 2018 14:05:30 -0800 Subject: [PATCH 1/2] Fix a race condition in dmypy While thinking about what might cause flakes in #5859, I found a race condition between `dmypy stop` and other `dmypy` commands. `dmypy stop` will send a response and close the socket before the status file has been deleted. This means that there is a window in which the daemon has reported to a client that it has exited but while a status file still exists. This can result in a number of issues, including the daemon appearing to be stuck (instead of stopped) to subsequent commands and also the exiting server deleting the status file of a subsequently started server. The fix is to remove the status file in `cmd_stop` before replying to the request. This ensures that, as far as clients are concerned, the daemon is exited after a stop command completes. I tested the bug and the fix by inserting a `time.sleep(1)` immediately before the `sys.exit(0)` in `serve`: this caused several tests to fail, and the changes fixed them. I believe that the observed flakes in the windows version in #5859 were caused by this issue, but in a way that the unix version was not susceptible to. --- mypy/dmypy_server.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/mypy/dmypy_server.py b/mypy/dmypy_server.py index 3bbcd84177fb..ad24f3f2a4d4 100644 --- a/mypy/dmypy_server.py +++ b/mypy/dmypy_server.py @@ -153,6 +153,7 @@ def __init__(self, options: Options, def serve(self) -> None: """Serve requests, synchronously (no thread or fork).""" + command = None try: sock = self.create_listening_socket() if self.timeout is not None: @@ -200,7 +201,12 @@ def serve(self) -> None: reset_global_state() sys.exit(0) finally: - os.unlink(STATUS_FILE) + # If this is something other than a clean stop, remove + # the status file. (We can't just simplify the logic + # and always remove the file, since that could cause + # us to remove a future server's status file.) + if command != 'stop': + os.unlink(STATUS_FILE) finally: shutil.rmtree(self.sock_directory) exc_info = sys.exc_info() @@ -235,6 +241,11 @@ def cmd_status(self) -> Dict[str, object]: def cmd_stop(self) -> Dict[str, object]: """Stop daemon.""" + # We need to remove the status file *before* we complete the + # RPC. Otherwise a race condition exists where a subsequent + # command can see a status file from a dying server and think + # it is a live one. + os.unlink(STATUS_FILE) return {} def cmd_run(self, version: str, args: Sequence[str]) -> Dict[str, object]: From 4ae483978ae99a23607ffedeab50166952cb5c91 Mon Sep 17 00:00:00 2001 From: "Michael J. Sullivan" Date: Mon, 26 Nov 2018 17:34:08 -0800 Subject: [PATCH 2/2] comment tweak --- mypy/dmypy_server.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/mypy/dmypy_server.py b/mypy/dmypy_server.py index ad24f3f2a4d4..212cf7e5c440 100644 --- a/mypy/dmypy_server.py +++ b/mypy/dmypy_server.py @@ -201,10 +201,11 @@ def serve(self) -> None: reset_global_state() sys.exit(0) finally: - # If this is something other than a clean stop, remove - # the status file. (We can't just simplify the logic - # and always remove the file, since that could cause - # us to remove a future server's status file.) + # If the final command is something other than a clean + # stop, remove the status file. (We can't just + # simplify the logic and always remove the file, since + # that could cause us to remove a future server's + # status file.) if command != 'stop': os.unlink(STATUS_FILE) finally: