tahoe-lafs/trac

write test for anti-Ubuntu-crash-reporter exception-catching code #1746

New issue

Closed

opened 2012-05-20 14:23:22 +00:00 by ChosenOne · 21 comments

ChosenOne commented

2012-05-20 14:23:22 +00:00

Owner

Hey,

I just noticed that 'tahoe put' (without any parameters, in an unconfigured state) might throw an IOError (No such file or directory: u'~/.tahoe/node.url').

While this itself is a little unfriendly towards user, it is also causing the Ubuntu 'Crash report' application the show up. The 'crash report' appears to be a new features in Ubuntu 12.04. Apparently, it's sitting in the background looking for failures/exceptions and nags people who know what they do :P

This may lead to bad statistics at Ubuntu about tahoe's stability. Also, it's pretty annoying. :)

(See attachment)

Hey, I just noticed that 'tahoe put' (without any parameters, in an unconfigured state) might throw an IOError (No such file or directory: u'~/.tahoe/node.url'). While this itself is a little unfriendly towards user, it is also causing the Ubuntu 'Crash report' application the show up. The 'crash report' appears to be a new features in Ubuntu 12.04. Apparently, it's sitting in the background looking for failures/exceptions and nags people who know what they do :P This may lead to bad statistics at Ubuntu about tahoe's stability. Also, it's pretty annoying. :) (See attachment)

tahoe-lafs added the

labels 2012-05-20 14:23:22 +00:00

tahoe-lafs added this to the undecided milestone 2012-05-20 14:23:22 +00:00

daira was assigned by tahoe-lafs

2012-05-20 14:23:22 +00:00

ChosenOne commented

2012-05-20 14:25:01 +00:00

Author

Owner

Attachment 50.png (24353 bytes) added

ubuntu crash report (collapsed)

**Attachment** 50.png (24353 bytes) added ubuntu crash report (collapsed)

50.png

24 KiB

ChosenOne commented

2012-05-20 14:40:30 +00:00

Author

Owner

I have verified this with a fresh Ubuntu installation. The reporter is called "apport" and according to /etc/default/apport it is enabled by default now.

daira commented

2012-05-20 14:47:07 +00:00

Grr, silly Ubuntu devs making our lives harder for no good reason.

"If you notice further problems, try restarting the computer."

What is this, Windows? As if restarting will help for 99% of conditions this detects :-(

Grr, silly Ubuntu devs making our lives harder for no good reason. "If you notice further problems, try restarting the computer." What is this, Windows? As if restarting will help for 99% of conditions this detects :-(

daira added

and removed

labels 2012-05-20 14:47:07 +00:00

ChosenOne commented

2012-05-20 15:10:15 +00:00

Author

Owner

See files in /etc/python*/sitecustomize.py:

# install the apport exception handler if available
try:
    import apport_python_hook
except ImportError:
    pass
else:
    apport_python_hook.install()

I traced it back into the apport-package, ubuntu provides.
Installing the hook does the following

    sys.excepthook = apport_excepthook

This means that your python environment is modified like this:

print repr(sys.excepthook)
print sys.excepthook.func_name
print type(sys.excepthook)'
# <function apport_excepthook at 0x7fb84e5335f0>
# apport_excepthook
# <type 'function'>

A dirty, fix for this ticket could be (untested!)

import sys
import types # for comparison. can we do this better?
if types.FunctionType == type(sys.excepthook):
    if sys.excepthook.func_name == 'apport_excepthook':
        sys.excepthook = lambda *x: None

See files in /etc/python*/sitecustomize.py: ``` # install the apport exception handler if available try: import apport_python_hook except ImportError: pass else: apport_python_hook.install() ``` I traced it back into the apport-package, ubuntu provides. Installing the hook does the following ``` sys.excepthook = apport_excepthook ``` This means that your python environment is modified like this: ``` print repr(sys.excepthook) print sys.excepthook.func_name print type(sys.excepthook)' # <function apport_excepthook at 0x7fb84e5335f0> # apport_excepthook # <type 'function'> ``` A dirty, fix for this ticket could be (untested!) ``` import sys import types # for comparison. can we do this better? if types.FunctionType == type(sys.excepthook): if sys.excepthook.func_name == 'apport_excepthook': sys.excepthook = lambda *x: None ```

daira commented

2012-05-20 15:28:26 +00:00

Thanks ChosenOne for investigating this.

Knowing that it is implemented by sys.excepthook, we can easily prevent that from triggering, by catching exceptions at the entry point to the CLI.

Thanks ChosenOne for investigating this. Knowing that it is implemented by `sys.excepthook`, we can easily prevent that from triggering, by catching exceptions at the entry point to the CLI.

daira commented

2012-05-20 15:38:00 +00:00

Attachment fix-1746.darcs.patch (108289 bytes) added

Catch exceptions from CLI in order to prevent the Ubuntu crash monolog from triggering. refs #1746

**Attachment** fix-1746.darcs.patch (108289 bytes) added Catch exceptions from CLI in order to prevent the Ubuntu crash monolog from triggering. refs #1746

fix-1746.darcs.patch

106 KiB

daira commented

2012-05-20 15:39:08 +00:00

ChosenOne, please test this patch against trunk.

[ChosenOne](wiki/ChosenOne), please test this patch against trunk.

daira modified the milestone from undecided to soon

2012-05-20 15:39:08 +00:00

ChosenOne commented

2012-05-20 18:24:14 +00:00

Author

Owner

The exception remains, but apport is not started. Patch works.

warner commented

2012-05-20 18:35:00 +00:00

I'm less confident about doing this for "tahoe start", if only for the case where we eventually have a double-clickable "launch tahoe" icon, and if the only way that an exception during startup can be noticed is by a GUI apport hook that notices the exception and reports it in a dialog (monolog) box.

I guess at a more fundamental level, Ubuntu (via apport) is telling us that exceptions are not an acceptable way to deliver "user made a mistake" error messages to users, which I can't entirely disagree with (although it's still pretty annoying).

I'm +1 for this for CLI commands like "tahoe put" and friends (although maybe we should just rewrite those commands to never report gateway failures or bad arguments as exceptions, always emitting a nicer error message). I'm less confident about doing this for "tahoe start", if only for the case where we eventually have a double-clickable "launch tahoe" icon, and if the only way that an exception during startup can be noticed is by a GUI apport hook that notices the exception and reports it in a dialog (monolog) box. I guess at a more fundamental level, Ubuntu (via apport) is telling us that exceptions are not an acceptable way to deliver "user made a mistake" error messages to users, which I can't entirely disagree with (although it's still pretty annoying).

daira commented

2012-05-20 19:07:17 +00:00

If we wanted to trap uncaught errors and display a modal dialog, we should do that. But I can't think why we would want it to look anything like the Ubuntu one, and I dislike intensely the idea that the error reporting behaviour of our code should be suddenly changed by distribution developers who obviously have no clue about usable error reporting should work.

In any case, this patch doesn't change the (visible) behaviour of tahoe start from what it does without apport.

gdt commented

2012-05-20 19:12:08 +00:00

Author

Owner

In general, it seems a good rule is that python exceptions from CLI commands should only hit top-level and be user-perceptible if there is a code bug. Python backtraces are not really so different from "segementation violation -- core dumped".

zooko commented

2012-05-21 02:41:27 +00:00

I agree with gdt.

Once we've fixed all such places in our code, so that we never intentionally display a Python backtrace to a user, then the behavior of the apport hook may not be inappropriate.

Any case where our code displays a Python backtrace to a user should be ticketed with the error keyword:

https://tahoe-lafs.org/trac/tahoe-lafs/query?status=!closed&keywords=~error&order=priority

I agree with gdt. Once we've fixed all such places in our code, so that we never intentionally display a Python backtrace to a user, then the behavior of the apport hook may not be inappropriate. Any case where our code displays a Python backtrace to a user should be ticketed with the `error` keyword: <https://tahoe-lafs.org/trac/tahoe-lafs/query?status=!closed&keywords=~error&order=priority>

daira commented

2012-05-31 22:02:20 +00:00

In changeset:2ee1bc7148d45719:

Catch exceptions from CLI in order to prevent the Ubuntu crash monolog from triggering. refs #1746

In changeset:2ee1bc7148d45719: ``` Catch exceptions from CLI in order to prevent the Ubuntu crash monolog from triggering. refs #1746 ```

daira commented

2012-05-31 22:38:27 +00:00

This is fixed; please open another ticket or tickets for any inappropriate backtraces.

daira added the

r/fixed

label 2012-05-31 22:38:27 +00:00

daira modified the milestone from soon to 1.10.0

2012-05-31 22:38:27 +00:00

daira closed this issue

2012-05-31 22:38:27 +00:00

daira commented

2012-05-31 23:11:00 +00:00

Reopening because it needs a test.

daira removed the

r/fixed

label 2012-05-31 23:11:00 +00:00

daira reopened this issue

2012-05-31 23:11:00 +00:00

warner commented

2012-06-01 20:36:45 +00:00

updating title, priority

warner added

p/normal

and removed

p/major

labels 2012-06-01 20:36:45 +00:00

warner changed title from ~~IOError and verbose exceptions trigger Ubuntu crash reporter~~ to write test for anti-Ubuntu-crash-reporter exception-catching code

2012-06-01 20:36:45 +00:00

warner commented

2012-06-01 20:37:43 +00:00

oh, also, ChosenOne's work is done here, assigning back to davidsarah for the test-writing

daira commented

2012-07-16 16:33:56 +00:00

In changeset:5889/cloud-backend:

[rebased for cloud-backend] Catch exceptions from CLI in order to prevent the Ubuntu crash monolog from triggering. refs #1746

In changeset:5889/cloud-backend: ``` [rebased for cloud-backend] Catch exceptions from CLI in order to prevent the Ubuntu crash monolog from triggering. refs #1746 ```

daira commented

2013-01-03 03:22:50 +00:00

Please review: https://github.com/davidsarah/tahoe-lafs/commit/64e326e211d4b08e543feb98c45d382dce519fab

Please review: <https://github.com/davidsarah/tahoe-lafs/commit/64e326e211d4b08e543feb98c45d382dce519fab>

daira removed their assignment 2013-01-03 03:22:50 +00:00

zooko was assigned by daira

2013-01-03 03:22:50 +00:00

amiller commented

2013-01-31 16:13:53 +00:00

I've reviewed this patch and its test. The patch catches all exceptions in the CLI by having a try/catch around the 'run' function for all CLI commands. The test correctly distinguishes between when this exception is caught rather than propagated. I haven't directly checked that this solves the ubuntu app reporting problem, since I don't know how to build the ubuntu version. But ChosenOne's analysis about how catching the exception solves the problem makes sense.

I've reviewed this patch and its test. The patch catches all exceptions in the CLI by having a try/catch around the 'run' function for all CLI commands. The test correctly distinguishes between when this exception is caught rather than propagated. I haven't directly checked that this solves the ubuntu app reporting problem, since I don't know how to build the ubuntu version. But [ChosenOne](wiki/ChosenOne)'s analysis about how catching the exception solves the problem makes sense.