wip - add contextmanager'd exiters and change return type of run() #7606

ity · 2019-04-23T00:00:52Z

Problem

Solution

Result

stuhood · 2019-04-23T01:08:42Z

src/python/pants/bin/daemon_pants_runner.py

+    """A contextmanager which temporarily overrides the exiter."""
+    try:
+      previous_exiter = cls._exiter
+      cls._exiter = exiter


It's possible that cls._exiter might no longer necessary, since we should always be able to use the global mutable exiter? If that's the case, then both DaemonPantsRunner.set_exiter and LocalPantsRunner.set_exiter could be merged into the same method somewhere. Maybe with ExceptionSink.exiter_as(mk_exiter=lambda previous_exiter: ..): or something (where the mk_exiter function could optionally wrap the existing exiter, or replace it)?

yes, absolutely, was thinking the same that its duplicated for no good reason - will look into this tomorrow

blorente

I like the architecture, I think what's left are my comments and adding unit tests.

blorente · 2019-04-24T09:04:32Z

src/python/pants/base/exception_sink.py

+    """A contextmanager which temporarily overrides the exiter."""
+    try:
+      previous_exiter = cls._exiter if cls._exiter else exiter
+      cls._exiter = exiter


I don't think this is needed, as ExceptionSink.reset_exiter() already overrides the class variables :)

We might need to edit reset_exiter() the way we did reset_signal_handler() in order to support trapped_signals() in #6574. Luckily all that would mean is making reset_exiter() return the previous exiter.

pants/src/python/pants/base/exception_sink.py

Lines 278 to 311 in f9b0313

@classmethod

def reset_signal_handler(cls, signal_handler):

"""

Class state:

- Overwrites `cls._signal_handler`.

OS state:

- Overwrites signal handlers for SIGINT, SIGQUIT, and SIGTERM.

:returns: The :class:`SignalHandler` that was previously registered, or None if this is

the first time this method was called.

"""

assert(isinstance(signal_handler, SignalHandler))

# NB: Modify process-global state!

for signum, handler in signal_handler.signal_handler_mapping.items():

signal.signal(signum, handler)

# Retry any system calls interrupted by any of the signals we just installed handlers for

# (instead of having them raise EINTR). siginterrupt(3) says this is the default behavior on

# Linux and OSX.

signal.siginterrupt(signum, False)

previous_signal_handler = cls._signal_handler

# NB: Mutate the class variables!

cls._signal_handler = signal_handler

return previous_signal_handler

@classmethod

@contextmanager

def trapped_signals(cls, new_signal_handler):

"""A contextmanager which temporarily overrides signal handling."""

try:

previous_signal_handler = cls.reset_signal_handler(new_signal_handler)

yield

finally:

cls.reset_signal_handler(previous_signal_handler)

blorente · 2019-04-24T09:06:16Z

src/python/pants/bin/local_pants_runner.py

-      run_tracker_result
-    )
-    self._exiter.exit(final_exit_code)
+        self._maybe_handle_help()


Note that this will exit(), so if you're returning in this function, it might be worth to return here as well.

cosmicexplorer · 2019-04-25T02:42:03Z

src/python/pants/base/exception_sink.py

+    """A contextmanager which temporarily overrides the exiter."""
+    try:
+      previous_exiter = cls._exiter if cls._exiter else exiter
+      cls._exiter = exiter


We might need to edit reset_exiter() the way we did reset_signal_handler() in order to support trapped_signals() in #6574. Luckily all that would mean is making reset_exiter() return the previous exiter.

pants/src/python/pants/base/exception_sink.py

Lines 278 to 311 in f9b0313

@classmethod

def reset_signal_handler(cls, signal_handler):

"""

Class state:

- Overwrites `cls._signal_handler`.

OS state:

- Overwrites signal handlers for SIGINT, SIGQUIT, and SIGTERM.

:returns: The :class:`SignalHandler` that was previously registered, or None if this is

the first time this method was called.

"""

assert(isinstance(signal_handler, SignalHandler))

# NB: Modify process-global state!

for signum, handler in signal_handler.signal_handler_mapping.items():

signal.signal(signum, handler)

# Retry any system calls interrupted by any of the signals we just installed handlers for

# (instead of having them raise EINTR). siginterrupt(3) says this is the default behavior on

# Linux and OSX.

signal.siginterrupt(signum, False)

previous_signal_handler = cls._signal_handler

# NB: Mutate the class variables!

cls._signal_handler = signal_handler

return previous_signal_handler

@classmethod

@contextmanager

def trapped_signals(cls, new_signal_handler):

"""A contextmanager which temporarily overrides signal handling."""

try:

previous_signal_handler = cls.reset_signal_handler(new_signal_handler)

yield

finally:

cls.reset_signal_handler(previous_signal_handler)

cosmicexplorer · 2019-04-25T02:48:12Z

src/python/pants/bin/local_pants_runner.py

-      goal_runner_result = self._maybe_run_v1()
-    finally:
+    # wrap the outer exiter
+    local_exiter = LocalExiter(self._run_tracker, self._repro, exiter=self._exiter)


Suggested change

local_exiter = LocalExiter(self._run_tracker, self._repro, exiter=self._exiter)

local_exiter = LocalExiter(self._run_tracker, self._repro, exiter=ExceptionSink.global_exiter())

cosmicexplorer · 2019-04-25T02:50:07Z

src/python/pants/bin/daemon_pants_runner.py

@@ -332,40 +328,40 @@ def post_fork_child(self):

    # Invoke a Pants run with stdio redirected and a proxied environment.
    with self.nailgunned_stdio(self._socket, self._env) as finalizer,\
-         hermetic_environment_as(**self._env):
+         hermetic_environment_as(**self._env),\
+         ExceptionSink.exiter_as(self._exiter) as daemon_exiter:


I believe we could avoid having .exiter_as() have to yield a value, by instead exposing an accessor to the globally registered exiter. This could look like the edit suggestion I've made in local_pants_runner.py. The only reason that might be nicer is because we then wouldn't have to pass the exiter as an argument into LocalPantsRunner, it would just read the global value.

cosmicexplorer · 2019-04-25T02:52:16Z

src/python/pants/bin/local_pants_runner.py

@@ -286,22 +283,27 @@ def _compute_final_exit_code(*codes):
    return max_code

  def _run(self):
-    try:
-      self._maybe_handle_help()
+    self._set_start_time(time.time())


Not allowing the start time to be injected via environment variable anymore sounds like something we would want to note in the PR description perhaps.

ity · 2019-05-02T23:23:15Z

update with https://github.com/pantsbuild/pants/pull/7596/files/2a46298b3ad112244b42e2b8fe80719baf20b7c4#r280397362 or create another issue

ity · 2019-06-19T18:39:10Z

@blorente I think this is still relevant - can look at this next, lmk if you think otherwise.

blorente · 2019-06-20T09:25:11Z

This is very much relevant, will take a look myself and hand off if it's not done in half a day.

**Problem** There is a bug where, once the LocalPantsRunner has reset the global exiter to its own LocalExiter, it won't un-set it ever, and therefore unhandled exceptions will use that exiter to exit. This has caused issues, around this line, because the LocalPantsRunner reset the global exiter and the DaemonPantsRunner didn't reset it. There is a regression test to demonstrate this issue. **Solution** Following the discussion on #7606, implement a contextmanager that temporarily overrides the global exiter. Remove the concept of "my exiter", and make classes (LocalPantsRunner, DaemonPantsRunner, PantsRunner and PantsDaemon) use the global exiter. **Result** The regression test passes, and _log_unhandled_exceptions_and_exit works as expected in DaemonPantsRunner.

Eric-Arellano · 2020-05-17T23:11:41Z

Closing due to being stale.

stuhood reviewed Apr 23, 2019

View reviewed changes

stuhood requested a review from cosmicexplorer April 23, 2019 01:09

ity added 2 commits April 24, 2019 00:26

resolve

b7af7d3

extract out exiter_as to ExceptionSink and cleanup

f9b0313

ity force-pushed the ity/exiter branch from 0ffa7ab to f9b0313 Compare April 24, 2019 07:52

ity requested a review from blorente April 24, 2019 07:55

blorente approved these changes Apr 24, 2019

View reviewed changes

cosmicexplorer reviewed Apr 25, 2019

View reviewed changes

cosmicexplorer mentioned this pull request Apr 25, 2019

Add the possibility to ignore sigint from other threads #7623

Merged

ity mentioned this pull request May 2, 2019

Pantsd without forking #7596

Merged

2 tasks

blorente mentioned this pull request Jul 25, 2019

Properly manage the lifetime of Exiters in Daemon Runs #7996

Merged

Eric-Arellano closed this May 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip - add contextmanager'd exiters and change return type of run() #7606

wip - add contextmanager'd exiters and change return type of run() #7606

ity commented Apr 23, 2019

stuhood Apr 23, 2019 •

edited

Loading

ity Apr 23, 2019

blorente left a comment

blorente Apr 24, 2019

cosmicexplorer Apr 25, 2019

blorente Apr 24, 2019

cosmicexplorer Apr 25, 2019

cosmicexplorer Apr 25, 2019

cosmicexplorer Apr 25, 2019

cosmicexplorer Apr 25, 2019

ity commented May 2, 2019

ity commented Jun 19, 2019

blorente commented Jun 20, 2019

Eric-Arellano commented May 17, 2020

	@classmethod
	def reset_signal_handler(cls, signal_handler):
	"""
	Class state:
	- Overwrites `cls._signal_handler`.
	OS state:
	- Overwrites signal handlers for SIGINT, SIGQUIT, and SIGTERM.

	:returns: The :class:`SignalHandler` that was previously registered, or None if this is
	the first time this method was called.
	"""
	assert(isinstance(signal_handler, SignalHandler))
	# NB: Modify process-global state!
	for signum, handler in signal_handler.signal_handler_mapping.items():
	signal.signal(signum, handler)
	# Retry any system calls interrupted by any of the signals we just installed handlers for
	# (instead of having them raise EINTR). siginterrupt(3) says this is the default behavior on
	# Linux and OSX.
	signal.siginterrupt(signum, False)

	previous_signal_handler = cls._signal_handler
	# NB: Mutate the class variables!
	cls._signal_handler = signal_handler
	return previous_signal_handler

	@classmethod
	@contextmanager
	def trapped_signals(cls, new_signal_handler):
	"""A contextmanager which temporarily overrides signal handling."""
	try:
	previous_signal_handler = cls.reset_signal_handler(new_signal_handler)
	yield
	finally:
	cls.reset_signal_handler(previous_signal_handler)

	local_exiter = LocalExiter(self._run_tracker, self._repro, exiter=self._exiter)
	local_exiter = LocalExiter(self._run_tracker, self._repro, exiter=ExceptionSink.global_exiter())

wip - add contextmanager'd exiters and change return type of run() #7606

wip - add contextmanager'd exiters and change return type of run() #7606

Conversation

ity commented Apr 23, 2019

Problem

Solution

Result

stuhood Apr 23, 2019 • edited Loading

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

ity commented May 2, 2019

ity commented Jun 19, 2019

blorente commented Jun 20, 2019

Eric-Arellano commented May 17, 2020

stuhood Apr 23, 2019 •

edited

Loading