Opened 8 years ago

Closed 7 years ago

#2509 defect closed fixed (fixed)

removing event triggers while in an event trigger callback does bad things

Reported by: jack Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: twonds, therve, yaubi Branch:
Author: Launchpad Bug:

Description

See the attached test case. Several callWhenRunning triggers are set up, but not all of them are called.

Note that removing an event trigger from the callback was done to work around a bug in trial where reactor events don't happen as one would expect. See #2498.

I'm not sure what the fix would be but the attached preliminary patch seems to be right thing.

Attachments (2)

test_triggerbug.py (709 bytes) - added by jack 8 years ago.
base.diff (1.1 KB) - added by jack 8 years ago.
change get to pop in _continueSystemEvent

Download all attachments as: .zip

Change History (22)

Changed 8 years ago by jack

Changed 8 years ago by jack

change get to pop in _continueSystemEvent

comment:1 Changed 8 years ago by jack

  • Keywords review added
  • Owner glyph deleted

comment:2 Changed 8 years ago by therve

  • Keywords reactor review removed
  • Owner set to therve

comment:3 Changed 8 years ago by therve

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

The test was (almost) good, but the fix wasn't. I fixed it by creating a new list, in both before and during/after trigger.

Please review in event-trigger-2509.

comment:4 Changed 8 years ago by therve

It seems it the same problem as in #1739. The solution rejected there is the same here, so maybe it'll get the same treatment :).

comment:5 Changed 8 years ago by glyph

  • Keywords review removed
  • Owner set to therve

Yeah, I have to say it still should be fixed for real. At least having it non-reentrant with as well as without Deferreds causes the error to show up early. If it is fixed in this way, I think it will make a more insidious kind of bug a lot harder to find.

comment:6 Changed 8 years ago by therve

  • Keywords review added
  • Owner therve deleted

I tried a fix, but a test is failing, so it means some users could rely on previous (broken) behaviour. Maybe we can add a warning ?

comment:7 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

I think we can continue to support the behavior expected by the test broken in this branch. It only requires more carefully tracking the state of system triggers as they are being executed.

A perenial problem in Twisted is the use of piles of ad-hoc attributes to
implement complex state machines. Rather, an explicit state machine design
should be used (consider
http://divmod.org/trac/browser/trunk/Epsilon/epsilon/modal.py as an example of
what I mean by that, although I am not necessarily suggesting using that code
specifically), as this makes it much easier to cover all possible cases, both
while writing the code and while reading it later on.

Adding system event triggers is probably fine at just about any time, but it
must be done differently depending on whether that kind of trigger is currently
being processed or not. Similarly, removing them is fine all the time,
provided the thing being removed has actually been added and not yet
executed
. The problem here seems to be insufficient checking for the not
yet executed
case. If we're more careful there, the current behavior can be
preserved, except for the bug, which can be removed.

I can provide more detail and suggestions if that would be useful (I did in a previous version of this comment but I lost the firefox window and so all of the text and I am feeling too lazy to retype it at the moment).

comment:8 Changed 7 years ago by therve

  • Owner changed from therve to exarkun

I guess I'm a bit stuck here. I don't know what's the expected behavior, and what's design you want.

comment:9 Changed 7 years ago by yaubi

  • Cc yaubi added

comment:10 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Okay, hopefully event-trigger-2509-4 has a good resolution for this.

I think my previous comment might have been somewhat misguided. After actually looking at the code, I see that therve had the right idea about adding a warning for the behavior in question, so I went ahead and did that.

I think there are a couple deficiencies in the branch:

  • the log.err() calls in _ThreePhaseEvent don't have arguments for the _why parameter. I don't see a reasonable way to write a test for such an argument.
  • The deprecation will point to the reactor implementation. It might be better if it pointed to the function which actually performed the deprecated action.

comment:11 Changed 7 years ago by therve

A few comments (not a full review):

  • in #1785, I've started to use a DummyReactor to test things in the ReactorBase class. I think it's cleaner than using the toplevel reactor, for SystemEventTestCase in particular (Note: we must merge this branch first :)).
  • removeTrigger_BEFORE with phase != 'before' is not tested
  • I would prefer 'in a future version of Twisted' to be more precise. Maybe that doesn't make sense here though, that'll be an arbitrary value anyway.
  • unused import types in test_internet

comment:12 follow-up: Changed 7 years ago by therve

  • Keywords review removed
  • Owner set to exarkun
  • You removed the callLater(0) in fireEvent. That may be an improvement, I just wanted to be sure it was intentional.
  • it would be great if fireSystemEvent could return a Deferred which fires when everything has been fired. This usecase might push the use of maybeDeferred everywhere instead of using try/except and the isinstance test in the before phase.
  • having the state variable only taking the BASE and BEFORE values defeats a bit the advantage of the state machine. I'd like to see at least 'DURING' and 'FINISH' states. Also, I think removeTrigger_BEFORE shouldn't call removeTrigger_BASE, but a common method.
  • the leading underscore of _ThreePhaseEvent makes it disappear from the apidocs. It's a shame with all this cool docstrings :).

Thanks!

comment:13 in reply to: ↑ 12 Changed 7 years ago by glyph

Replying to therve:

  • the leading underscore of _ThreePhaseEvent makes it disappear from the apidocs. It's a shame with all this cool docstrings :).

There should be a set of "maintainer" API docs that includes private stuff. I didn't even realize that pydoctor had implemented hiding yet. Just because it's well documented does not mean it should be public ;).

comment:14 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to therve

You removed the callLater(0) in fireEvent. That may be an improvement, I just wanted to be sure it was intentional.

The callLater(0 was indeed removed intentionally. I hope it was a good idea. It is a change in behavior, I suppose: eg, reactor.stop() will run during and after triggers synchronously now, whereas it would not before. I don't think there's any documentation which promises either behavior.

it would be great if fireSystemEvent could return a Deferred which fires when everything has been fired. This usecase might push the use of maybeDeferred everywhere instead of using try/except and the isinstance test in the before phase.

fireSystemEvent doesn't get used directly much, so probably no one would notice if it returned a Deferred. The isinstance check doesn't bother me so much. If you want, you can file a new enhancement though.

having the state variable only taking the BASE and BEFORE values defeats a bit the advantage of the state machine. I'd like to see at least 'DURING' and 'FINISH' states. Also, I think removeTrigger_BEFORE shouldn't call removeTrigger_BASE, but a common method.

I agree having DURING and FINISH states would be more complete, but I can't think of any practical difference it would make. Nothing can actually get a reference to the _ThreePhaseEvent instance through any public API, so nothing can check the state attribute and notice it is inconsistent during those two phases. Unless I missed something, I think it can probably stay how it is.

I tried having removeTrigger_BEFORE and _BASE call a common implementation, but since removeTrigger_BASE doesn't actually do anything else at all, it didn't seem to buy much. removeTrigger_BASE ended up just calling the other method, and the two obvious changes to removeTrigger_BEFORE also didn't seem very high impact.

the leading underscore of _ThreePhaseEvent makes it disappear from the apidocs. It's a shame with all this cool docstrings :).

Yea. Maybe time for a pydoctor enhancement ticket? :)

comment:15 Changed 7 years ago by therve

  • Keywords review removed
  • Owner changed from therve to exarkun

Hey you cheated you give it back without writing any code :).

The only useful thing would be to enhance fireSystemEvent, but that out of scope so I'll keep that for me.

Go ahead and merge.

comment:16 Changed 7 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to therve

Oops, forgot to address the things from the first review comment. I removed the unused import and added three new test methods. #1785 is still outstanding and I don't know what release of Twisted will replace the deprecation with an error, so I didn't do anything about those.

comment:17 Changed 7 years ago by therve

  • Keywords review removed
  • Owner changed from therve to exarkun

OK. Please open a ticket to decide when the deprecation will be replaced by an error after merging.

comment:18 Changed 7 years ago by exarkun

Opened #2764

comment:19 Changed 7 years ago by exarkun

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

(In [20909]) Merge event-trigger-2509-4

Author: exarkun
Reviewer: therve
Fixes #2509

Factor most of the implementation of the system event parts of IReactorCore
into a separate helper class with more rigorous testing for edge cases
which were previously implemented incorrectly. Add a warning to an application
behavior which is probably buggy (removing an already-executed before-phase
trigger during before-phase execution).

comment:20 Changed 4 years ago by <automation>

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