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

fix exception handling of KeyboardInterrupt during startup #8182

Merged

Conversation

@cosmicexplorer
Copy link
Contributor

commented Aug 17, 2019

Problem

If pants is hit with a Ctrl-C when in the engine at the right time when it starts up, it fails to handle the exception correctly, due to an incorrect datatype() type constraint:

Traceback (most recent call last):
  File "/Users/dmcclanahan/tools/pants-v3/src/python/pants/engine/native.py", line 256, in exc_handler
    error_info = native.CFFIExternMethodRuntimeErrorInfo(exc_type, exc_value, traceback)
  File "/Users/dmcclanahan/tools/pants-v3/src/python/pants/util/objects.py", line 229, in __new__
    '\n'.join(type_failure_msgs)))
pants.util.objects.TypedDatatypeInstanceConstructionError: type check error in class CFFIExternMethodRuntimeErrorInfo: 1 error type checking constructor arguments:
field 'exc_value' was invalid: value KeyboardInterrupt('User interrupted execution with control-c!') (with type 'KeyboardInterrupt') must satisfy this type constraint: SubclassesOf(Exception).

Additionally, when this occurs, the exception is invisible to the user, because the default behavior decided previously in exception_sink.py was to avoid printing a stacktrace if errors occurred at import time,:

timestamp: 2019-08-17T13:51:45.667543
Exception caught: (pants.build_graph.address_lookup_error.AddressLookupError) (backtrace omitted)
Exception message: Build graph construction failed: RuntimeError cannot use from_handle() on NULL pointer

Both of these have led to repeated confusion from users, especially when using pantsd.

Solution

  • Extend the type constraint to be SubclassesOf(BaseException), since apparently KeyboardInterrupt is not an Exception.
  • Default to printing the stacktrace for exceptions during import time instead of hiding it.

Result

Users don't have mysterious errors when they try to quit a pants run!

@@ -533,4 +533,4 @@ def _handle_signal_gracefully(cls, signum, signame, traceback_lines):
# to explicitly request it. The exception log will have any stacktraces regardless so this should
# not hamper debugging.
ExceptionSink.reset_should_print_backtrace_to_terminal(
should_print_backtrace=os.environ.get('PANTS_PRINT_EXCEPTION_STACKTRACE') == 'True')
should_print_backtrace=os.environ.get('PANTS_PRINT_EXCEPTION_STACKTRACE', 'True') == 'True')

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 17, 2019

Contributor

Do you know why this doesn't use global_options.py?

register('--print-exception-stacktrace', advanced=True, type=bool,
help='Print to console the full exception stack trace if encountered.')

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 17, 2019

Author Contributor

Because it runs at import time! This is overwritten immediately after we parse bootstrap options:

ExceptionSink.reset_should_print_backtrace_to_terminal(global_bootstrap_options.print_exception_stacktrace)

Beyond an environment variable, I wasn't sure how to allow the user to affect how errors are displayed at import time. There may be a more reasonable interface I'm not seeing.

@cosmicexplorer cosmicexplorer requested a review from jsirois Aug 18, 2019

@jsirois

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Solution

  • Extend the type constraint to be SubclassesOf(Exception, KeyboardInterrupt), since apparently KeyboardInterrupt is not an Exception.

Yes: https://docs.python.org/3.6/library/exceptions.html#KeyboardInterrupt
But with no opinion expressed, does this mean that the type constraint should have been BaseException all along?: https://docs.python.org/3.6/library/exceptions.html#BaseException

@blorente
Copy link
Contributor

left a comment

+1 to having CFFIExternMethodRuntimeErrorInfo.exc_value inherit from BaseException.

Otherwise, this seems great! I'm assuming this is super timing-dependent, so I can't think of how to write a test that wouldn't be flaky eventually. That said, if you have ideas, I'd love to see a regression test for this!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-ctrl-c-on-import branch from dcebb62 to 157968c Sep 3, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:fix-ctrl-c-on-import branch from 3e8b47d to 2b46760 Sep 4, 2019

@cosmicexplorer cosmicexplorer merged commit 874cd09 into pantsbuild:master Sep 6, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.