Make precomputing fail more usefully (pantsbuild#7994)

**Problem**
DaemonPantsRunner precomputes the v2 graph products at creation time (before run). If that raised an exception, it would store a return code of 1, run normally, and then exit with 1 at the end:

class DaemonPantsRunner:
...
  def run(self):
    ....
    else:
        self._exiter.exit(self.**exit_code** if self.exit_code else PANTS_SUCCEEDED_EXIT_CODE)
This hints that the behaviour that we want is to actually fail the run if the precomputation fails. If that is the case, we would like to fail more usefully, with a real exception.

**Solution**
Create a wrapper exception type that wraps whatever error happened in the precomputation. Then, in DPR.run(), re-raise this deferred exception immediately after setting up logging and stdio channels.

**Result**
What before seemed to succeed and returned 1, should now return a proper exception.

src/python/pants/bin/daemon_pants_runner.py

@@ -102,6 +102,30 @@ def exit_code(self):
     return self._exit_code
 
 
+class _PantsProductPrecomputeFailed(Exception):
+  """
+  Represents a failure in the precompute step of v2 products for pants runs.
+  (e.g. The user called ./pants dependees and there was a broken link)
+
+  Will be raised by the DaemonPantsRunner at creation time.
+  """
+
+  def __init__(self, wrapped_exception):
+    """
+    :param wrapped_exception: Original exception raised by the engine.
+    """
+    super(_PantsProductPrecomputeFailed, self).__init__('Product precomputation in the daemon raised an exception: {}'.format(wrapped_exception))
+
+    self.wrapped_exception = wrapped_exception
+
+  @property
+  def exit_code(self):
+    if isinstance(self.wrapped_exception, _PantsRunFinishedWithFailureException):
+      return self.wrapped_exception.exit_code
+    else:
+      return PANTS_FAILED_EXIT_CODE
+
+
 class DaemonPantsRunner:
   """A daemonizing PantsRunner that speaks the nailgun protocol to a remote client.
 
@@ -111,22 +135,32 @@ class DaemonPantsRunner:
   @classmethod
   def create(cls, sock, args, env, services, scheduler_service):
     maybe_shutdown_socket = MaybeShutdownSocket(sock)
+    exception = None
+    exit_code = PANTS_SUCCEEDED_EXIT_CODE
 
+    # TODO(#8002) This can probably be moved to the try:except block in DaemonPantsRunner.run() function,
+    #             Making exception handling a lot easier.
     try:
       # N.B. This will redirect stdio in the daemon's context to the nailgun session.
       with cls.nailgunned_stdio(maybe_shutdown_socket, env, handle_stdin=False) as finalizer:
         options, _, options_bootstrapper = LocalPantsRunner.parse_options(args, env)
         subprocess_dir = options.for_global_scope().pants_subprocessdir
         graph_helper, target_roots, exit_code = scheduler_service.prefork(options, options_bootstrapper)
         finalizer()
-    except Exception:
+    except Exception as e:
       graph_helper = None
       target_roots = None
       options_bootstrapper = None
       # TODO: this should no longer be necessary, remove the creation of subprocess_dir
       subprocess_dir = os.path.join(get_buildroot(), '.pids')
-      exit_code = 1
-      # TODO This used to raise the _GracefulTerminationException, and maybe it should again, or notify in some way that the prefork has failed.
+      exception = _PantsProductPrecomputeFailed(e)
+
+    # NB: If a scheduler_service.prefork finishes with a non-0 exit code but doesn't raise an exception
+    # (e.g. ./pants list-and-die-for-testing ...). We still want to know about it.
+    if exception is None and exit_code != PANTS_SUCCEEDED_EXIT_CODE:
+      exception = _PantsProductPrecomputeFailed(
+        _PantsRunFinishedWithFailureException(exit_code=exit_code)
+      )
 
     return cls(
       maybe_shutdown_socket,
@@ -137,11 +171,11 @@ def create(cls, sock, args, env, services, scheduler_service):
       services,
       subprocess_dir,
       options_bootstrapper,
-      exit_code
+      exception
     )
 
   def __init__(self, maybe_shutdown_socket, args, env, graph_helper, target_roots, services,
-               metadata_base_dir, options_bootstrapper, exit_code):
+               metadata_base_dir, options_bootstrapper, exception):
     """
     :param socket socket: A connected socket capable of speaking the nailgun protocol.
     :param list args: The arguments (i.e. sys.argv) for this run.
@@ -163,8 +197,8 @@ def __init__(self, maybe_shutdown_socket, args, env, graph_helper, target_roots,
     self._target_roots = target_roots
     self._services = services
     self._options_bootstrapper = options_bootstrapper
-    self.exit_code = exit_code
     self._exiter = DaemonExiter(maybe_shutdown_socket)
+    self._exception = exception
 
   # TODO: this should probably no longer be necesary, remove.
   def _make_identity(self):
@@ -275,6 +309,11 @@ def run(self):
       hermetic_environment_as(**self._env), \
       encapsulated_global_logger():
       try:
+        # Raise any exceptions we may have found when precomputing products.
+        # NB: We raise it here as opposed to earlier because we have setup logging and stdio.
+        if self._exception is not None:
+          raise self._exception
+
         # Clean global state.
         clean_global_runtime_state(reset_subsystem=True)
 
@@ -299,11 +338,14 @@ def run(self):
         ExceptionSink.log_exception(
           'Pants run failed with exception: {}; exiting'.format(e))
         self._exiter.exit(e.exit_code)
+      except _PantsProductPrecomputeFailed as e:
+        ExceptionSink.log_exception(repr(e))
+        self._exiter.exit(e.exit_code)
       except Exception as e:
         # TODO: We override sys.excepthook above when we call ExceptionSink.set_exiter(). That
         # excepthook catches `SignalHandledNonLocalExit`s from signal handlers, which isn't
         # happening here, so something is probably overriding the excepthook. By catching Exception
         # and calling this method, we emulate the normal, expected sys.excepthook override.
         ExceptionSink._log_unhandled_exception_and_exit(exc=e)
       else:
-        self._exiter.exit(self.exit_code if self.exit_code else PANTS_SUCCEEDED_EXIT_CODE)
+        self._exiter.exit(PANTS_SUCCEEDED_EXIT_CODE)

tests/python/pants_test/bin/BUILD

@@ -12,3 +12,13 @@ python_tests(
   ],
   tags = {'integration'},
 )
+
+python_tests(
+  name='daemon_pants_runner',
+  sources=['test_daemon_pants_runner.py'],
+  dependencies=[
+    'src/python/pants/base:exiter',
+    'src/python/pants/bin',
+    'tests/python/pants_test:test_infra',
+  ]
+)

tests/python/pants_test/bin/test_daemon_pants_runner.py

@@ -0,0 +1,96 @@
+# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+
+
+from unittest.mock import Mock, patch
+
+from contextlib2 import contextmanager
+
+from pants.base.exiter import PANTS_FAILED_EXIT_CODE
+from pants.bin.daemon_pants_runner import (DaemonPantsRunner, _PantsProductPrecomputeFailed,
+                                           _PantsRunFinishedWithFailureException)
+from pants.bin.local_pants_runner import LocalPantsRunner
+from pants_test.test_base import TestBase
+
+
+class DaemonPantsRunnerTest(TestBase):
+
+  def setUp(self):
+    self.mock_sock = Mock()
+
+    self.mock_nailgunned_stdio = Mock()
+    self.mock_nailgunned_stdio.return_value.__enter__ = Mock()
+    self.mock_nailgunned_stdio.return_value.__exit__ = Mock()
+
+  def __create_mock_exiter(self):
+    mock_exiter = Mock()
+    mock_exiter.exit = Mock()
+    return mock_exiter
+
+  @contextmanager
+  def enable_creating_dpr(self):
+    with patch.object(LocalPantsRunner, 'parse_options', Mock(return_value=(Mock(), Mock(), Mock()))):
+      yield
+
+  def test_precompute_exceptions_propagated(self):
+    """
+    Test that exceptions raised at precompute time are propagated correctly.
+
+    May become obsolete after #8002 is resolved.
+    """
+    raising_scheduler_service = Mock()
+    raising_scheduler_service.prefork = Mock(side_effect=Exception('I called prefork'))
+
+    with self.enable_creating_dpr():
+      dpr = DaemonPantsRunner.create(
+        sock=self.mock_sock,
+        args=[],
+        env={},
+        services={},
+        scheduler_service=raising_scheduler_service
+      )
+
+      self.assertEqual(
+        repr(dpr._exception),
+        repr(_PantsProductPrecomputeFailed(Exception('I called prefork')))
+      )
+
+      self.check_runs_exit_with_code(dpr, PANTS_FAILED_EXIT_CODE)
+
+  def test_precompute_propagates_failures(self):
+    """
+    Tests that, when precompute returns a non-zero exit code (but doesn't raise exceptions),
+    it will be propagated to the end of the run.
+
+    May become obsolete after #8002 is resolved.
+    """
+    weird_return_value = 19
+
+    non_zero_returning_scheduler_service = Mock()
+    non_zero_returning_scheduler_service.prefork = Mock(
+      return_value=(-1, -1, weird_return_value)
+    )
+
+    with self.enable_creating_dpr():
+      dpr = DaemonPantsRunner.create(
+        sock=self.mock_sock,
+        args=[],
+        env={},
+        services={},
+        scheduler_service=non_zero_returning_scheduler_service
+      )
+
+      self.assertEqual(
+        repr(dpr._exception),
+        repr(_PantsProductPrecomputeFailed(_PantsRunFinishedWithFailureException(exit_code=weird_return_value)))
+      )
+
+      self.check_runs_exit_with_code(dpr, weird_return_value)
+
+  def check_runs_exit_with_code(self, daemon_pants_runner, code):
+    with patch.object(DaemonPantsRunner, 'nailgunned_stdio', self.mock_nailgunned_stdio):
+      daemon_pants_runner._exiter = self.__create_mock_exiter()
+      daemon_pants_runner.run()
+      self.assertIs(daemon_pants_runner._exiter.exit.called, True)
+      self.assertEqual(daemon_pants_runner._exiter.exit.call_args[0][0], code)