reactor.run should raise an error when called while the reactor is running

[jml]

Re-assigning to core maintainer.

Sorry glyph.

[therve]

Ready to review in run-error-1785. I did some (nice ?) refactoring in test_internet in the way.

[jml]

test_internet.py is definitely a lot cleaner. Thanks.

In test_internet.py, the docstring for DummyReactor says "A dummy reactor overriding string necessary to make core starts." I don't know what it means by 'overriding string'. Please make the docstring clearer.

In test_internet.py, test_stopWhenNotRunning, you advance the clock before trying a second self.reactor.stop. I don't know why you do that, so it would be good to add a comment explaining why.

For the same test, it might be a good idea to split into two unit tests.

You add a startRunning method to ReactorBase, but it's not in any of the documented reactor interfaces. It also appears to have a slightly different signature to startRunning on other reactors. If appropriate, please add a method to the right interface and make it take an installSignalHandlers option. If this is outside the scope of this branch, please file a bug to do same.

Thanks, jml

[therve]

Replying to jml:

test_internet.py is definitely a lot cleaner. Thanks. In test_internet.py, the docstring for DummyReactor says "A dummy reactor overriding string necessary to make core starts." I don't know what it means by 'overriding string'. Please make the docstring clearer. In test_internet.py, test_stopWhenNotRunning, you advance the clock before trying a second self.reactor.stop. I don't know why you do that, so it would be good to add a comment explaining why. For the same test, it might be a good idea to split into two unit tests.

OK, everything above should be done.

You add a startRunning method to ReactorBase, but it's not in any of the documented reactor interfaces. It also appears to have a slightly different signature to startRunning on other reactors. If appropriate, please add a method to the right interface and make it take an installSignalHandlers option. If this is outside the scope of this branch, please file a bug to do same.

I think that resolving this issue needs a little bit more visibility than this ticket. There are already some differences, like the installSignalHandler parameter which is not in the interface. Today we have:

    def run():
        """Fire 'startup' System Events, move the reactor to the 'running'
        state, then run the main loop until it is stopped with stop() or
        crash().
        """

Something like that might be good:

    def startRunning(installSignalHandlers):
        """Fire 'startup' System Events and move the reactor to the 'running'
        state.
        """

    def run(installSignalHandlers=True):
        """Call starRunning, then run the main loop until it is stopped with stop() or
        crash().
        """

Thanks,

Thanks for the review!

[jml]

Pyflakes:

jml@rhino:~/Code/Twisted/trunk$ pyflakes twisted/test/test_internet.py
twisted/test/test_internet.py:14: redefinition of unused 'ssl' from line 12
twisted/test/test_internet.py:597: redefinition of fuction 'test_stopWhenNotStarted' from line 589
jml@rhino:~/Code/Twisted/trunk$ pyflakes twisted/internet/posixbase.py
twisted/internet/posixbase.py:59: redefinition of unused 'win32process' from line 56
jml@rhino:~/Code/Twisted/trunk$ pyflakes twisted/internet/iocpreactor/proactor.py
twisted/internet/iocpreactor/proactor.py:5: 'defer' imported but unused
twisted/internet/iocpreactor/proactor.py:8: 'implements' imported but unused
jml@rhino:~/Code/Twisted/trunk$ pyflakes twisted/internet/error.py
jml@rhino:~/Code/Twisted/trunk$ pyflakes twisted/internet/base.py
twisted/internet/base.py:25: redefinition of unused 'fcntl' from line 23
twisted/internet/base.py:655: redefinition of fuction 'callFromThread' from line 607

(Is there some more convenient way of doing pyflakes on all changed files?)

Please correct these flakes.

Please file a bug for changing the interface as you discussed -- I think your suggestions are good ones.

Once those are done, feel free to merge.

[exarkun]

Every method on every class in every reactor implementation inheritance hierarchy doesn't need to be on an interface.

Specifically, there shouldn't be a startRunning in addition to run on any reactor interface.

[jml]

Then it's purpose should be clearer. At the least, the docstring could say something like "Don't call this directly, call reactor.run() instead." (Although if it's an internal method then it really should begin with an underscore.)

Speaking of the docstring, "Method called when the reactor stars:" should become "Method called when the reactor starts:". twisted/internet/base.py

[therve]

(In [20750]) Add a new error, fix docstring.

Refs #1785

[therve]

I fixed the docstring, and corrected the flakes I could. Back to review.

[radix]

[1] I really think "reactor.stop(); reactor.stop()" should raise an exception from the second call. Maybe it shouldn't be the ReactorNotRunning, perhaps it should be something like ReactorStopping. I'm not sure the best way to do this; the only thing that immediately comes to mind is by changing the "stopped" attribute to "running_state" which can be in RUNNING, STOPPING, and STOPPED states.

[2] + @raise error.ReactorAlreadyRunning: raised if trying to start the + reactor when is already started.

You need an "it" between "when" and "is".

Thanks a lot for the improvements in test_internet.

[therve]

Replying to radix:

[1] I really think "reactor.stop(); reactor.stop()" should raise an exception from the second call. Maybe it shouldn't be the ReactorNotRunning, perhaps it should be something like ReactorStopping. I'm not sure the best way to do this; the only thing that immediately comes to mind is by changing the "stopped" attribute to "running_state" which can be in RUNNING, STOPPING, and STOPPED states.

I'm a bit reluctant to add this here. Of course, I've modified stop() so it could look strange, but I didn't add any attributes. Would it be OK to create another ticket for that?

[2] + @raise error.ReactorAlreadyRunning: raised if trying to start the + reactor when is already started. You need an "it" between "when" and "is".

Done

[glyph]

Sorry for being late to the party here, but why aren't we deprecating this before we're making it raise an exception? It seems likely that we are going to break some working code.

c.f. CompatibilityPolicy

[therve]

Replying to glyph:

Sorry for being late to the party here, but why aren't we deprecating this before we're making it raise an exception? It seems likely that we are going to break some working code. c.f. CompatibilityPolicy

I'm not sure this is working code. But I guess we could do that.

[exarkun]

twisted.test.test_internet.ReactorBaseTestCase.test_stopShutDownEvents fails for me:

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_internet.py", line 1072, in test_stopShutDownEvents
    self.assertEquals(events, [("before", "shutdown")])
twisted.trial.unittest.FailTest: [('before', 'shutdown'), ('during', 'shutdown')] != [('before', 'shutdown')]

[therve]

(In [21060])

• Fix the tests with the new behavior

Refs #1785.

[therve]

This is fixed.

[exarkun]

Changes look good. The tests look a little weird though, probably because they're trying to squeeze into a really tight corner and test some unusual things.

Some thoughts:

• reactor.run() is really supposed to block. Having a dummy reactor with a non-blocking run implementation feels wrong, like the tests aren't really exercising the important codepaths. Earlier I commented that startRunning doesn't belong on any interface, and I still think that's true, but it might be better for the tests if they didn't worry about the interface for a moment and called startRunning directly. Then the dummy reactor wouldn't need a run implementation at all, and the tests would actually be directly testing the real implementation of startRunning.
• installWaker probably shouldn't be required here. I looked through the code to see why it was, and I noticed that there are two separate call sites which install a waker. One of them certainly isn't invoked by the tests, PosixReactorBase.__init__, since the tests don't get that class involved. The other is the one you were protecting against, I suppose - ReactorBase._initThreads. I guess this isn't really harmful, since installWaker (at least the one in posixbase.py) guards itself against multiple installation attempts. Still, this seems suboptimal. Maybe it's a candidate for another ticket.
• doIteration seems genuinely unnecessary (I commented it out and ran test_internet, everything passed)

For extra happy fun joy points, gtk and gtk2 reactors both fail to run the test suite now, exploding terribly with a traceback ending in:

  File "/home/exarkun/Projects/Twisted/trunk/twisted/internet/gtk2reactor.py", line 173, in run
    self.startRunning(installSignalHandlers=installSignalHandlers)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/internet/posixbase.py", line 219, in startRunning
    ReactorBase.startRunning(self)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/internet/base.py", line 573, in startRunning
    raise error.ReactorAlreadyRunning(
twisted.internet.error.ReactorAlreadyRunning: Can't start a reactor multiple times

[therve]

(In [21115])

• Call crash on parent to get the good value of the running flag

Refs #1785

[therve]

(In [21116])

• Simplify DummyReactor, remove unnecessary methods

Refs #1785.

[therve]

OK, the problem with gtk/gtk2 wasn't so terrible. I also made the cleanups mentioned in the review. Thanks!

[exarkun]

Looks good to me. But...

To revisit, glyph's point: compatibility. This is a very hairy area of Twisted. I'm a bit torn about whether or not it's worth the effort to go through the deprecation phase for this change. You could do something before - though it worked only by accident. I guess a major point of the backwards compatibility policy is that it isn't a choice whether or not to change an API without first deprecating the old behavior: we just don't do it.

So I guess startRunning should emit a DeprecationWarning (although we should really decide that we're going to start doing PendingDeprecationWarnings first at some point) instead of raising an exception.

(I tried pretty hard to arrive at a different conclusion but I couldn't manage to, sorry.)

[therve]

(In [21194]) Replace the exception by a warning.

Refs #1785.

[therve]

Fine, let's first warn user. See the somewhat limited change above.

Back to review.

[exarkun]

I started to review this, but then Firefox screwed up so I have to restart it. The only comment I have so far is that there is a typo in ReactorCoreTestCase.__doc__: functionnalities.

[therve]

(In [21354]) Typo

Refs #1785

[jml]

Please land this branch.

[therve]

(In [21395]) Merge run-error-1785-2

Author: therve Reviewers: jml, exarkun, radix Refs #1785

Make the reactor issue a warning when started a second time.

[jml]

Why is this ticket still open? What needs to be done?

[exarkun]

There's a workflow issue here. The ticket is that a particular change which is not backwards compatible should be implemented. By accident (at least, unplanning from when the ticket was created), what ended up happening is that the existing behavior was deprecated, with the intent of making the behavioral change in a future release. The ticket is unresolved since the behavior it describes is unimplemented. This isn't what we usually do, obviously.

[exarkun]

(In [25850]) Branching to 'reentrant-reactor.run-1785'

[exarkun]

(In [25851]) Make the warning into an exception; update the test to reflect the new behavior

refs #1785

[exarkun]

The warning was added for 8.0, which was released in March of 2008. That's not quite a year ago. It'll probably be pretty close by the time 9.0 is actually released, though. This is also an egregious misuse of a Twisted API which was never intended to be supported and never actually worked reliably. I would be happy to bend the almost-adopted compatibility policy (which indicates a full year of deprecation before changing the behavior to raise an exception) in this case to allow a slightly shorter period of time (at least ten months at this point, and in practice it probably will be a full year, based on our past release cycles).

[therve]

+1.

[exarkun]

(In [25935]) Merge reentrant-reactor.run-1785

Author: exarkun Reviewer: therve Fixes: #1785

Replace the deprecation warning emitted when reentrantly calling reactor.run with an exception.