From 14ca4f07d1e2d63c045edebcbd83e6bd1da988d3 Mon Sep 17 00:00:00 2001 From: Pavel Minaev Date: Tue, 1 Sep 2020 11:37:01 -0700 Subject: [PATCH] Fix #351: Python warnings in debugger code Don't use inspect.getargspec on Python 2. Close log file objects on exit. Close os.devnull file objects on exit. Close the listener socket used to get endpoints info from adapter. Fix invalid escape sequences. Run tests with Python warnings treated as errors. Fix Django deprecation warning in test web app. Work around pytest issues caused spaces in test names. --- src/debugpy/adapter/__main__.py | 3 +- src/debugpy/adapter/clients.py | 7 +- src/debugpy/common/compat.py | 8 +- src/debugpy/common/log.py | 20 +++-- src/debugpy/server/api.py | 133 +++++++++++++++++--------------- src/debugpy/server/cli.py | 2 +- tests/debug/session.py | 1 + tests/debugpy/test_run.py | 4 +- tests/test_data/django1/app.py | 1 + 9 files changed, 105 insertions(+), 74 deletions(-) diff --git a/src/debugpy/adapter/__main__.py b/src/debugpy/adapter/__main__.py index a250561a..956be80c 100644 --- a/src/debugpy/adapter/__main__.py +++ b/src/debugpy/adapter/__main__.py @@ -24,7 +24,8 @@ def main(args): # so disable it to avoid the pipe filling and locking up. This must be done # as early as possible, before the logging module starts writing to it. if args.port is None: - sys.stderr = open(os.devnull, "w") + sys.stderr = stderr = open(os.devnull, "w") + atexit.register(stderr.close) from debugpy import adapter from debugpy.common import compat, log, sockets diff --git a/src/debugpy/adapter/clients.py b/src/debugpy/adapter/clients.py index 47d0f3bd..7500e0cf 100644 --- a/src/debugpy/adapter/clients.py +++ b/src/debugpy/adapter/clients.py @@ -4,6 +4,7 @@ from __future__ import absolute_import, division, print_function, unicode_literals +import atexit import os import sys @@ -41,8 +42,10 @@ class Client(components.Component): stream = messaging.JsonIOStream.from_stdio() # Make sure that nothing else tries to interfere with the stdio streams # that are going to be used for DAP communication from now on. - sys.stdin = open(os.devnull, "r") - sys.stdout = open(os.devnull, "w") + sys.stdin = stdin = open(os.devnull, "r") + atexit.register(stdin.close) + sys.stdout = stdout = open(os.devnull, "w") + atexit.register(stdout.close) else: stream = messaging.JsonIOStream.from_socket(sock) diff --git a/src/debugpy/common/compat.py b/src/debugpy/common/compat.py index d836aee2..81104f5c 100644 --- a/src/debugpy/common/compat.py +++ b/src/debugpy/common/compat.py @@ -183,7 +183,13 @@ def kwonly(f): If the default value is kwonly.required, then the argument must be specified. """ - arg_names, args_name, kwargs_name, arg_defaults = inspect.getargspec(f) + try: + inspect.getfullargspec + except AttributeError: + arg_names, args_name, kwargs_name, arg_defaults = inspect.getargspec(f) + else: + arg_names, args_name, kwargs_name, arg_defaults, _, _, _ = inspect.getfullargspec(f) + assert args_name is None and kwargs_name is None argc = len(arg_names) pos_argc = argc - len(arg_defaults) diff --git a/src/debugpy/common/log.py b/src/debugpy/common/log.py index 35e7c61e..17c856da 100644 --- a/src/debugpy/common/log.py +++ b/src/debugpy/common/log.py @@ -4,6 +4,7 @@ from __future__ import absolute_import, division, print_function, unicode_literals +import atexit import contextlib import functools import inspect @@ -43,11 +44,12 @@ def _update_levels(): class LogFile(object): - def __init__(self, filename, file, levels=LEVELS): + def __init__(self, filename, file, levels=LEVELS, close_file=True): info("Also logging to {0!j}.", filename) self.filename = filename self.file = file + self.close_file = close_file self._levels = frozenset(levels) with _lock: @@ -88,10 +90,11 @@ class LogFile(object): _update_levels() info("Not logging to {0!j} anymore.", self.filename) - try: - self.file.close() - except Exception: - pass + if self.close_file: + try: + self.file.close() + except Exception: + pass def __enter__(self): return self @@ -350,9 +353,16 @@ stderr = LogFile( "", sys.stderr, levels=os.getenv("DEBUGPY_LOG_STDERR", "warning error").split(), + close_file=False, ) +@atexit.register +def _close_files(): + for file in tuple(_files.values()): + file.close() + + # The following are helper shortcuts for printf debugging. They must never be used # in production code. diff --git a/src/debugpy/server/api.py b/src/debugpy/server/api.py index b1cf20a7..4c939a93 100644 --- a/src/debugpy/server/api.py +++ b/src/debugpy/server/api.py @@ -41,7 +41,7 @@ _adapter_process = None def _settrace(*args, **kwargs): log.debug("pydevd.settrace(*{0!r}, **{1!r})", args, kwargs) # The stdin in notification is not acted upon in debugpy, so, disable it. - kwargs.setdefault('notify_stdin', False) + kwargs.setdefault("notify_stdin", False) try: return pydevd.settrace(*args, **kwargs) except Exception: @@ -159,74 +159,83 @@ def listen(address, settrace_kwargs): except Exception as exc: log.swallow_exception("Can't listen for adapter endpoints:") raise RuntimeError("can't listen for adapter endpoints: " + str(exc)) - endpoints_host, endpoints_port = endpoints_listener.getsockname() - log.info( - "Waiting for adapter endpoints on {0}:{1}...", endpoints_host, endpoints_port - ) - host, port = address - adapter_args = [ - sys.executable, - os.path.dirname(adapter.__file__), - "--for-server", - str(endpoints_port), - "--host", - host, - "--port", - str(port), - "--server-access-token", - server_access_token, - ] - if log.log_dir is not None: - adapter_args += ["--log-dir", log.log_dir] - log.info("debugpy.listen() spawning adapter: {0!j}", adapter_args) - - # On Windows, detach the adapter from our console, if any, so that it doesn't - # receive Ctrl+C from it, and doesn't keep it open once we exit. - creationflags = 0 - if sys.platform == "win32": - creationflags |= 0x08000000 # CREATE_NO_WINDOW - creationflags |= 0x00000200 # CREATE_NEW_PROCESS_GROUP - - # Adapter will outlive this process, so we shouldn't wait for it. However, we - # need to ensure that the Popen instance for it doesn't get garbage-collected - # by holding a reference to it in a non-local variable, to avoid triggering - # https://bugs.python.org/issue37380. try: - global _adapter_process - _adapter_process = subprocess.Popen( - adapter_args, close_fds=True, creationflags=creationflags + endpoints_host, endpoints_port = endpoints_listener.getsockname() + log.info( + "Waiting for adapter endpoints on {0}:{1}...", + endpoints_host, + endpoints_port, ) - if os.name == "posix": - # It's going to fork again to daemonize, so we need to wait on it to - # clean it up properly. - _adapter_process.wait() - else: - # Suppress misleading warning about child process still being alive when - # this process exits (https://bugs.python.org/issue38890). - _adapter_process.returncode = 0 - pydevd.add_dont_terminate_child_pid(_adapter_process.pid) - except Exception as exc: - log.swallow_exception("Error spawning debug adapter:", level="info") - raise RuntimeError("error spawning debug adapter: " + str(exc)) - try: - sock, _ = endpoints_listener.accept() + host, port = address + adapter_args = [ + sys.executable, + os.path.dirname(adapter.__file__), + "--for-server", + str(endpoints_port), + "--host", + host, + "--port", + str(port), + "--server-access-token", + server_access_token, + ] + if log.log_dir is not None: + adapter_args += ["--log-dir", log.log_dir] + log.info("debugpy.listen() spawning adapter: {0!j}", adapter_args) + + # On Windows, detach the adapter from our console, if any, so that it doesn't + # receive Ctrl+C from it, and doesn't keep it open once we exit. + creationflags = 0 + if sys.platform == "win32": + creationflags |= 0x08000000 # CREATE_NO_WINDOW + creationflags |= 0x00000200 # CREATE_NEW_PROCESS_GROUP + + # Adapter will outlive this process, so we shouldn't wait for it. However, we + # need to ensure that the Popen instance for it doesn't get garbage-collected + # by holding a reference to it in a non-local variable, to avoid triggering + # https://bugs.python.org/issue37380. try: - sock.settimeout(None) - sock_io = sock.makefile("rb", 0) + global _adapter_process + _adapter_process = subprocess.Popen( + adapter_args, close_fds=True, creationflags=creationflags + ) + if os.name == "posix": + # It's going to fork again to daemonize, so we need to wait on it to + # clean it up properly. + _adapter_process.wait() + else: + # Suppress misleading warning about child process still being alive when + # this process exits (https://bugs.python.org/issue38890). + _adapter_process.returncode = 0 + pydevd.add_dont_terminate_child_pid(_adapter_process.pid) + except Exception as exc: + log.swallow_exception("Error spawning debug adapter:", level="info") + raise RuntimeError("error spawning debug adapter: " + str(exc)) + + try: + sock, _ = endpoints_listener.accept() try: - endpoints = json.loads(sock_io.read().decode("utf-8")) + sock.settimeout(None) + sock_io = sock.makefile("rb", 0) + try: + endpoints = json.loads(sock_io.read().decode("utf-8")) + finally: + sock_io.close() finally: - sock_io.close() - finally: - sockets.close_socket(sock) - except socket.timeout: - log.swallow_exception("Timed out waiting for adapter to connect:", level="info") - raise RuntimeError("timed out waiting for adapter to connect") - except Exception as exc: - log.swallow_exception("Error retrieving adapter endpoints:", level="info") - raise RuntimeError("error retrieving adapter endpoints: " + str(exc)) + sockets.close_socket(sock) + except socket.timeout: + log.swallow_exception( + "Timed out waiting for adapter to connect:", level="info" + ) + raise RuntimeError("timed out waiting for adapter to connect") + except Exception as exc: + log.swallow_exception("Error retrieving adapter endpoints:", level="info") + raise RuntimeError("error retrieving adapter endpoints: " + str(exc)) + + finally: + endpoints_listener.close() log.info("Endpoints received from adapter: {0!j}", endpoints) diff --git a/src/debugpy/server/cli.py b/src/debugpy/server/cli.py index 13c93456..1e1df210 100644 --- a/src/debugpy/server/cli.py +++ b/src/debugpy/server/cli.py @@ -152,7 +152,7 @@ switches = [ # ====== =========== ====== # Switches that are documented for use by end users. - ("-(\?|h|-help)", None, print_help_and_exit), + ("-(\\?|h|-help)", None, print_help_and_exit), ("-(V|-version)", None, print_version_and_exit), ("--log-to" , "", set_arg("log_to")), ("--log-to-stderr", None, set_const("log_to_stderr", True)), diff --git a/tests/debug/session.py b/tests/debug/session.py index 2b391063..9fb0608a 100644 --- a/tests/debug/session.py +++ b/tests/debug/session.py @@ -310,6 +310,7 @@ class Session(object): env.update(base_env) env["PYTHONUNBUFFERED"] = "1" + env["PYTHONWARNINGS"] = "error" env["DEBUGPY_TEST_SESSION_ID"] = str(self.id) env.prepend_to("PYTHONPATH", DEBUGGEE_PYTHONPATH.strpath) diff --git a/tests/debugpy/test_run.py b/tests/debugpy/test_run.py index d7a0c6b1..b7ab5fed 100644 --- a/tests/debugpy/test_run.py +++ b/tests/debugpy/test_run.py @@ -139,7 +139,7 @@ def test_sudo(pyfile, tmpdir, run, target): @pytest.mark.parametrize("run", runners.all_launch_terminal) @pytest.mark.parametrize("python_args", ["", "-v"]) -@pytest.mark.parametrize("python", ["", "custompy", "custompy -O"]) +@pytest.mark.parametrize("python", ["", "custompy", "custompy|-O"]) @pytest.mark.parametrize("python_key", ["python", "pythonPath"]) def test_custom_python(pyfile, run, target, python_key, python, python_args): @pyfile @@ -151,7 +151,7 @@ def test_custom_python(pyfile, run, target, python_key, python, python_args): debuggee.setup() backchannel.send([sys.executable, sys.flags.optimize, sys.flags.verbose]) - python = python.split() + python = python.split("|") python_args = python_args.split() python_cmd = (python if len(python) else [sys.executable]) + python_args diff --git a/tests/test_data/django1/app.py b/tests/test_data/django1/app.py index aa40b091..d4c1e569 100644 --- a/tests/test_data/django1/app.py +++ b/tests/test_data/django1/app.py @@ -23,6 +23,7 @@ def sigint_handler(signal, frame): signal.signal(signal.SIGINT, sigint_handler) settings.configure( + MIDDLEWARE=[], DEBUG=True, SECRET_KEY="CD8FF4C1-7E6C-4E45-922D-C796271F2345", ROOT_URLCONF=sys.modules[__name__],