Skip to content

Commit

Permalink
Handle ValueError when child wants to reset stderr but it is closed. (p…
Browse files Browse the repository at this point in the history
…antsbuild#6932)

### Problem

After forking, when the child wants to reset `sys.stderr` as part of setting up the `ExceptionSink`, if `sys.stderr` was closed, it would except and crash.

### Solution

Wrap the line in `try:except`, and log it as a warning. However, I have a doubt:
The line in question happens in {{post_fork_child}} after a call to {{daemonize}}, and fails because {{sys.stderr}} is closed. So, given that we are considering that case as "non-fatal" (hence turning the exception into a warning), would it make sense to instead write something like:

```python
if not sys.stderr.closed:
  ExceptionSink.reset_interactive_output_stream(sys.stderr)
else:
  logger = logging.getLogger(__name__)
  logger.warn("Cannot reset output stream - sys.stderr is closed")
```

The difference is small, but if the answer is no it means that I'm missing something.

### Result

One less hard-to-find crash, hopefully.
  • Loading branch information
blorente authored and Stu Hood committed Dec 18, 2018
1 parent bbc80e8 commit 6774bba
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 10 deletions.
24 changes: 14 additions & 10 deletions src/python/pants/base/exception_sink.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,20 @@ def reset_interactive_output_stream(cls, interactive_output_stream):
This is where the the error message on exit will be printed to as well.
"""
# NB: mutate process-global state!
if faulthandler.unregister(signal.SIGUSR2):
logger.debug('re-registering a SIGUSR2 handler')
# This permits a non-fatal `kill -31 <pants pid>` for stacktrace retrieval.
faulthandler.register(signal.SIGUSR2, interactive_output_stream,
all_threads=True, chain=False)

# NB: mutate the class variables!
# We don't *necessarily* need to keep a reference to this, but we do here for clarity.
cls._interactive_output_stream = interactive_output_stream
try:
# NB: mutate process-global state!
if faulthandler.unregister(signal.SIGUSR2):
logger.debug('re-registering a SIGUSR2 handler')
# This permits a non-fatal `kill -31 <pants pid>` for stacktrace retrieval.
faulthandler.register(signal.SIGUSR2, interactive_output_stream,
all_threads=True, chain=False)

# NB: mutate the class variables!
# We don't *necessarily* need to keep a reference to this, but we do here for clarity.
cls._interactive_output_stream = interactive_output_stream
except ValueError:
# Warn about "ValueError: IO on closed file" when stderr is closed.
ExceptionSink.log_exception("Cannot reset output stream - sys.stderr is closed")

@classmethod
def exceptions_log_path(cls, for_pid=None, in_dir=None):
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/bin/daemon_pants_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ def post_fork_child(self):
# terminal window.
# TODO: test the above!
ExceptionSink.reset_exiter(self._exiter)

ExceptionSink.reset_interactive_output_stream(sys.stderr)

# Ensure anything referencing sys.argv inherits the Pailgun'd args.
Expand Down

0 comments on commit 6774bba

Please sign in to comment.