Opened 9 years ago

Closed 6 years ago

#1785 defect closed fixed (fixed)

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

Reported by: jml Owned by:
Priority: high Milestone: Twisted-9.0
Component: core Keywords:
Cc: therve, exarkun Branch: branches/reentrant-reactor.run-1785
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description


Change History (38)

comment:1 Changed 8 years ago by jml

  • Owner changed from jml to glyph

Re-assigning to core maintainer.

Sorry glyph.

comment:2 Changed 7 years ago by therve

  • Owner changed from glyph to therve

comment:3 Changed 7 years ago by therve

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

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

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

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

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

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

comment:9 Changed 7 years ago by therve

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

Refs #1785

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

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

(In [21060])

  • Fix the tests with the new behavior

Refs #1785.

comment:17 Changed 7 years ago by therve

  • Cc exarkun added
  • Keywords review added
  • Owner therve deleted

This is fixed.

comment:18 Changed 7 years ago by exarkun

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

(In [21115])

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

Refs #1785

comment:20 Changed 7 years ago by therve

(In [21116])

  • Simplify DummyReactor, remove unnecessary methods

Refs #1785.

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

  • Owner set to exarkun
  • Status changed from new to assigned

comment:23 Changed 7 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to therve
  • Status changed from assigned to new

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

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

Refs #1785.

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

comment:27 Changed 7 years ago by therve

(In [21354]) Typo

Refs #1785

comment:28 Changed 7 years ago by jml

  • Keywords review removed
  • Owner set to therve

Please land this branch.

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

  • Milestone set to Core-2.7
  • Priority changed from highest to high

comment:31 Changed 7 years ago by jml

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

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

comment:33 Changed 6 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/reentrant-reactor.run-1785

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

comment:34 Changed 6 years ago by exarkun

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

refs #1785

comment:35 Changed 6 years ago by exarkun

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

  • Keywords review removed
  • Owner set to exarkun

+1.

comment:37 Changed 6 years ago by exarkun

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

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

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