Opened 5 years ago

Last modified 10 months ago

#5962 defect new

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

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: core Keywords: easy
Cc: Michael Grazebrook Branch: branches/calllater-infinite-loop-5962
branch-diff, diff-cov, branch-cov, buildbot
Author: ashfall

Description (last modified by Itamar Turner-Trauring)

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 4 years ago.
clockinfloop_2.patch (2.8 KB) - added by Saurabh 4 years ago.
Added topfile, better variable names,comments.
callLater-5962-v1.patch (2.6 KB) - added by Michael Grazebrook 4 years ago.
callLater-5962-v2.patch (3.1 KB) - added by Michael Grazebrook 4 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 4 years ago by Itamar Turner-Trauring

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 4 years ago by Jean-Paul Calderone

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 4 years ago by Itamar Turner-Trauring

Description: modified (diff)

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

comment:4 Changed 4 years ago by rodney757

Owner: set to rodney757

Changed 4 years ago by Saurabh

Attachment: clockinfloop.patch added

comment:5 Changed 4 years ago by Saurabh

Keywords: review added
Owner: rodney757 deleted

comment:6 Changed 4 years ago by Itamar Turner-Trauring

Owner: set to Itamar Turner-Trauring

comment:7 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: changed from Itamar Turner-Trauring 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 4 years ago by Saurabh

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

Changed 4 years ago by Saurabh

Attachment: clockinfloop_2.patch added

Added topfile, better variable names,comments.

comment:9 Changed 4 years ago by Saurabh

Keywords: review added
Owner: Saurabh deleted

comment:10 Changed 4 years ago by Saurabh

Keywords: gsoc added

comment:11 Changed 4 years 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 4 years 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 4 years ago by Michael Grazebrook

Attachment: callLater-5962-v1.patch added

Changed 4 years ago by Michael Grazebrook

Attachment: callLater-5962-v2.patch added

comment:13 Changed 4 years ago by Michael Grazebrook

Cc: Michael Grazebrook 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 4 years ago by Itamar Turner-Trauring

Keywords: review added

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

comment:15 Changed 4 years ago by ashfall

Author: ashfall
Branch: branches/calllater-infinite-loop-5962

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

comment:16 Changed 4 years ago by ashfall

Owner: set to ashfall
Status: newassigned

comment:17 Changed 4 years ago by ashfall

Keywords: review removed
Owner: changed from ashfall to Michael Grazebrook
Status: assignednew

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 4 years 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 4 years ago by Michael Grazebrook

Owner: changed from Michael Grazebrook to ashfall

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

comment:20 Changed 4 years ago by ashfall

Owner: ashfall deleted

comment:21 Changed 10 months ago by JP "JPol" Polin

It seems like this still happens (16.4.1 on 2.7.12). Would it be worthwhile for me to pick this up?

Note: See TracTickets for help on using tickets.