Skip to content

Conversation

JaredNeil
Copy link
Member

@JaredNeil JaredNeil commented Sep 14, 2020

The default global ExecutionContext simply logs uncaught exceptions. Since Future only catches NonFatal exceptions, any Fatal exceptions would cause the Future to never complete, which meant a WorkResponse was never sent back to the bazel server, which would then be stuck waiting forever for a response.

Since Fatal exceptions can leave the JVM in an invalid state, we exit if any thread has an unhandled Fatal exception.

@jroylance jroylance self-requested a review September 14, 2020 21:43
Copy link

@jroylance jroylance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming it works when tested

@JaredNeil JaredNeil force-pushed the jaredneil-catch-fatal branch from 8112693 to 66a469d Compare September 14, 2020 23:53
@JaredNeil JaredNeil changed the title Catch all exceptions Exit Worker process on Fatal exceptions Sep 14, 2020
The default global ExecutionContext simply logs uncaught exceptions.
Since Future only catches NonFatal exceptions, any Fatal exceptions
would cause the Future to never complete, which meant a WorkResponse was
never sent back to the bazel server, which would then be stuck waiting
forever for a response.

Since Fatal exceptions can leave the JVM in an invalid state, we exit if
any thread has an unhandled Fatal exception.
Zinc seems to run into this error pretty frequently, and if uncaught,
the entire worker process will exit suddenly, which can leave Bazel in a
weird state where it tries to send messages to the worker, but can't
because the process has already exited. Consider this a temporary
workaround until the underlying issues with Bazel can be fixed.
@JaredNeil JaredNeil force-pushed the jaredneil-catch-fatal branch from 66a469d to fef122d Compare September 15, 2020 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants