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

[jack]

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.

[jack]

change get to pop in _continueSystemEvent

[therve]

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.

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

[glyph]

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.

[therve]

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 ?

[exarkun]

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

[therve]

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

[exarkun]

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.

[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

[therve]

• 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!

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

[exarkun]

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? :)

[therve]

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.

[exarkun]

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.

[therve]

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

[exarkun]

Opened #2764

[exarkun]

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