Repository: python/mypy
Pull request: Some daemon code cleanup
- Move do_restart() and wait_for_server() right after do_start()
- Introduce BadStatus exception, use instead of SystemExit
- Add a timeout option to request(), use for stop/status
- Include all socket ops in the try/except clause
- Rationalize and document exceptions raised by receive().
- Move GcLogger to its own file; fixes #4194
- Make status quiet by default; add -v to print full status
- Replace -q flags with -v flags
- If 'error' in response, sys.exit with that message
- Streamline the way command subparsers are updated
- Misc other cleanup

Problem statement:
Refactor some parts of dmypy.py
- [ ] Move class GcLogger to its own file -- it depends on nothing.
- [x] Move the Server class to its own file, and the mypy imports with it -- could improve client startup time.

Hints from issue discussion:

Commit message: Fix flake8 & mypy errors

Patch to review:
diff --git a/mypy/dmypy.py b/mypy/dmypy.py
index f3e86402758e..180d2121cbc2 100644
--- a/mypy/dmypy.py
+++ b/mypy/dmypy.py
@@ -26,42 +26,50 @@
 parser.set_defaults(action=None)
 subparsers = parser.add_subparsers()
 
-start_parser = subparsers.add_parser('start', help="Start daemon")
-start_parser.add_argument('--log-file', metavar='FILE', type=str,
-                          help="Direct daemon stdout/stderr to FILE")
-start_parser.add_argument('flags', metavar='FLAG', nargs='*', type=str,
-                          help="Regular mypy flags (precede with --)")
+start_parser = p = subparsers.add_parser('start', help="Start daemon")
+p.add_argument('--log-file', metavar='FILE', type=str,
+               help="Direct daemon stdout/stderr to FILE")
+p.add_argument('flags', metavar='FLAG', nargs='*', type=str,
+               help="Regular mypy flags (precede with --)")
 
-status_parser = subparsers.add_parser('status', help="Show daemon status")
+restart_parser = p = subparsers.add_parser('restart',
+    help="Restart daemon (stop or kill followed by start)")
+p.add_argument('--log-file', metavar='FILE', type=str,
+               help="Direct daemon stdout/stderr to FILE")
+p.add_argument('flags', metavar='FLAG', nargs='*', type=str,
+               help="Regular mypy flags (precede with --)")
 
-stop_parser = subparsers.add_parser('stop', help="Stop daemon (asks it politely to go away)")
+status_parser = p = subparsers.add_parser('status', help="Show daemon status")
+p.add_argument('-v', '--verbose', action='store_true', help="Print detailed status")
 
-kill_parser = subparsers.add_parser('kill', help="Kill daemon (kills the process)")
+stop_parser = p = subparsers.add_parser('stop', help="Stop daemon (asks it politely to go away)")
 
-restart_parser = subparsers.add_parser('restart',
-    help="Restart daemon (stop or kill followed by start)")
-restart_parser.add_argument('--log-file', metavar='FILE', type=str,
-                            help="Direct daemon stdout/stderr to FILE")
-restart_parser.add_argument('flags', metavar='FLAG', nargs='*', type=str,
-                            help="Regular mypy flags (precede with --)")
+kill_parser = p = subparsers.add_parser('kill', help="Kill daemon (kills the process)")
 
-check_parser = subparsers.add_parser('check', help="Check some files (requires running daemon)")
-check_parser.add_argument('-q', '--quiet', action='store_true',
-                          help="Suppress instrumentation stats")
-check_parser.add_argument('files', metavar='FILE', nargs='+', help="File (or directory) to check")
+check_parser = p = subparsers.add_parser('check',
+                                         help="Check some files (requires running daemon)")
+p.add_argument('-v', '--verbose', action='store_true', help="Print detailed status")
+p.add_argument('-q', '--quiet', action='store_true', help=argparse.SUPPRESS)  # Deprecated
+p.add_argument('files', metavar='FILE', nargs='+', help="File (or directory) to check")
 
-recheck_parser = subparsers.add_parser('recheck',
+recheck_parser = p = subparsers.add_parser('recheck',
     help="Check the same files as the most previous  check run (requires running daemon)")
-recheck_parser.add_argument('-q', '--quiet', action='store_true',
-                            help="Suppress instrumentation stats")
+p.add_argument('-v', '--verbose', action='store_true', help="Print detailed status")
+p.add_argument('-q', '--quiet', action='store_true', help=argparse.SUPPRESS)  # Deprecated
+
+hang_parser = p = subparsers.add_parser('hang', help="Hang for 100 seconds")
 
-hang_parser = subparsers.add_parser('hang', help="Hang for 100 seconds")
+daemon_parser = p = subparsers.add_parser('daemon', help="Run daemon in foreground")
+p.add_argument('flags', metavar='FLAG', nargs='*', type=str,
+               help="Regular mypy flags (precede with --)")
 
-daemon_parser = subparsers.add_parser('daemon', help="Run daemon in foreground")
-daemon_parser.add_argument('flags', metavar='FLAG', nargs='*', type=str,
-                           help="Regular mypy flags (precede with --)")
+help_parser = p = subparsers.add_parser('help')
 
-help_parser = subparsers.add_parser('help')
+del p
+
+
+class BadStatus(Exception):
+    pass
 
 
 def main() -> None:
@@ -70,7 +78,10 @@ def main() -> None:
     if not args.action:
         parser.print_usage()
     else:
-        args.action(args)
+        try:
+            args.action(args)
+        except BadStatus as err:
+            sys.exit(err.args[0])
 
 
 ActionFunction = Callable[[argparse.Namespace], None]
@@ -84,7 +95,6 @@ def register(func: ActionFunction) -> None:
 
 
 # Action functions (run in client from command line).
-# TODO: Use a separate exception instead of SystemExit to indicate failures.
 
 @action(start_parser)
 def do_start(args: argparse.Namespace) -> None:
@@ -99,79 +109,35 @@ def do_start(args: argparse.Namespace) -> None:
     """
     try:
         pid, sockname = get_status()
-    except SystemExit as err:
+    except BadStatus as err:
         # Lazy import so this import doesn't slow down other commands.
         from mypy.dmypy_server import daemonize, Server
-        if daemonize(Server(args.flags).serve, args.log_file):
+        if daemonize(Server(args.flags).serve, args.log_file) != 0:
             sys.exit(1)
         wait_for_server()
     else:
         sys.exit("Daemon is still alive")
 
 
-@action(status_parser)
-def do_status(args: argparse.Namespace) -> None:
-    """Print daemon status.
-
-    This verifies that it is responsive to requests.
-    """
-    status = read_status()
-    show_stats(status)
-    check_status(status)
-    try:
-        response = request('status')
-    except Exception as err:
-        print("Daemon is stuck; consider %s kill" % sys.argv[0])
-        raise
-    else:
-        show_stats(response)
-
-
-@action(stop_parser)
-def do_stop(args: argparse.Namespace) -> None:
-    """Stop daemon politely (via a request)."""
-    try:
-        response = request('stop')
-    except Exception as err:
-        sys.exit("Daemon is stuck; consider %s kill" % sys.argv[0])
-    else:
-        if response:
-            print("Stop response:", response)
-        else:
-            print("Daemon stopped")
-
-
-@action(kill_parser)
-def do_kill(args: argparse.Namespace) -> None:
-    """Kill daemon rudely (by killing the process)."""
-    pid, sockname = get_status()
-    try:
-        os.kill(pid, signal.SIGKILL)
-    except os.error as err:
-        sys.exit(str(err))
-    else:
-        print("Daemon killed")
-
-
 @action(restart_parser)
 def do_restart(args: argparse.Namespace) -> None:
-    """Restart daemon.
+    """Restart daemon (it may or may not be running; but not hanging).
 
     We first try to stop it politely if it's running.  This also sets
-    mypy flags (and has the same issues as start).
+    mypy flags (like start).
     """
     try:
         response = request('stop')
-    except SystemExit:
+    except BadStatus:
         pass
     else:
-        if response:
+        if response != {}:
             sys.exit("Status: %s" % str(response))
         else:
             print("Daemon stopped")
     # Lazy import so this import doesn't slow down other commands.
     from mypy.dmypy_server import daemonize, Server
-    if daemonize(Server(args.flags).serve, args.log_file):
+    if daemonize(Server(args.flags).serve, args.log_file) != 0:
         sys.exit(1)
     wait_for_server()
 
@@ -185,7 +151,7 @@ def wait_for_server(timeout: float = 5.0) -> None:
     while time.time() < endtime:
         try:
             data = read_status()
-        except SystemExit:
+        except BadStatus:
             # If the file isn't there yet, retry later.
             time.sleep(0.1)
             continue
@@ -196,6 +162,59 @@ def wait_for_server(timeout: float = 5.0) -> None:
     sys.exit("Timed out waiting for daemon to start")
 
 
+@action(status_parser)
+def do_status(args: argparse.Namespace) -> None:
+    """Print daemon status.
+
+    This verifies that it is responsive to requests.
+    """
+    status = read_status()
+    if args.verbose:
+        show_stats(status)
+    check_status(status)
+    try:
+        response = request('status', timeout=5)
+    except BadStatus:
+        raise
+    except Exception as err:
+        print("Daemon is stuck; consider %s kill" % sys.argv[0])
+        raise
+    else:
+        if args.verbose:
+            show_stats(response)
+        if 'error' in response:
+            sys.exit(response['error'])
+        print("Daemon is up and running")
+
+
+@action(stop_parser)
+def do_stop(args: argparse.Namespace) -> None:
+    """Stop daemon via a 'stop' request."""
+    try:
+        response = request('stop', timeout=5)
+    except BadStatus:
+        raise
+    except Exception as err:
+        sys.exit("Daemon is stuck; consider %s kill" % sys.argv[0])
+    else:
+        if response:
+            print("Stop response:", response)
+        else:
+            print("Daemon stopped")
+
+
+@action(kill_parser)
+def do_kill(args: argparse.Namespace) -> None:
+    """Kill daemon process with SIGKILL."""
+    pid, sockname = get_status()
+    try:
+        os.kill(pid, signal.SIGKILL)
+    except OSError as err:
+        sys.exit(str(err))
+    else:
+        print("Daemon killed")
+
+
 @action(check_parser)
 def do_check(args: argparse.Namespace) -> None:
     """Ask the daemon to check a list of files."""
@@ -203,7 +222,7 @@ def do_check(args: argparse.Namespace) -> None:
     response = request('check', files=args.files)
     t1 = time.time()
     response['roundtrip_time'] = t1 - t0
-    check_output(response, args.quiet)
+    check_output(response, args.verbose)
 
 
 @action(recheck_parser)
@@ -216,33 +235,43 @@ def do_recheck(args: argparse.Namespace) -> None:
     response = request('recheck')
     t1 = time.time()
     response['roundtrip_time'] = t1 - t0
-    check_output(response, args.quiet)
+    check_output(response, args.verbose)
+
 
+def check_output(response: Dict[str, Any], verbose: bool) -> None:
+    """Print the output from a check or recheck command.
 
-def check_output(response: Dict[str, Any], quiet: bool) -> None:
-    """Print the output from a check or recheck command."""
+    Call sys.exit() unless the status code is zero.
+    """
+    if 'error' in response:
+        sys.exit(response['error'])
     try:
-        out, err, status = response['out'], response['err'], response['status']
+        out, err, status_code = response['out'], response['err'], response['status']
     except KeyError:
         sys.exit("Response: %s" % str(response))
     sys.stdout.write(out)
     sys.stderr.write(err)
-    if not quiet:
+    if verbose:
         show_stats(response)
-    if status:
-        sys.exit(status)
+    if status_code:
+        sys.exit(status_code)
 
 
 def show_stats(response: Mapping[str, object]) -> None:
     for key, value in sorted(response.items()):
         if key not in ('out', 'err'):
             print("%-24s: %10s" % (key, "%.3f" % value if isinstance(value, float) else value))
+        else:
+            value = str(value).replace('\n', '\\n')
+            if len(value) > 50:
+                value = value[:40] + ' ...'
+            print("%-24s: %s" % (key, value))
 
 
 @action(hang_parser)
 def do_hang(args: argparse.Namespace) -> None:
     """Hang for 100 seconds, as a debug hack."""
-    request('hang')
+    print(request('hang', timeout=1))
 
 
 @action(daemon_parser)
@@ -262,24 +291,32 @@ def do_help(args: argparse.Namespace) -> None:
 # Client-side infrastructure.
 
 
-def request(command: str, **kwds: object) -> Dict[str, Any]:
+def request(command: str, *, timeout: Optional[float] = None,
+            **kwds: object) -> Dict[str, Any]:
     """Send a request to the daemon.
 
     Return the JSON dict with the response.
+
+    Raise BadStatus if there is something wrong with the status file.
+
+    Return {'error': <message>} if there was something wrong with the
+    response (including OSError raised by a socket operation).
     """
     args = dict(kwds)
-    if command:
-        args.update(command=command)
-    data = json.dumps(args)
+    args.update(command=command)
+    bdata = json.dumps(args).encode('utf8')
     pid, sockname = get_status()
     sock = socket.socket(socket.AF_UNIX)
-    sock.connect(sockname)
-    sock.sendall(data.encode('utf8'))
-    sock.shutdown(socket.SHUT_WR)
+    if timeout is not None:
+        sock.settimeout(timeout)
     try:
+        sock.connect(sockname)
+        sock.sendall(bdata)
+        sock.shutdown(socket.SHUT_WR)
         response = receive(sock)
     except OSError as err:
         return {'error': str(err)}
+    # TODO: Other errors, e.g. ValueError, UnicodeError
     else:
         return response
     finally:
@@ -291,7 +328,7 @@ def get_status() -> Tuple[int, str]:
 
     Return (pid, sockname) on success.
 
-    Raise SystemExit(<message>) if something's wrong.
+    Raise BadStatus if something's wrong.
     """
     data = read_status()
     return check_status(data)
@@ -302,36 +339,40 @@ def check_status(data: Dict[str, Any]) -> Tuple[int, str]:
 
     Return (pid, sockname) on success.
 
-    Raise SystemExit(<message>) if something's wrong.
+    Raise BadStatus if something's wrong.
     """
     if 'pid' not in data:
-        raise SystemExit("Invalid status file (no pid field)")
+        raise BadStatus("Invalid status file (no pid field)")
     pid = data['pid']
     if not isinstance(pid, int):
-        raise SystemExit("pid field is not an int")
+        raise BadStatus("pid field is not an int")
     try:
         os.kill(pid, 0)
     except OSError as err:
-        raise SystemExit("Daemon has died")
+        raise BadStatus("Daemon has died")
     if 'sockname' not in data:
-        raise SystemExit("Invalid status file (no sockname field)")
+        raise BadStatus("Invalid status file (no sockname field)")
     sockname = data['sockname']
     if not isinstance(sockname, str):
-        raise SystemExit("sockname field is not a string")
+        raise BadStatus("sockname field is not a string")
     return pid, sockname
 
 
 def read_status() -> Dict[str, object]:
-    """Read status file."""
+    """Read status file.
+
+    Raise BadStatus if the status file doesn't exist or contains
+    invalid JSON or the JSON is not a dict.
+    """
     if not os.path.isfile(STATUS_FILE):
-        raise SystemExit("No status file found")
+        raise BadStatus("No status file found")
     with open(STATUS_FILE) as f:
         try:
             data = json.load(f)
         except Exception as err:
-            raise SystemExit("Malformed status file (not JSON)")
+            raise BadStatus("Malformed status file (not JSON)")
     if not isinstance(data, dict):
-        raise SystemExit("Invalid status file (not a dict)")
+        raise BadStatus("Invalid status file (not a dict)")
     return data
 
 
diff --git a/mypy/dmypy_server.py b/mypy/dmypy_server.py
index a1b40595dc0c..a873823db75e 100644
--- a/mypy/dmypy_server.py
+++ b/mypy/dmypy_server.py
@@ -16,11 +16,11 @@
 
 from typing import Any, Callable, Dict, List, Mapping, Optional, Sequence
 
-# TODO: Import all mypy modules lazily to speed up client startup time.
 import mypy.build
 import mypy.errors
 import mypy.main
 from mypy.dmypy_util import STATUS_FILE, receive
+from mypy.gclogger import GcLogger
 
 
 def daemonize(func: Callable[[], None], log_file: Optional[str] = None) -> int:
@@ -221,48 +221,6 @@ def cmd_hang(self) -> Dict[str, object]:
 # Misc utilities.
 
 
-class GcLogger:
-    """Context manager to log GC stats and overall time."""
-
-    def __enter__(self) -> 'GcLogger':
-        self.gc_start_time = None  # type: Optional[float]
-        self.gc_time = 0.0
-        self.gc_calls = 0
-        self.gc_collected = 0
-        self.gc_uncollectable = 0
-        gc.callbacks.append(self.gc_callback)
-        self.start_time = time.time()
-        return self
-
-    def gc_callback(self, phase: str, info: Mapping[str, int]) -> None:
-        if phase == 'start':
-            assert self.gc_start_time is None, "Start phase out of sequence"
-            self.gc_start_time = time.time()
-        elif phase == 'stop':
-            assert self.gc_start_time is not None, "Stop phase out of sequence"
-            self.gc_calls += 1
-            self.gc_time += time.time() - self.gc_start_time
-            self.gc_start_time = None
-            self.gc_collected += info['collected']
-            self.gc_uncollectable += info['uncollectable']
-        else:
-            assert False, "Unrecognized gc phase (%r)" % (phase,)
-
-    def __exit__(self, *args: object) -> None:
-        while self.gc_callback in gc.callbacks:
-            gc.callbacks.remove(self.gc_callback)
-
-    def get_stats(self) -> Dict[str, float]:
-        end_time = time.time()
-        result = {}
-        result['gc_time'] = self.gc_time
-        result['gc_calls'] = self.gc_calls
-        result['gc_collected'] = self.gc_collected
-        result['gc_uncollectable'] = self.gc_uncollectable
-        result['build_time'] = end_time - self.start_time
-        return result
-
-
 MiB = 2**20
 
 
diff --git a/mypy/dmypy_util.py b/mypy/dmypy_util.py
index e2cc87f56815..7ccef77a229b 100644
--- a/mypy/dmypy_util.py
+++ b/mypy/dmypy_util.py
@@ -12,7 +12,13 @@
 
 
 def receive(sock: socket.socket) -> Any:
-    """Receive JSON data from a socket until EOF."""
+    """Receive JSON data from a socket until EOF.
+
+    Raise a subclass of OSError if there's a socket exception.
+
+    Raise OSError if the data received is not valid JSON or if it is
+    not a dict.
+    """
     bdata = bytearray()
     while True:
         more = sock.recv(100000)
@@ -21,7 +27,10 @@ def receive(sock: socket.socket) -> Any:
         bdata.extend(more)
     if not bdata:
         raise OSError("No data received")
-    data = json.loads(bdata.decode('utf8'))
+    try:
+        data = json.loads(bdata.decode('utf8'))
+    except Exception:
+        raise OSError("Data received is not valid JSON")
     if not isinstance(data, dict):
         raise OSError("Data received is not a dict (%s)" % str(type(data)))
     return data
diff --git a/mypy/gclogger.py b/mypy/gclogger.py
new file mode 100644
index 000000000000..650ef2f04930
--- /dev/null
+++ b/mypy/gclogger.py
@@ -0,0 +1,46 @@
+import gc
+import time
+
+from typing import Mapping, Optional
+
+
+class GcLogger:
+    """Context manager to log GC stats and overall time."""
+
+    def __enter__(self) -> 'GcLogger':
+        self.gc_start_time = None  # type: Optional[float]
+        self.gc_time = 0.0
+        self.gc_calls = 0
+        self.gc_collected = 0
+        self.gc_uncollectable = 0
+        gc.callbacks.append(self.gc_callback)
+        self.start_time = time.time()
+        return self
+
+    def gc_callback(self, phase: str, info: Mapping[str, int]) -> None:
+        if phase == 'start':
+            assert self.gc_start_time is None, "Start phase out of sequence"
+            self.gc_start_time = time.time()
+        elif phase == 'stop':
+            assert self.gc_start_time is not None, "Stop phase out of sequence"
+            self.gc_calls += 1
+            self.gc_time += time.time() - self.gc_start_time
+            self.gc_start_time = None
+            self.gc_collected += info['collected']
+            self.gc_uncollectable += info['uncollectable']
+        else:
+            assert False, "Unrecognized gc phase (%r)" % (phase,)
+
+    def __exit__(self, *args: object) -> None:
+        while self.gc_callback in gc.callbacks:
+            gc.callbacks.remove(self.gc_callback)
+
+    def get_stats(self) -> Mapping[str, float]:
+        end_time = time.time()
+        result = {}
+        result['gc_time'] = self.gc_time
+        result['gc_calls'] = self.gc_calls
+        result['gc_collected'] = self.gc_collected
+        result['gc_uncollectable'] = self.gc_uncollectable
+        result['build_time'] = end_time - self.start_time
+        return result


Code review comments:
diff_hunk: @@ -196,14 +162,67 @@ def wait_for_server(timeout: float = 5.0) -> None:
     sys.exit("Timed out waiting for daemon to start")
 
 
+@action(status_parser)
+def do_status(args: argparse.Namespace) -> None:
+    """Print daemon status.
+
+    This verifies that it is responsive to requests.
+    """
+    status = read_status()
+    if args.verbose:
+        show_stats(status)
+    check_status(status)
+    try:
+        response = request('status', timeout=5)
+    except BadStatus:
+        raise
+    except Exception as err:
+        print("Daemon is stuck; consider %s kill" % sys.argv[0])
path: mypy/dmypy.py
text: @user1:
Should we display some information about the exception, since we are catching arbitrary exceptions? An exception could be caused by a bug in dmypy.