Clock.callLater(0, f) can lead to infinite loop

[exarkun]

Consider:

from twisted.internet.task import Clock

def f():
    c.callLater(0, f)

c = Clock()
f()
c.advance(0)
print 'Done'

The expected behavior is that f runs once and then Done is printed and the program exits. Instead, Clock.advance spends forever running and re-running f. This is similar to a bug that the real IReactorTime implementation used to have but that we fixed.

[itamar]

This ticket is easy if it only fixes the Clock issue. In which the "it would be nice" part of merging it with the reactor implementation should get its own ticket.

[exarkun]

In which the "it would be nice" part of merging it with the reactor implementation should get its own ticket.

Want to file that? :)

[itamar]

OK, I moved the "it would be nice" feature to a separate ticket, #6425.

[itamar]

Thanks for the patch! The basic logic makes sense to me, and the test does indeed fail without the code and pass with it.

There remain a few minor issues:

1. The code is confusing if you don't know the reason tempCalls exists; perhaps some explanatory comments, and better variable names ("_insideAdvance" maybe instead of "checkLoop"?) would make it clearer why that code exists.
2. The code should have a news file ( https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles). If you "svn add" it then it will be included when you run "svn diff".
3. The tests variables really don't need to be set on self, just use local variables.
4. The code should follow the coding standard; in particular the test code does not. Keep in mind that some code is old and doesn't quite follow the standard yet. E.g. test methods should start with "test_", but other tests in that module don't yet.
5. The docstring for the test should explain in more detail what behavior you expect. Also, it shouldn't start with "check that" or "test that". Rather, something like "If a function called by Clock.advance(0) reschedules itself with a delay of 0, then it should" etc..

Please resubmit with fixes for these issues.

[Saurabh]

Thanks for the reply and suggestions! I will work on those issues now.

[Saurabh]

Added topfile, better variable names,comments.

[tom.prince]

Thanks for your contribution. Sorry for the delay in the response.

1. Instead of comments in __init__ describing the instance variables, they should be documented in the class docstring with @ivar` epytext markup.
2. I wonder what happens if advance is called during a call to advance? I think it would be possible to trigger an infinite loop in this case. Perhaps the answer is "don't do that".
3. Why is the additional logic only triggered when amount == 0? I think an infinite loop would still trigger, if amount > 0.
4. In the docstring for test_AdvanceInfiniteLoop, you shouldn't describe the particular solution you used to fix the test, particularly, since the solution you describe isn't the only possible solution.
   • The first letter of the name should be lowercase. See the  coding standard.
   • The test does test that the implementation won't lead to an infinite loop. But, it won't actually lead to an infitite loop. It would be good to explain what the test directly tests (in addition to explaning why it exists).
5. I had a look at the implemention in the real reactor, and noticed that it only adds new calls to the list of calls and sorts them just before processing them. I wonder if doing something like this might address 2. (Doing this would probably involve changing getDelayedCalls as well.

Please resubmit for revew after addressing 1-4.