Opened 11 years ago

Closed 7 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
branch-diff, diff-cov, branch-cov, buildbot
Author:

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 Andrii V. Mishkovskyi 7 years ago.
First attempt at the actual fix

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 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 11 years ago by TimothyFitz

Cc: TimothyFitz added

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

Keywords: easy added
Owner: TimothyFitz deleted
Priority: normalhighest

Changed 7 years ago by Andrii V. Mishkovskyi

Attachment: 2066.patch added

First attempt at the actual fix

comment:4 Changed 7 years ago by Andrii V. Mishkovskyi

Keywords: review added

comment:5 Changed 7 years ago by Andrii V. Mishkovskyi

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

comment:6 Changed 7 years ago by lvh

Author: lvh
Branch: branches/reactor-restart-raise-2066

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

comment:7 Changed 7 years ago by lvh

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

comment:6 Changed 7 years ago by lvh

Author: lvh

comment:7 Changed 7 years ago by lvh

Keywords: review removed

Looks great! Merging.

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

Resolution: fixed
Status: newclosed

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