From 47ef53da1750bde1f93e42ae8dd5cc76ae6ec97b Mon Sep 17 00:00:00 2001 From: John Sirois Date: Sun, 19 Jun 2022 15:18:30 -0700 Subject: [PATCH 1/3] Simplify `PEX` teardown / leave stderr in tact. For historical reasons that are no longer relevant, `PEX` accrued complicated teardown code that, among other things, replaced `sys.stderr` with `/dev/null`. This could lead to lost messages to stderr. Simply remove the complicated and no-longer useful code. Fixes #1802 --- pex/pex.py | 97 +++++++++------------------- pex/variables.py | 5 ++ tests/integration/test_issue_1802.py | 96 +++++++++++++++++++++++++++ tests/test_pex.py | 8 --- 4 files changed, 131 insertions(+), 75 deletions(-) create mode 100644 tests/integration/test_issue_1802.py diff --git a/pex/pex.py b/pex/pex.py index e10fbe88a..b57fceeaf 100644 --- a/pex/pex.py +++ b/pex/pex.py @@ -5,10 +5,7 @@ import ast import os -import re import sys -import warnings -import zipfile from distutils import sysconfig from site import USER_SITE from types import ModuleType @@ -16,7 +13,6 @@ from pex import third_party from pex.bootstrap import Bootstrap from pex.common import die -from pex.compatibility import PY3 from pex.dist_metadata import CallableEntryPoint, Distribution, EntryPoint, find_distributions from pex.environment import PEXEnvironment from pex.executor import Executor @@ -485,72 +481,39 @@ def execute(self): This function makes assumptions that it is the last function called by the interpreter. """ - teardown_verbosity = self._vars.PEX_TEARDOWN_VERBOSE - try: - if self._vars.PEX_TOOLS: - if not self._pex_info.includes_tools: - die( - "The PEX_TOOLS environment variable was set, but this PEX was not built " - "with tools (Re-build the PEX file with `pex --include-tools ...`)" - ) + if self._vars.PEX_TOOLS: + if not self._pex_info.includes_tools: + die( + "The PEX_TOOLS environment variable was set, but this PEX was not built " + "with tools (Re-build the PEX file with `pex --include-tools ...`)" + ) - from pex.tools import main as tools + from pex.tools import main as tools - exit_value = tools.main(pex=PEX(sys.argv[0])) - else: - self.activate() - - pex_file = os.environ.get("PEX", None) - if pex_file: - try: - from setproctitle import setproctitle # type: ignore[import] - - setproctitle( - "{python} {pex_file} {args}".format( - python=sys.executable, - pex_file=pex_file, - args=" ".join(sys.argv[1:]), - ) - ) - except ImportError: - TRACER.log( - "Not setting process title since setproctitle is not available in " - "{pex_file}".format(pex_file=pex_file), - V=3, - ) - - exit_value = self._wrap_coverage(self._wrap_profiling, self._execute) - sys.exit(exit_value) - except Exception: - # Allow the current sys.excepthook to handle this app exception before we tear things - # down in finally, then reraise so that the exit status is reflected correctly. - sys.excepthook(*sys.exc_info()) - raise - except SystemExit as se: - # Print a SystemExit error message, avoiding a traceback in python3. - # This must happen here, as sys.stderr is about to be torn down - if not isinstance(se.code, int) and se.code is not None: # type: ignore[unreachable] - print(se.code, file=sys.stderr) # type: ignore[unreachable] - raise - finally: - # squash all exceptions on interpreter teardown -- the primary type here are - # atexit handlers failing to run because of things such as: - # http://stackoverflow.com/questions/2572172/referencing-other-modules-in-atexit - if not teardown_verbosity: - sys.stderr.flush() - sys.stderr = open(os.devnull, "w") - if PY3: - # Python 3 warns about unclosed resources. In this case we intentionally do not - # close `/dev/null` since we want all stderr to flow there until the latest - # stages of Python interpreter shutdown when the Python runtime will `del` the - # open file and thus finally close the underlying file descriptor. As such, - # suppress the warning. - warnings.filterwarnings( - action="ignore", - message=r"unclosed file {}".format(re.escape(str(sys.stderr))), - category=ResourceWarning, + sys.exit(tools.main(pex=PEX(sys.argv[0]))) + + self.activate() + + pex_file = os.environ.get("PEX", None) + if pex_file: + try: + from setproctitle import setproctitle # type: ignore[import] + + setproctitle( + "{python} {pex_file} {args}".format( + python=sys.executable, + pex_file=pex_file, + args=" ".join(sys.argv[1:]), ) - sys.excepthook = lambda *a, **kw: None + ) + except ImportError: + TRACER.log( + "Not setting process title since setproctitle is not available in " + "{pex_file}".format(pex_file=pex_file), + V=3, + ) + + sys.exit(self._wrap_coverage(self._wrap_profiling, self._execute)) def _execute(self): # type: () -> Any diff --git a/pex/variables.py b/pex/variables.py index 61afd6568..3cc95aebb 100644 --- a/pex/variables.py +++ b/pex/variables.py @@ -235,6 +235,11 @@ def __init__(self, environ=None, rc=None): "The `PEX_UNZIP` env var is deprecated. This env var is no longer read since " "unzipping PEX zip files before execution is now the default." ) + if "PEX_TEARDOWN_VERBOSE" in self._environ: + pex_warnings.warn( + # TODO(John Sirois): XXX + "" + ) def copy(self): # type: () -> Dict[str, str] diff --git a/tests/integration/test_issue_1802.py b/tests/integration/test_issue_1802.py new file mode 100644 index 000000000..a092748d1 --- /dev/null +++ b/tests/integration/test_issue_1802.py @@ -0,0 +1,96 @@ +# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md). +# Licensed under the Apache License, Version 2.0 (see LICENSE). +import os.path +import subprocess +from textwrap import dedent + +import pytest + +from pex.common import safe_open +from pex.compatibility import PY2 +from pex.testing import run_pex_command +from pex.typing import TYPE_CHECKING + +if TYPE_CHECKING: + from typing import Any + + +@pytest.mark.skipif(PY2, reason="Example code used to drive test is Python 3 only.") +def test_stderr_not_torn_down(tmpdir): + # type: (Any) -> None + + exe = os.path.join(str(tmpdir), "exe") + with safe_open(exe, "w") as fp: + fp.write( + dedent( + """\ + import sys + import logging + import atexit + import logging.handlers + import queue + import sys + import faulthandler + + import absl.app as absl_app + import absl.logging as absl_logging + from absl.flags import FLAGS + + + def run(): + print("hello") + absl_logging.error("HELP ME") + + + def init_sys_logging(): + root_logger = logging.getLogger() + + FLAGS.alsologtostderr = True + + # No limit on queue size. + log_queue = queue.Queue(-1) + queue_forwarder = logging.handlers.QueueHandler(log_queue) + root_logger.addHandler(queue_forwarder) + + queue_handlers = [] + + # If absl logging is enabled; re-parent it to the queue. + absl_handler = absl_logging.get_absl_handler() + if absl_handler in root_logger.handlers: + root_logger.handlers.remove(absl_handler) + queue_handlers.append(absl_handler) + + queue_log_listener = logging.handlers.QueueListener( + log_queue, *queue_handlers, respect_handler_level=True + ) + queue_log_listener.start() + + atexit.register(queue_log_listener.stop) + + FLAGS.mark_as_parsed() + + + if __name__ == "__main__": + absl_logging.set_verbosity(0) + absl_logging.use_absl_handler() + absl_logging.get_absl_handler().use_absl_log_file() + + faulthandler.enable() + + init_sys_logging() + + def run_wrapper(fn) -> int: + absl_app._run_main(lambda args: fn(), sys.argv) + return 0 + + sys.exit(run_wrapper(run)) + """ + ) + ) + pex = os.path.join(str(tmpdir), "pex") + run_pex_command(args=["absl-py==0.10.0", "--exe", exe, "-o", pex]).assert_success() + process = subprocess.Popen(args=[pex], stderr=subprocess.PIPE) + _, stderr = process.communicate() + error = stderr.decode("utf-8") + assert 0 == process.returncode + assert "HELP ME" in error diff --git a/tests/test_pex.py b/tests/test_pex.py index e602810bd..8da03c026 100644 --- a/tests/test_pex.py +++ b/tests/test_pex.py @@ -22,7 +22,6 @@ from pex.pex_builder import PEXBuilder from pex.pex_info import PexInfo from pex.testing import ( - IS_PYPY3, PY27, PY310, WheelBuilder, @@ -113,7 +112,6 @@ def test_pex_sys_exit_prints_objects(): _test_sys_exit('Exception("derp")', b"derp\n", 1) -@pytest.mark.xfail(IS_PYPY3, reason="https://github.com/pantsbuild/pex/issues/1210") def test_pex_atexit_swallowing(): # type: () -> None body = textwrap.dedent( @@ -128,12 +126,6 @@ def raise_on_exit(): ) so, rc = run_simple_pex_test(body) - assert so == b"" - assert rc == 0 - - env_copy = os.environ.copy() - env_copy.update(PEX_TEARDOWN_VERBOSE="1") - so, rc = run_simple_pex_test(body, env=env_copy) assert b"This is an exception" in so assert rc == 0 From 86803a9613266352440f52f9c77b838628562432 Mon Sep 17 00:00:00 2001 From: John Sirois Date: Sun, 19 Jun 2022 15:39:25 -0700 Subject: [PATCH 2/3] Fix dangling todo fixme. --- pex/variables.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pex/variables.py b/pex/variables.py index 3cc95aebb..08dacb89d 100644 --- a/pex/variables.py +++ b/pex/variables.py @@ -237,8 +237,9 @@ def __init__(self, environ=None, rc=None): ) if "PEX_TEARDOWN_VERBOSE" in self._environ: pex_warnings.warn( - # TODO(John Sirois): XXX - "" + "The `PEX_TEARDOWN_VERBOSE` env var is deprecated. This env var is no longer read " + "since PEX teardown has been removed in favor of the natural teardown environment " + "provided by the Python runtime." ) def copy(self): From 2c4480a2282b333093184de4942cdc25c97a3f99 Mon Sep 17 00:00:00 2001 From: John Sirois Date: Sun, 19 Jun 2022 21:53:03 -0700 Subject: [PATCH 3/3] Fix test for PyPy. --- tests/test_pex.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_pex.py b/tests/test_pex.py index 8da03c026..51f50a77b 100644 --- a/tests/test_pex.py +++ b/tests/test_pex.py @@ -22,6 +22,7 @@ from pex.pex_builder import PEXBuilder from pex.pex_info import PexInfo from pex.testing import ( + IS_PYPY, PY27, PY310, WheelBuilder, @@ -66,7 +67,6 @@ def test_excepthook_honored(): def excepthook(ex_type, ex, tb): print('Custom hook called with: {0}'.format(ex)) - sys.exit(42) sys.excepthook = excepthook @@ -76,7 +76,7 @@ def excepthook(ex_type, ex, tb): so, rc = run_simple_pex_test(body) assert so == b"Custom hook called with: This is an exception\n", "Standard out was: %r" % so - assert rc == 42 + assert rc == 1 def _test_sys_exit(arg, expected_output, expected_rc):