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

Honor installed sys.excepthook in pex teardown. #42

Merged

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Feb 7, 2015

Previously an exception that bubbled from the underlying app was
unconditionally printed.

Previously an exception that bubbled from the underlying app was
unconditionally printed.
@jsirois
Copy link
Member Author

jsirois commented Feb 7, 2015

This is in support of pantsbuild/pants#1026

import sys

def excepthook(ex_type, ex, tb):
print('Custom hook called with: {}'.format(ex))
Copy link
Contributor

Choose a reason for hiding this comment

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

I learned that '{}'.format() is not supported in Python 2.6. '{0}'.format() instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - see RB / initial test failure. Fixed now.

Update CHANGES.rst with the RB ID.
Fixup string formatting to be python2.6 compatible.
wickman added a commit that referenced this pull request Feb 8, 2015
Honor installed sys.excepthook in pex teardown.
@wickman wickman merged commit 9c2048c into pex-tool:master Feb 8, 2015
@wickman
Copy link
Contributor

wickman commented Feb 8, 2015

Thanks John -- if you look at twitter.common.python 0.5.0 to 0.5.4, you can see this specific issue dogged me. This solution seems to be the correct one.

@jsirois jsirois deleted the jsirois/pex/fix_run_except_handling branch February 8, 2015 00:03
@jsirois
Copy link
Member Author

jsirois commented Feb 8, 2015

OK - cool, you're welcome. Things are still odd on our end - I have to do things like re-import sys and traceback inside out custom hook function or else they turn up None, but this is delicate territory.

@wickman
Copy link
Contributor

wickman commented Feb 8, 2015

See the comment in the code re: atexit exceptions. Just import and save handles to the functions you need to call, otherwise the modules will get garbage collected before you call those functions when sys=traceback=None.

@jsirois
Copy link
Member Author

jsirois commented Feb 8, 2015

Thanks. Yeah - that works. Its odd for sure, but better than what I had:

diff --git a/src/python/pants/bin/pants_exe.py b/src/python/pants/bin/pants_exe.py
index c53136e..46052cf 100644
--- a/src/python/pants/bin/pants_exe.py
+++ b/src/python/pants/bin/pants_exe.py
@@ -19,53 +19,64 @@ _VERSION_OPTION = '--version'
 _PRINT_EXCEPTION_STACKTRACE = '--print-exception-stacktrace'


-def _do_exit(result=0, msg=None, out=sys.stderr):
-  if msg:
-    print(msg, file=out)
-  if _LOG_EXIT_OPTION in sys.argv and result == 0:
-    print("\nSUCCESS\n")
-  sys.exit(result)
-
-
-def _exit_and_fail(msg=None):
-  _do_exit(result=1, msg=msg)
-
-
-def _unhandled_exception_hook(exception_class, exception, tb):
-  msg = ''
-  if _PRINT_EXCEPTION_STACKTRACE in sys.argv:
-    msg = '\nException caught:\n' + ''.join(traceback.format_tb(tb))
-  if str(exception):
-    msg += '\nException message: %s\n' % str(exception)
-  else:
-    msg += '\nNo specific exception message.\n'
-  # TODO(Jin Feng) Always output the unhandled exception details into a log file.
-  _exit_and_fail(msg)
-
-
-def _run():
+class _Exiter(object):
+  def __init__(self):
+    # Since we have some exit paths that run via the sys.excepthook,
+    # symbols we use can become garbage collected before we use them; ie:
+    # we can find `sys` and `traceback` are `None`.  As a result we capture
+    # all symbols we need here to ensure we function in excepthook context.
+    # See: http://stackoverflow.com/questions/2572172/referencing-other-modules-in-atexit
+    self._exit = sys.exit
+    self._format_tb = traceback.format_tb
+    self._is_log_exit = _LOG_EXIT_OPTION in sys.argv
+    self._is_print_backtrace = _PRINT_EXCEPTION_STACKTRACE in sys.argv
+
+  def do_exit(self, result=0, msg=None, out=sys.stderr):
+    if msg:
+      print(msg, file=out)
+    if self._is_log_exit and result == 0:
+      print("\nSUCCESS\n")
+    self._exit(result)
+
+  def exit_and_fail(self, msg=None):
+    self.do_exit(result=1, msg=msg)
+
+  def unhandled_exception_hook(self, exception_class, exception, tb):
+    msg = ''
+    if self._is_print_backtrace:
+      msg = '\nException caught:\n' + ''.join(self._format_tb(tb))
+    if str(exception):
+      msg += '\nException message: %s\n' % str(exception)
+    else:
+      msg += '\nNo specific exception message.\n'
+    # TODO(Jin Feng) Always output the unhandled exception details into a log file.
+    self.exit_and_fail(msg)
+
+
+def _run(exiter):
   # Place the registration of the unhandled exception hook as early as possible in the code.
-  sys.excepthook = _unhandled_exception_hook
+  sys.excepthook = exiter.unhandled_exception_hook

   logging.basicConfig()
   version = pants_version()
   if len(sys.argv) == 2 and sys.argv[1] == _VERSION_OPTION:
-    _do_exit(msg=version, out=sys.stdout)
+    exiter.do_exit(msg=version, out=sys.stdout)

   root_dir = get_buildroot()
   if not os.path.exists(root_dir):
-    _exit_and_fail('PANTS_BUILD_ROOT does not point to a valid path: %s' % root_dir)
+    exiter.exit_and_fail('PANTS_BUILD_ROOT does not point to a valid path: %s' % root_dir)

   goal_runner = GoalRunner(root_dir)
   goal_runner.setup()
   result = goal_runner.run()
-  _do_exit(result)
+  exiter.do_exit(result)

 def main():
+  exiter = _Exiter()
   try:
-    _run()
+    _run(exiter)
   except KeyboardInterrupt:
-    _exit_and_fail('Interrupted by user.')
+    exiter.exit_and_fail('Interrupted by user.')

 if __name__ == '__main__':
   main()

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