Opened 2 years ago

Last modified 9 months ago

#5962 defect new

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

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: core Keywords: easy
Cc: michael@… Branch: branches/calllater-infinite-loop-5962
(diff, github, buildbot, log)
Author: ashfall Launchpad Bug:

Description (last modified by itamar)

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.

Attachments (4)

clockinfloop.patch (2.0 KB) - added by Saurabh 18 months ago.
clockinfloop_2.patch (2.8 KB) - added by Saurabh 18 months ago.
Added topfile, better variable names,comments.
callLater-5962-v1.patch (2.6 KB) - added by mgrazebrook 11 months ago.
callLater-5962-v2.patch (3.1 KB) - added by mgrazebrook 11 months ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 19 months ago by itamar

  • Keywords easy added

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.

comment:2 Changed 19 months ago by 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? :)

comment:3 Changed 19 months ago by itamar

  • Description modified (diff)

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

comment:4 Changed 19 months ago by rodney757

  • Owner set to rodney757

Changed 18 months ago by Saurabh

comment:5 Changed 18 months ago by Saurabh

  • Keywords review added
  • Owner rodney757 deleted

comment:6 Changed 18 months ago by itamar

  • Owner set to itamar

comment:7 Changed 18 months ago by itamar

  • Keywords review removed
  • Owner changed from itamar to Saurabh

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.

comment:8 Changed 18 months ago by Saurabh

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

Changed 18 months ago by Saurabh

Added topfile, better variable names,comments.

comment:9 Changed 18 months ago by Saurabh

  • Keywords review added
  • Owner Saurabh deleted

comment:10 Changed 18 months ago by Saurabh

  • Keywords gsoc added

comment:11 Changed 17 months ago by tom.prince

  • Keywords review removed
  • Owner set to Saurabh

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.

comment:12 Changed 13 months ago by Saurabh

  • Keywords gsoc removed

Changing, the logic from only amount == 0, to all amounts leads to errors as in this gist errors. I cannot understand the flaw in logic...will be glad if someone can help.

Changed 11 months ago by mgrazebrook

Changed 11 months ago by mgrazebrook

comment:13 Changed 11 months ago by mgrazebrook

  • Cc michael@… added
  • Owner Saurabh deleted

The original fix failed test; we uncovered two problems:

  • It could still be made to loop with advance(1)
  • When reset, the func attribute was deleted

We amended the code and this needs re-review. The new code:

  • Will not execute functions adding during the current execution of advance
  • Sets DelayedCall class variables to None instead of deleting them
  • Tests for None before executing func

Use callLater-5962-v2.patch

Investigating some test failures under Windows, which I may not complete myself. All test_task.py tests pass.

comment:14 Changed 11 months ago by itamar

  • Keywords review added

THanks for working on this! To ensure tickets get reviewed, please add the review keyword.

comment:15 Changed 9 months ago by ashfall

  • Author set to ashfall
  • Branch set to branches/calllater-infinite-loop-5962

(In [41421]) Branching to 'calllater-infinite-loop-5962'

comment:16 Changed 9 months ago by ashfall

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

comment:17 Changed 9 months ago by ashfall

  • Keywords review removed
  • Owner changed from ashfall to mgrazebrook
  • Status changed from assigned to new

Thanks for working on this!

  1. I forced a build on this, and there some build failures, mostly related to processes. So, it seems like Clock must have a missing test case, if these tests for other things fail.
  1. The names of all the new test modules must begin with test_, as per the Twisted Coding Standard
  1. Docstrings for the tests can be improved; in particular, for testAdvanceCallLaterZeroAdvanceOne, referring to the "original proposed fix" should be avoided, a test docstring should describe what the test method does.

Thanks again for your interest in this, please resubmit for review after addressing the above points.

comment:18 Changed 9 months ago by ashfall

  1. There's also a twistchecker error, naming-related. Please refer to the Coding Standard linked above to make sure twistedchecker is happy.

comment:19 Changed 9 months ago by mgrazebrook

  • Owner changed from mgrazebrook to ashfall

Thanks for looking at this. Due to current project pressure, I will not be able to continue this work.

comment:20 Changed 9 months ago by ashfall

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