Opened 11 years ago

Closed 8 years ago

#1785 defect closed fixed (fixed)

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

Reported by: Jonathan Lange Owned by:
Priority: high Milestone: Twisted-9.0
Component: core Keywords:
Cc: therve, Jean-Paul Calderone Branch: branches/reentrant-reactor.run-1785
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description


Change History (38)

comment:1 Changed 11 years ago by Jonathan Lange

Owner: changed from Jonathan Lange to Glyph

Re-assigning to core maintainer.

Sorry glyph.

comment:2 Changed 10 years ago by therve

Owner: changed from Glyph to therve

comment:3 Changed 10 years ago by therve

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

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

comment:4 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to therve

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

comment:5 in reply to:  4 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

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!

comment:6 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to therve

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.

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

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.

comment:8 Changed 10 years ago by Jonathan Lange

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

comment:9 Changed 10 years ago by therve

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

Refs #1785

comment:10 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

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

comment:11 Changed 10 years ago by radix

Keywords: review removed
Owner: set to therve

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

comment:12 in reply to:  11 Changed 10 years ago by therve

Keywords: review added
Owner: changed from therve to radix

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

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

comment:14 in reply to:  13 Changed 10 years ago by 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.

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

Keywords: review removed
Owner: changed from radix to therve

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')]

comment:16 Changed 10 years ago by therve

(In [21060])

  • Fix the tests with the new behavior

Refs #1785.

comment:17 Changed 10 years ago by therve

Cc: Jean-Paul Calderone added
Keywords: review added
Owner: therve deleted

This is fixed.

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

Keywords: review removed
Owner: set to therve

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

comment:19 Changed 10 years ago by therve

(In [21115])

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

Refs #1785

comment:20 Changed 10 years ago by therve

(In [21116])

  • Simplify DummyReactor, remove unnecessary methods

Refs #1785.

comment:21 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

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

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

Owner: set to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to therve
Status: assignednew

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

comment:24 Changed 10 years ago by therve

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

Refs #1785.

comment:25 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

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

Back to review.

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

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.

comment:27 Changed 10 years ago by therve

(In [21354]) Typo

Refs #1785

comment:28 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to therve

Please land this branch.

comment:29 Changed 10 years ago by 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.

comment:30 Changed 10 years ago by therve

Milestone: Core-2.7
Priority: highesthigh

comment:31 Changed 9 years ago by Jonathan Lange

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

comment:32 Changed 9 years ago by Jean-Paul Calderone

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.

comment:33 Changed 8 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/reentrant-reactor.run-1785

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

comment:34 Changed 8 years ago by Jean-Paul Calderone

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

refs #1785

comment:35 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added
Owner: therve deleted

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

comment:36 Changed 8 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

+1.

comment:37 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

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

comment:38 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.