Opened 10 years ago

Closed 10 years ago

#2457 enhancement closed fixed (fixed)

failed reactor selection is reported poorly by trial and twistd

Reported by: Jean-Paul Calderone Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve, Jean-Paul Calderone, Jonathan Lange Branch:
Author:

Description

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.

Change History (21)

comment:1 Changed 10 years ago by therve

Owner: changed from Glyph to therve

comment:2 Changed 10 years ago by therve

Cc: therve added
Keywords: review added
Owner: therve deleted
Priority: normalhighest

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.

comment:3 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

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.

comment:4 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

I added the tests, back to review.

comment:5 Changed 10 years ago by radix

Keywords: review removed
Owner: set to therve
  1. This looks good. Can you rename ReactorNotAvailable to something a bit more specific, like ReactorInstallationFailed or something.
  1. 80 columns

Thanks. Please merge after making these trivial modifications.

comment:6 Changed 10 years ago by therve

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

Refs #2457.

comment:7 Changed 10 years ago by therve

(In [20880]) 80 cols

Refs #2457.

comment:8 Changed 10 years ago by therve

Resolution: fixed
Status: newclosed

(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.

comment:9 Changed 10 years ago by therve

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

Refs #2457.

comment:10 Changed 10 years ago by therve

Resolution: fixed
Status: closedreopened

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

comment:11 Changed 10 years ago by therve

(In [20898]) Fix the formatting.

Refs #2457

comment:12 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted
Status: reopenednew

That should be better now, sorry for the noise.

comment:13 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to therve

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.

comment:14 Changed 10 years ago by therve

(In [21054])

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

Refs #2457

comment:15 in reply to:  13 Changed 10 years ago by therve

Keywords: review added
Owner: changed from therve to Jonathan Lange

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.

comment:16 Changed 10 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone Jonathan Lange added
Keywords: review removed
Owner: changed from Jonathan Lange to therve

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.

comment:17 Changed 10 years ago by therve

(In [21193])

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

Refs #2457.

comment:18 in reply to:  16 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

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.

comment:19 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to therve

Merge away.

comment:20 Changed 10 years ago by therve

Resolution: fixed
Status: newclosed

(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.

comment:21 Changed 6 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.