Opened 8 years ago

Closed 4 years ago

#2066 enhancement closed fixed (fixed)

Restarting a reactor should raise an exception

Reported by: TimothyFitz Owned by:
Priority: highest Milestone:
Component: core Keywords: easy
Cc: TimothyFitz Branch: branches/reactor-restart-raise-2066
(diff, github, buildbot, log)
Author: Launchpad Bug:

Description

About once a month someone asks twisted-python why they have really strange behaviour, and happen to be restarting the reactor. Restarting the reactor is unsupported (currently, by all implementors, nothing prevents it from being implemented) so these all fail, but always fail poorly. A call to reactor.run() after reactor.stop() should raise an exception that clearly says "This reactor does not support restarting" or "Reactors do not support restarting".

Attachments (1)

2066.patch (2.7 KB) - added by mishok13 4 years ago.
First attempt at the actual fix

Download all attachments as: .zip

Change History (12)

comment:1 Changed 8 years ago by TimothyFitz

Glyph Lefkowitz

On Thu, 7 Sep 2006 15:01:08 -0400, James Y Knight <foom@…> wrote:

However, even though it'd be possible to fix, perhaps it is better to
simply throw an exception on the second reactor.run(), since most people
who do that shouldn't actually be running the reactor twice, they just
didn't know the better thing to do yet.

I think that's a good idea. Based on the nature of the questions that I've seen, I >think that this would probably be a good idea even if we *did* have a restartable >reactor. Add an interface, IRestartableReactor, and require people who want to use >it to call 'reactor.restart()'. While this might not actually *do* anything, the >conspicuous absence of it from any example code and a potentially useful error >message from reactor.run would prevent the question from being asked repeatedly...

This bug will cover making reactor.run() raise on a second call. Someone else can pick up IRestartableReactor and the like.

comment:2 Changed 8 years ago by TimothyFitz

  • Cc TimothyFitz added

comment:3 Changed 4 years ago by exarkun

  • Keywords easy added
  • Owner TimothyFitz deleted
  • Priority changed from normal to highest

Changed 4 years ago by mishok13

First attempt at the actual fix

comment:4 Changed 4 years ago by mishok13

  • Keywords review added

comment:5 Changed 4 years ago by mishok13

The test is basically a rewrite of test_multipleRun and the patch introduces new instance variable (which sucks).

comment:6 Changed 4 years ago by lvh

  • Author set to lvh
  • Branch set to branches/reactor-restart-raise-2066

(In [31008]) Branching to 'reactor-restart-raise-2066'

comment:7 Changed 4 years ago by lvh

(In [31009]) Apply patch, refs #2066

comment:6 Changed 4 years ago by lvh

  • Author lvh deleted

comment:7 Changed 4 years ago by lvh

  • Keywords review removed

Looks great! Merging.

comment:7 Changed 4 years ago by lvh

(Okay, that's actually a lie: it's missing a topfile, but I'm going to add that for you)

comment:11 Changed 4 years ago by lvh

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

(In [31011]) Raise when you try to restart an unrestartable reactor

Author: mishok13
Reviewer: lvh
Fixes: #2066

Unrestartable reactors not complaining was confusing. When you try to do it, you get an exception now, which is a much nicer failure mode.

Note: See TracTickets for help on using tickets.