Skip to content

Commit

Permalink
Fix a race condition in dmypy (#5956)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
msullivan authored Nov 27, 2018
1 parent 1a9e280 commit 6402851
Showing 1 changed file with 13 additions and 1 deletion.
14 changes: 13 additions & 1 deletion mypy/dmypy_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -200,7 +201,13 @@ def serve(self) -> None:
reset_global_state()
sys.exit(0)
finally:
os.unlink(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:
shutil.rmtree(self.sock_directory)
exc_info = sys.exc_info()
Expand Down Expand Up @@ -235,6 +242,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]:
Expand Down

0 comments on commit 6402851

Please sign in to comment.