Opened 8 years ago

Closed 7 years ago

#2457 enhancement closed fixed (fixed)

failed reactor selection is reported poorly by trial and twistd

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve, exarkun, jml Branch:
Author: Launchpad Bug:

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 7 years ago by therve

  • Owner changed from glyph to therve

comment:2 Changed 7 years ago by therve

  • Cc therve added
  • Keywords review added
  • Owner therve deleted
  • Priority changed from normal to highest

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 7 years ago by exarkun

  • 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 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

I added the tests, back to review.

comment:5 Changed 7 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 7 years ago by therve

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

Refs #2457.

comment:7 Changed 7 years ago by therve

(In [20880]) 80 cols

Refs #2457.

comment:8 Changed 7 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(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 7 years ago by therve

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

Refs #2457.

comment:10 Changed 7 years ago by therve

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:11 Changed 7 years ago by therve

(In [20898]) Fix the formatting.

Refs #2457

comment:12 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted
  • Status changed from reopened to new

That should be better now, sorry for the noise.

comment:13 follow-up: Changed 7 years ago by jml

  • 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 7 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 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to jml

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 follow-up: Changed 7 years ago by exarkun

  • Cc exarkun jml added
  • Keywords review removed
  • Owner changed from jml 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 7 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 7 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 7 years ago by jml

  • Keywords review removed
  • Owner set to therve

Merge away.

comment:20 Changed 7 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

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