Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the possibility to ignore sigint from other threads #7623

Merged
merged 5 commits into from Apr 30, 2019

Conversation

Projects
None yet
3 participants
@blorente
Copy link
Contributor

commented Apr 25, 2019

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.

@illicitonion
Copy link
Contributor

left a comment

Looks good! One naming thing :)

def _handle_sigint_if_enabled(self, signum, _frame):
with self._ignore_sigint_lock:
self._check_sigint_gate_is_correct()
should_ignore_sigint = self._ignore_sigint

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 25, 2019

Contributor

Both of the names on this line are kind of unclear...

should_ignore_sigint reads to me like a boolean (when actually the only "true" value of it is 0, which is False as a boolean)
self._ignore_sigint also reads to me like a boolean.

Maybe we could call both of these number_of_threads_ignoring_sigint or similar?

@@ -310,6 +338,18 @@ def trapped_signals(cls, new_signal_handler):
finally:
cls.reset_signal_handler(previous_signal_handler)

@classmethod
@contextmanager
def ignoring_sigint(cls):

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 25, 2019

Contributor

I think this might be more easily and explicitly done by creating a new SignalHandler subclass in python_repl.py with the lock and flag, then using with ExceptionSink.trapped_signals(sigint_toggling_signal_handler): there, instead of reaching in and mutating the global signal handler field, or moving signal handling concerns into ExceptionSink itself. This seems to be more testable, less coupled, and less spooky, and makes it clear when looking at the python repl task that some exclusive locking which modifies global state is going on.

This solution seems to work great as is -- my main goal with the global state in ExceptionSink was to make global state changes explicit and avoid mixing application-specific and process-handling logic. SignalHandler was an attempt to move task-specific or process-specific logic out of ExceptionSink and decouple it from the way we actually exit or log errors. Since it seems like ExceptionSink.trapped_signals() overwriting the existing signal handling provides the temporary override we want, it seems reasonable to use that approach instead of creating a new interface which reaches into the signal handler base class to modify it.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 25, 2019

Contributor

See #7606 for an example of an Exiter accepting another instance of an Exiter and applying the prototype pattern in order to do a two-step teardown within LocalPantsRunner by calling another exiter passed into the constructor -- we could do something similar here with a SignalHandler subclass which does the same thing as a base signal handler instance, but also adds the ignoring_sigint() logic.

This comment has been minimized.

Copy link
@blorente

blorente Apr 25, 2019

Author Contributor

I agree, wholeheartedly. But, in a world without pantsd-runner, the repl runs in a non-main thread, so when we try to do ExceptionSink.trapped_signals(), and this tries to signal.signal() for whatever handler we passed from python-repl, it will crash saying that this can only be done from the main thread.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 26, 2019

Contributor

Oh! I didn't realize that, sorry. Could we then add an underscore to SignalHandler._ignoring_sigint() and then add something like:

NB: This method calls signal.signal(), which will crash if not called from the main thread!

to the docstring for .reset_signal_handler() and .trapped_signals()?

@@ -310,6 +338,18 @@ def trapped_signals(cls, new_signal_handler):
finally:
cls.reset_signal_handler(previous_signal_handler)

@classmethod
@contextmanager
def ignoring_sigint(cls):

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 26, 2019

Contributor

Oh! I didn't realize that, sorry. Could we then add an underscore to SignalHandler._ignoring_sigint() and then add something like:

NB: This method calls signal.signal(), which will crash if not called from the main thread!

to the docstring for .reset_signal_handler() and .trapped_signals()?

@blorente blorente force-pushed the blorente:blorente/gated-signal-handler branch from ea1a714 to bd37935 Apr 29, 2019

@blorente blorente merged commit 7ca3089 into pantsbuild:master Apr 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@blorente blorente referenced this pull request Apr 30, 2019

Merged

Pantsd without forking #7596

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.