Opened 5 years ago

Closed 5 years ago

#6290 defect closed fixed (fixed)

TimerService.stopService should wait for a running call to finish.

Reported by: Tom Prince Owned by: Tom Prince
Priority: normal Milestone:
Component: core Keywords:
Cc: Richard Wall Branch: branches/TimerService-wait-for-finish-6290
branch-diff, diff-cov, branch-cov, buildbot
Author: tom.prince

Description

When TimerService.stopService is called while a call is in progress, it should return a deferred that fires after the call finishes.

Attachments (1)

TimerService.stopService.patch (3.3 KB) - added by Tom Prince 5 years ago.

Download all attachments as: .zip

Change History (10)

Changed 5 years ago by Tom Prince

comment:1 Changed 5 years ago by Tom Prince

Author: tomprince
Branch: branches/TimerService-wait-for-finish-6290

(In [37160]) Branching to TimerService-wait-for-finish-6290.

comment:2 Changed 5 years ago by Tom Prince

Author: tomprincetom.prince
Keywords: review added
Owner: Tom Prince deleted

buildbot run

The failing tests on lucid32-py2.7maint are due to #6314 and are unrelated.

comment:3 Changed 5 years ago by Richard Wall

Cc: Richard Wall added
Owner: set to Richard Wall
Status: newassigned

Reviewing...

comment:4 Changed 5 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Tom Prince
Status: assignednew

Looks pretty good Tom.

  • Tests all pass.
  • Merges cleanly to trunk.
  • trial --coverage shows coverage for all the changes in TimerService.

A few points:

  1. Missing news file
  2. source:branches/TimerService-wait-for-finish-6290/twisted/application/internet.py
    1. TimerService.clock:
      1. Shouldn't this be private? It's only used for testing purposes. Although I notice that LoopingCall has a public clock attribute. So maybe...
      2. Document it with @ivar, like LoopingCall does.
      3. ...in which case, fix the class docstring formatting while you're there.
      4. Should clock be listed as volatile to prevent it being (un)pickled / (un)serialised? In another service Procmon.ProcessMonitor - for example - I think exarkun asked me to explicitly delete the reactor attribute in get_state which is a similar situation...maybe.
    2. TimeService.stopService
      1. IMHO it looks strange to upcall to service.Service.stopService in a callback. What if in the future, the base method needs to do something synchronously? Would it be safer to call stopService and assign the result to a local variable which can be returned from the callback? Not sure.
      2. I think you should override the docstring in this method. Explain that it *always* returns a deferred and that it returns when the any in progress calls have finished.
  3. source:branches/TimerService-wait-for-finish-6290/twisted/application/test/test_internet.py
    1. test_failedCallLogsError
      1. Typo in docstring "returns a deferred the errbacks"
      2. Why do you need clock.advance here? test_stopServiceWaits doesn't, it relies on LoopingCall.start(now=True), so why not rely on that here too? In which case, maybe you can get rid of TimerService.clock (for now). See what you think.

-RichardW.

comment:5 Changed 5 years ago by Jean-Paul Calderone

trial --coverage shows coverage for all the changes in TimerService.

Here's a tip that should probably be documented somewhere - trial --coverage ... is way worse than python-coverage run --branch --source ... trial .... You definitely always want to use the latter. In fact, we should almost certainly get rid of the former (and maybe replace it with something based on python-coverage, or maybe not).

comment:6 Changed 5 years ago by Tom Prince

Keywords: review added
Owner: changed from Tom Prince to Richard Wall
  1. Missing news file

Done

TimerService.clock: 2.1.1. Shouldn't this be private? It's only used for testing purposes. Although I notice that LoopingCall has a public clock attribute. So maybe...

We want to promote people using twisted to write testable code. Having this public allows people to test the interaction of a TimerService with other code they write. (Note: They shouldn't be testing that TimerService works, but they may have functionality that depends on the operating of a TimerService)

2.1.2 Document it with @ivar, like LoopingCall does.

Done.

2.1.3 ...in which case, fix the class docstring formatting while you're there.

Done.

2.1.4 Should clock be listed as volatile to prevent it being (un)pickled / (un)serialised? In another service Procmon.ProcessMonitor - for example - I think exarkun asked me to explicitly delete the reactor attribute in get_state which is a similar situation...maybe.

If the global reactor is being used, this is probably None, so I don't think that is necessary.

2.2 TimeService.stopService 2.2.1 IMHO it looks strange to upcall to service.Service.stopService in a callback. What if in the future, the base method needs to do something synchronously? Would it be safer to call stopService and assign the result to a local variable which can be returned from the callback? Not sure.

I want to upcall *after* this class has stopped shutting down.

2.2.2 I think you should override the docstring in this method. Explain that it *always* returns a deferred and that it returns when the any in progress calls have finished.

Done.

3.1. test_failedCallLogsError 3.1.1 Typo in docstring "returns a deferred the errbacks"

Fixed

3.1.2 Why do you need clock.advance here? test_stopServiceWaits doesn't, it relies on LoopingCall.start(now=True), so why not rely on that here too?

Done

In which case, maybe you can get rid of TimerService.clock (for now). See what you think.

I think it is a useful attribute, so I added some explicit tests for it. One thing about using Clock instead of the global reactor is that you don't need to be careful to cleanup after the tests (cancelling callLaters for example.)

comment:7 Changed 5 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Tom Prince

Ok. That all sounds fine, but...

  1. Build results look bad
  2. I noticed a few typos in the new docstrings
    1. periodcally
    2. secons
    3. triggered when the when
  3. Epylinks - I still don't properly understand the rules for these, but...
    1. L{None} shouldn't that be C{None} cos there's nothing to actually link to.
    2. Same with L{float}, L{callable}
    3. L{start} should be L{TimerService.startService}

+1 from me if you make the build results good and if you fix those typos and links.

comment:8 Changed 5 years ago by Richard Wall

...and "it's" > "its" in the news file.

comment:9 Changed 5 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37359]) Merge TimerService-wait-for-finish-6290: Make TimerService.stopService wait for a running call to finish.

Author: tom.prince Reviewers: rwall Fixes: #6290

There was no public way to wait for the running call to finish which meant that reactor wouldn't wait for it to complete, and that test couldn't clean up.

Note: See TracTickets for help on using tickets.