Pantsd without forking #7596

ity · 2019-04-19T17:47:53Z

Problem

see #7390. New PR with a shared branch for collaboration (if only github would allow editing target branches in PRs)

Solution

TODO:

REPLs can't install signal handlers.
Environment variables are inherited for every pants run under pantsd.

Result

### Problem In the context of #7596, pants tasks will be executed outside of the main thread. This means that tasks like python-repl cannot override signal handling (code), because this can only be done from the main thread. ### Solution Create an atomic variable, SignalHandler._ignore_sigint, that gates SIGINT handling. Create a global context manager inside ExceptionSink that toggles this for a certain time if needed. ### Result Any thread can pause the handling of SIGINT in a controlled way. python-repl no longer installs signal handlers, but rather it toggles the handling of the signal. ### Caveat We may want to gate all signals individually, but the semantics of SIGINT are usually different enough from SIGTERM and SIGKILL that I think it's okay to only gate the first one.

blorente · 2019-04-30T10:42:06Z

Heads up, #7623 just merged and it unblocks a better fix for python_repl.py, namely using ExceptionSink.ignoring_sigint().

stuhood

Thanks, looks good, although once things are green the travis edits need to be nuked.

stuhood · 2019-04-30T23:01:32Z

src/python/pants/bin/daemon_pants_runner.py

+class NoopExiter(Exiter):
+  def exit(self, result, *args, **kwargs):
+    if result != 0:
+      # TODO this exception was not designed for this, but it happens to do what we want.


Expanding this comment would be good: what is it that we want from the NoopExiter? As with many things, I expect that we can clean it up post merge, but would be good to include a bit more context in case ... that takes a while.

Some context on this: We want the invocation of LocalPantsRunner.run() to raise an exception if it finishes with an error code, because then DaemonPantsRunner can catch that and process that as necessary (sending the error message to the nailgun client, for example). This processing happens in this set of catch blocks.

_GracefulTerminationException was designed to be a way to signal "errors that happen when we try to close the pants run before it's done". These exceptions were raised by this call.

In this line, we are changing the semantics of _GracefulTerminationException(), from "pants hasn't been able to complete the run" to "pants hasn't been able to complete the run, or pants has completed the run and it is a failure (error code >0)".

The correct way to do this would be to either explicitly update the docstring of _GracefulTerminationException to account for the change in semantics, or to create a new exception type (maybe PantsFinishedWithFailureException), and add another elif branch to this block of code

stuhood · 2019-04-30T23:02:42Z

src/python/pants/bin/daemon_pants_runner.py


-  def __init__(self, socket):
+  def __init__(self, maybe_shutdown_socket):
    # N.B. Assuming a fork()'d child, cause os._exit to be called here to avoid the routine


This part isn't accurate anymore, so would be good to clarify what this class does now.

added todo in #7606

stuhood · 2019-04-30T23:04:10Z

src/python/pants/bin/daemon_pants_runner.py

      graph_helper = None
      target_roots = None
      options_bootstrapper = None
-      # N.B. This will be overridden with the correct value if options


This comment should still be accurate I think? If it's not, then creating the subprocess_dir should no longer be necessary, and we can leave a TODO about removing it.

stuhood · 2019-04-30T23:06:21Z

src/python/pants/bin/daemon_pants_runner.py

          yield fd
      else:
        with open('/dev/null', 'rb') as fh:
          yield fh.fileno()

+    # The NailgunStreamWriter probably shouldn't grab the socket without a lock...


Should convert this to a TODO that references a ticket, probably.

stuhood · 2019-04-30T23:07:25Z

src/python/pants/bin/daemon_pants_runner.py

-        self._exiter.exit(e.exit_code)
-      except Exception:
+      except Exception as e:
+        # LocalPantsRunner.set_start_time resets the global exiter,


Is this still true? My understanding was that context managers were added to reset the exiters.

stuhood · 2019-04-30T23:12:20Z

src/python/pants/java/nailgun_io.py

+        finally:
+          os.close(read_fd)
+
+  # TODO: The error is that when the client finishes (and presumably closes the socket),


Would be good to refer to a ticket, or expand/remove this.

stuhood · 2019-04-30T23:14:50Z

tests/python/pants_test/pantsd/test_pantsd_integration.py

@@ -402,7 +403,7 @@ def test_pantsd_pid_change(self):
  def test_pantsd_memory_usage(self):
    """Validates that after N runs, memory usage has increased by no more than X percent."""
    number_of_runs = 10
-    max_memory_increase_fraction = 0.35
+    max_memory_increase_fraction = 0.60 # TODO this should be closer to 0.35


Can refer to Patrick's ticket for this.

stuhood · 2019-04-30T23:15:53Z

tests/python/pants_test/pantsd/test_pantsd_integration.py

@@ -477,6 +479,7 @@ def test_pantsd_parse_exception_success(self):
    finally:
      rm_rf(test_path)

+  @unittest.skip("Pantsd is not expected to handle parallel runs anymore.")


If these will be skipped rather than deleted, we should probably refer to a ticket about re-enabling parallel runs for v2.

stuhood · 2019-04-30T23:16:02Z

tests/python/pants_test/pantsd/test_pantsd_integration.py

      os.kill(client_pid, signum)
      waiter_run = waiter_handle.join()
      self.assert_failure(waiter_run)

+
+      print("Run finished, stderr data is {}".format(waiter_run.stderr_data))


blorente

This is very exciting! Thanks for all the work on this.

blorente · 2019-05-02T12:13:12Z

src/python/pants/backend/python/tasks/python_repl.py

+      # That said, this means that under pantsd,
+      # Ctrl-C will simply crash the repl (and the daemon).
+      # TODO(#7623) Potential more robust (but more invasive) fix.
+      self._run_repl(pex, **pex_run_kwargs)


This is fixed as of #7623, we can just replace the whole body of this method with:

Suggested change

self._run_repl(pex, **pex_run_kwargs)

with ExceptionSink.ignoring_signint():

self._run_repl(pex, **pex_run_kwargs)

blorente · 2019-05-02T12:14:09Z

src/python/pants/base/exception_sink.py

@@ -425,6 +425,7 @@ def _exit_with_failure(cls, terminal_msg):
  @classmethod
  def _log_unhandled_exception_and_exit(cls, exc_class=None, exc=None, tb=None, add_newline=False):
    """A sys.excepthook implementation which logs the error and exits with failure."""
+


Leftover blank line

blorente · 2019-05-02T12:16:25Z

src/python/pants/bin/daemon_pants_runner.py

 from pants.init.util import clean_global_runtime_state
 from pants.java.nailgun_io import NailgunStreamStdinReader, NailgunStreamWriter
-from pants.java.nailgun_protocol import ChunkType, NailgunProtocol
-from pants.pantsd.process_manager import ProcessManager
+from pants.java.nailgun_protocol import ChunkType, MaybeShutdownSocket, NailgunProtocol
 from pants.util.contextutil import hermetic_environment_as, stdio_as
 from pants.util.socket import teardown_socket


 class DaemonSignalHandler(SignalHandler):


I don't think this signal handler ever takes effect, because we don't have a good way to enable it from the main thread (this whole piece of code runs in a child thread, and we can't use custom signal handlers in those for now). I think this entire class can be removed.

blorente · 2019-05-02T12:30:31Z

src/python/pants/bin/daemon_pants_runner.py

+class NoopExiter(Exiter):
+  def exit(self, result, *args, **kwargs):
+    if result != 0:
+      # TODO this exception was not designed for this, but it happens to do what we want.


Some context on this: We want the invocation of LocalPantsRunner.run() to raise an exception if it finishes with an error code, because then DaemonPantsRunner can catch that and process that as necessary (sending the error message to the nailgun client, for example). This processing happens in this set of catch blocks.

_GracefulTerminationException was designed to be a way to signal "errors that happen when we try to close the pants run before it's done". These exceptions were raised by this call.

In this line, we are changing the semantics of _GracefulTerminationException(), from "pants hasn't been able to complete the run" to "pants hasn't been able to complete the run, or pants has completed the run and it is a failure (error code >0)".

The correct way to do this would be to either explicitly update the docstring of _GracefulTerminationException to account for the change in semantics, or to create a new exception type (maybe PantsFinishedWithFailureException), and add another elif branch to this block of code

blorente · 2019-05-02T12:33:04Z

src/python/pants/bin/daemon_pants_runner.py

-  """An Exiter that emits unhandled tracebacks and exit codes via the Nailgun protocol."""
+  """An Exiter that emits unhandled tracebacks and exit codes via the Nailgun protocol.
+
+  TODO: This no longer really follows the Exiter API, per-se (or at least, it doesn't call super).


If this no longer follows the Exiter API, and (for now) we don't need the DaemonExiter to act as an exiter, we could maybe inline this code, or turn it into a standalone class and make this not inherit Exiter (possibly changing the name to DaemonNailgunCloser or something to avoid confusing this with other *Exiter classes.

added todo in #7606

blorente · 2019-05-02T12:36:49Z

src/python/pants/bin/daemon_pants_runner.py

-
-    self._exiter = DaemonExiter(socket)
+    self.exit_code = exit_code
+    self._exiter = DaemonExiter(maybe_shutdown_socket)

  def _make_identity(self):


We may not need this anymore. I'm not sure, but it seems related to ProcessManager. If we need it, I suspect it will be for Nailgun communication.

blorente · 2019-05-02T12:43:53Z

src/python/pants/bin/local_pants_runner.py

      options_bootstrapper=options_bootstrapper,
    )
    global_options = options.for_global_scope()
+    setup_logging_from_options(global_options)


Might be worth noting that this change only works because of the encapsulated_logger in DaemonPantsRunner, and therefore we don't have to gate logging setup anymore.

blorente · 2019-05-02T12:45:32Z

src/python/pants/bin/pants_runner.py

@@ -57,7 +65,10 @@ def run(self):
    for message_regexp in global_bootstrap_options.ignore_pants_warnings:
      warnings.filterwarnings(action='ignore', message=message_regexp)

-    if global_bootstrap_options.enable_pantsd:
+    # TODO For kill-pantsd and clean-all, we cannot run them in the daemon itself, because


There are #7205 (issue) and #7206 (PR), that originally proposed this solution. So this PR should either close that, or leave a TODO linking to that.

blorente · 2019-05-02T12:50:12Z

src/python/pants/pantsd/pants_daemon.py

+    self._services = services
+
+  def handle_sigint(self, signum, _frame):
+    for service in self._services:


This should maybe terminate itself but not watchman (with this function).

Something like:

class PantsDaemonSignalHandler(SignalHandler): def __init__(self, daemon): super(PantsDaemonSignalHandler, self).__init__() self._daemon = daemon def handle_sigint(self, signum, _frame): self._daemon.terminate(include_watchman=False) # Send a useful message to users

blorente · 2019-05-02T12:52:03Z

src/python/pants/pantsd/pants_daemon.py

+
+  def handle_sigint(self, signum, _frame):
+    for service in self._services:
+      service.record_exception(KeyboardInterrupt('remote client sent control-c!'))


This was a test for an api that I forgot to remove, it doesn't actually do anything, this line can be safely removed.

illicitonion

Woo!

illicitonion · 2019-05-02T15:32:14Z

tests/python/pants_test/pantsd/test_pantsd_integration.py

@@ -458,6 +459,7 @@ def test_pantsd_invalidation_stale_sources(self):
    finally:
      rm_rf(test_path)

+  @unittest.skip("Pantsd is not expected to handle parallel runs anymore.")


Is this long-term true? I thought we did expect pantsd to still handle parallel runs of some goals, including list?

If it's long-term true, let's delete the test.
If it's not long-term true, let's add a link to a ticket about fixing it.

thanks, added TODO

This synchronises background threads that read from the nailgun socket with the DaemonPantsRunner which closes it

**Implementation for #8002** Before removing forking from pantsd (#7596 ), we needed to warm up the v2 product graph of the daemon before forking into a pantsd-runner process (relevant code here). Now, however, this warming is not necessary (because we don't fork a process per run of pants anymore), so we can do some cleanup to that code: We can remove the concept of "warming" (here), which is invoked from scheduler_service.prefork (here). This probably means that we can remove the entire warm_product_graph function. We can reword the code around scheduler_service.prefork so that it doesn't mention forking at all. It was called prefork because that's what we did before #7596, but it can be called something different now. We can merge the try:except block in DaemonPantsRunner.create() (here) into the try:except block in DaemonPantsRunner.run() (here). The run() function aims to centralize and correctly handle all the exceptions that happen on a pants run, so the "prefork" logic should be integrated there as well. This will help a lot in correctly diagnosing pantsd problems, as this split and the wrangled exception handling that happen as a result have caused numerous issues.

ity mentioned this pull request Apr 19, 2019

wip - pantsd without forking #7441

Closed

ity added this to the 1.16.x milestone Apr 19, 2019

blorente force-pushed the pantsd-sans-forking branch 2 times, most recently from 7cfc6e8 to b8d598d Compare April 25, 2019 09:20

This was referenced Apr 25, 2019

[pantsd] Pantsd test harness wrongly identifies some processes as pantsd lingering processes. #7621

Closed

Add the possibility to ignore sigint from other threads #7623

Merged

blorente force-pushed the pantsd-sans-forking branch from d1642af to 3513d9f Compare April 26, 2019 14:20

ity force-pushed the pantsd-sans-forking branch from 3513d9f to 49ae0dd Compare April 30, 2019 20:58

stuhood approved these changes Apr 30, 2019

View reviewed changes

patliu85 mentioned this pull request May 1, 2019

Investigate pantsd memory usage increases more than expected #7647

Closed

ity force-pushed the pantsd-sans-forking branch from ca3b407 to 5f4c6a8 Compare May 1, 2019 20:05

stuhood requested review from illicitonion and blorente May 1, 2019 23:46

blorente approved these changes May 2, 2019

View reviewed changes

illicitonion approved these changes May 2, 2019

View reviewed changes

ity force-pushed the pantsd-sans-forking branch from abe3e0c to 2a79fc9 Compare May 3, 2019 03:27

blorente force-pushed the pantsd-sans-forking branch from e8fa10d to 9e85d3e Compare May 3, 2019 09:24

ity force-pushed the pantsd-sans-forking branch from 9e85d3e to b6ef5b3 Compare May 3, 2019 14:54

illicitonion and others added 10 commits May 3, 2019 08:31

Squashed removing forking

08efab1

Don't close the same fd twice

21d2840

Modify some pantsd tests

f606eb9

Logging was not being initialized in non-daemon pants runs

fca104a

A more local, less invasive, but more broken for repl signal handling

428bc15

Remove leftover code

9a9b8ec

Differentiate self-closing pipes from pipes

c414d82

Don't try to read from a closed nailgun socket

8b6d6b6

This synchronises background threads that read from the nailgun socket with the DaemonPantsRunner which closes it

some cleanup

04674cc

cleanup logging; ready for review

28372ce

ity and others added 8 commits May 3, 2019 08:32

Bring back _log_unhandled_exception_and_exit

a1b21aa

fix termination test failure

77878fa

Set exit_code on error

13e71e3

Fix unit tests

ed804e8

Remove obsolete comment

0ebb076

fix auto-shutdown integration test failure

28075f7

Addressed all review comments

4e78493

revert python repl review comment to fix failing test

9215622

ity force-pushed the pantsd-sans-forking branch from b6ef5b3 to 9215622 Compare May 3, 2019 15:32

remove travis edits

cde0ef6

ity merged commit 5ee0e81 into pantsbuild:master May 3, 2019

blorente mentioned this pull request May 30, 2019

Remove Resettable from the Rust code #7821

Closed

This was referenced Jul 3, 2019

[pantsd] Remove dead code around prefork graph warming #8002

Closed

don't start up a pantsd if we're only invoking kill-pantsd or clean-all #7205

Closed

Enable pantsd by default #4438

Closed

wisechengyi mentioned this pull request Jul 23, 2019

[pantsd] Remove dead code around prefork graph warming #8095

Merged

stuhood mentioned this pull request May 7, 2020

pantsd nailgunerror segfault #6714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pantsd without forking #7596

Pantsd without forking #7596

ity commented Apr 19, 2019 •

edited by blorente

Loading

blorente commented Apr 30, 2019

stuhood left a comment

stuhood Apr 30, 2019

blorente May 2, 2019

stuhood Apr 30, 2019

ity May 2, 2019

stuhood Apr 30, 2019

stuhood Apr 30, 2019

stuhood Apr 30, 2019

stuhood Apr 30, 2019

stuhood Apr 30, 2019

stuhood Apr 30, 2019

stuhood Apr 30, 2019

blorente left a comment

blorente May 2, 2019

blorente May 2, 2019

blorente May 2, 2019

blorente May 2, 2019

blorente May 2, 2019

ity May 2, 2019

blorente May 2, 2019

ity May 2, 2019

blorente May 2, 2019

blorente May 2, 2019

blorente May 2, 2019

blorente May 2, 2019

illicitonion left a comment

illicitonion May 2, 2019

ity May 2, 2019

	self._run_repl(pex, **pex_run_kwargs)
	with ExceptionSink.ignoring_signint():
	self._run_repl(pex, **pex_run_kwargs)

Pantsd without forking #7596

Pantsd without forking #7596

Conversation

ity commented Apr 19, 2019 • edited by blorente Loading

Problem

Solution

Result

blorente commented Apr 30, 2019

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blorente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illicitonion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ity commented Apr 19, 2019 •

edited by blorente

Loading