failed reactor selection is reported poorly by trial and twistd

[exarkun]

There are at least two failure modes for reactor selection:

• the specified reactor doesn't exist at all
• the reactor specified exists but cannot be imported (eg, due to a missing extension module)

Both cases currently result in a traceback from either trial or twistd. At least one of them can definitely be handled more nicely.

[therve]

I did something really simple in reactor-selection-2457, adding a new exception, and using it in the ReactorSelectionMixin.

The only doubt I have is whether or not we should print the traceback in case of ImportError. For now I hide it, but it might be useful.

Please review.

[exarkun]

Hmm, yea, swallowing the traceback from installer.install() is a little bit nasty. It might be nice to have a verbose mode which spits it out... but that's probably out of scope for this ticket.

The twisted/application/app.py changes should have unit tests, though.

[therve]

I added the tests, back to review.

[radix]

1. This looks good. Can you rename ReactorNotAvailable to something a bit more specific, like ReactorInstallationFailed or something.

2. 80 columns

Thanks. Please merge after making these trivial modifications.

[therve]

(In [20879]) Rename to ReactorInstallationFailed, 80 cols.

Refs #2457.

[therve]

(In [20880]) 80 cols

Refs #2457.

[therve]

(In [20881]) Merge reactor-selection-2457

Author: therve Reviewers: radix, exarkun Fixes #2457

Add explicit messages to the reactor selection used by twistd and trial so that not existent reactors and not available reactors are reported better without ugly traceback.

[therve]

(In [20897]) Revert [20881]: regression on --help-reactors.

Refs #2457.

[therve]

I did a stupid change on help_reactors that broke the formatting.

[therve]

(In [20898]) Fix the formatting.

Refs #2457

[therve]

That should be better now, sorry for the noise.

[jml]

Thanks for doing this. Having better errors on 'reactor missing' is a really nice thing to have.

Things you need to change:

• The docstrings for test_reactorSelectionMixinNonExistent and test_reactorSelectionMixinNotAvailable refer to "when passing <an error condition>". I'm unclear on what is actually being said here (in particular, what do you mean by "passing"?). Please make these docstrings clearer.
• Almost all the tests in PluggableReactorTestCase are buggy. They change a global variable, install, without restoring it to its original value. This leads to erratic or fragile tests. I know you didn't write these tests in the first place, but this really should be fixed asap. Saving install in setUp and restoring it in tearDown should be enough.

Things you don't need to change, but that I need to mention:

• I think that the self.pluginResults = [...] stuff is confusing. You need to know how the test fixture works in order to understand the test.

Questions:

• I'm uncertain about ReactorInstallationFailed. Why do you think it would be a good idea to make all reactor installation errors friendly ones? I think it's good to have a special exception for reactors to raise when they want to display a friendly error, but I do not think this should be the default. The implementation in your branch means that NameErrors and other things will have their tracebacks hidden.

Please re-assign to me for review when you've made the changes and answered my question.

Thanks again.

[therve]

(In [21054])

• Add/cleanup docstrings
• Add a FakeReactor to hook the install method and remove the global install

Refs #2457

[therve]

Thanks for the comments. I think I've fixed all the issues above.

Replying to jml:

Questions: * I'm uncertain about ReactorInstallationFailed. Why do you think it would be a good idea to make all reactor installation errors friendly ones? I think it's good to have a special exception for reactors to raise when they want to display a friendly error, but I do not think this should be the default. The implementation in your branch means that NameErrors and other things will have their tracebacks hidden.

It's true that all tracebacks will be swallow by this change. That has already been brought up by exarkun. The real advantage of this solution is that I don't have to modify all reactors to change the exceptions raised (which can be very different). Note that the message given by the exception is forwarded, which is better than nothing.

Do you have a suggestion? The only one I see is to modify every reactors, but I don't think this very helpful.

[exarkun]

I guess one other possible solution is to drop ReactorInstallationFailed, declare that install can raise any random Exception subclass, and handle Exception in opt_reactor where it currently handles ReactorInstallationFailed. This means reactor implementations don't need to change and that the traceback is available to the code which does the logging. Of course, if the code that does the logging isn't also changed in some other way, the traceback is still lost. It does have the benefit of requiring slightly less code to achieve the same result, though.

Having a function that can raise any exception at all feels a little bad, but I think that's the nature of this API. It's job is to load some arbitrary code. Basically any random bad thing could result from this.

On a different topic, I think it's best for Options subclasses to restrict themselves to raising UsageError. The ideal pattern is for the code that calls parseOptions to handle UsageError in whatever way is most appropriate (ie, writing to stdout and then exiting with an error code). This lets the Options class focus on options and not worry about user interface issues, and lets the calling code really make the decision about what the interface is (ie, it might be presenting the options in a GUI window). Sorry I didn't say something like this in my earlier review (not sure it occurred to me at the time - slight upside (or is it a downside, I dunno) of having a long time pass between reviews, it's like I'm two different people sometimes).

Despite those two issues, this branch basically looks good to me. I'm not sure if jml has further concerns (can you comment if you do, jml?) but I wouldn't be unhappy to see this branch merged as it is now. If you can make any improvements based on what I've said above, that would just be icing on the cake.

[therve]

(In [21193])

• Remove ReactorInstallationFailed and use direct exception instead
• Use UsageError instead of writing to stdout

Refs #2457.

[therve]

Replying to exarkun:

Sorry I didn't say something like this in my earlier review (not sure it occurred to me at the time - slight upside (or is it a downside, I dunno) of having a long time pass between reviews, it's like I'm two different people sometimes).

I would be upset if it wasn't totally true :). Back to review, with above comments (hopefully) corrected.

[jml]

Merge away.

[therve]

(In [21338]) Merge reactor-selection-2457

Author: therve Reviewers: jml, exarkun, radix Fixes #2457

Add exception management to the reactor selection mixin used by twistd and trial so that non-existent reactors and not available reactors are reported better without ugly traceback.