Ticket #1875 (closed enhancement: fixed)

Opened 4 years ago

Last modified 3 years ago

twisted.internet.task.deferLater()

Reported by: mithrandi Owned by: exarkun
Priority: highest Milestone:
Component: core Keywords:
Cc: jknight, exarkun, radix, jerub, mithrandi, strank, therve, yaubi Branch: branches/task-deferlater-1875-2
Author: exarkun Launchpad Bug:

Description

See attached patch.

Attachments

deferlater.diff Download (3.2 KB) - added by mithrandi 4 years ago.

Change History

Changed 4 years ago by mithrandi

  • summary changed from {{{twisted.internet.task.deferLater()}}} to twisted.internet.task.deferLater()

Changed 4 years ago by mithrandi

  • owner changed from exarkun to jerub

Attaching updated patch.

Changed 4 years ago by mithrandi

Changed 4 years ago by jerub

  • keywords review removed
  • owner changed from jerub to mithrandi

Okay, I've had a good look at the patch, and I'm satisfied that it does what it says it does. I have two concerns.

One: I want to run this past glyph or exarkun, just to make sure that this code is worth going into Twisted. It looks very similar to reactor.callLater, I feel there should be one obvious way of doing things. Should we deprecate callLater and point to deferLater? Or should we put guidelines in here for where deferLater should be used instead of callLater.

Two: def _callLater(self, *a, **kw): should probably be written: def __callLater(self, delay, func):. The function is private, and is only invoked like this: self._callLater(delay, lambda: d.callback(None)).

Please update the patch with the above modifications, and I'll take responsibility for merging the updated patch if/when I get approval/guidance.

Changed 4 years ago by jerub

The double underscore in front of __callLater in my comment is a typo. It should be a single underscore.

Changed 4 years ago by jerub

Branch created: task-deferlater-1875

I've altered the twisted.web2.test.test_http tests to import deferLater, and use that instead. This has managed to reduce the amount of code required, which is highly satisfactory.

More tests for deferLater would be good - we don't currently have a test for if the function raises an error and fires an errback.

Changed 4 years ago by jknight

I'm confused, why does it do an addCallback for you? That seems unnecessary and redundant. I'd suggest using the function as it was in test_http. Simple, clean, no extraneous gunk.

def deferLater(secs):

d = defer.Deferred() reactor.callLater(secs, d.callback, None) return d

Changed 4 years ago by radix

What James said.

Changed 4 years ago by exarkun

Why is

deferLater(n).addCallback(lambda ign: f(x))

preferable to

deferLater(n, f, x)

?

Changed 4 years ago by exarkun

  • cc jknight, exarkun, radix, jerub, mithrandi added

How about a compromise? The function can default to lambda: None, then both of the above will work.

Changed 4 years ago by jknight

It's preferable for simplicity of API. The deferred is the primary concept here, and it has a perfectly good API already which there's no good reason to hide.

Changed 4 years ago by exarkun

It's not perfectly good. The feature it is missing in this case is invoking a callback without passing any arguments. This is what is addressed by allowing a function and arguments to be passed directly to deferLater.

Changed 4 years ago by jknight

Same thing is true for all deferred callback functions. This shouldn't be special.

Changed 4 years ago by exarkun

But there's never any possibility of a useful value being passed in here. The fact that the call is being made is the entirety of the information being delivered.

Changed 3 years ago by strank

  • cc strank added

On the mailing list today, I asked about a way to get the result of a task scheduled with callLater. deferLater nearly solves that problem.

As for the API, it would be easy to allow both variants: simply change the callable argument to default to None::

    def __call__(self, delay, callable_=None, *args, **kw):
        d = defer.Deferred() 
        if callable_ is not None:
            d.addCallback(lambda dummy: callable(*args, **kw)) 
        self._callLater(delay, lambda: d.callback(None)) 
        return d 

If there has to be only one way to call it, I think that deferLater(n, f, x) is much better, for the same reasons as exarkun mentioned.

What I am missing is a way to get to the DelayedCall instance returned by callLater. I would simply add a task attribute to the returned Deferred::

        ...
        d.task = self._callLater(delay, lambda: d.callback(None))
        return d 

but I am not sure if this is nice enough.

Changed 3 years ago by therve

  • cc therve added

Can we reach an agreement on this? We can probably have the both interfaces, one basic and another a little more high-level?

Changed 3 years ago by therve

ping!

Changed 3 years ago by yaubi

  • cc yaubi added

Changed 3 years ago by mithrandi

  • owner mithrandi deleted

Changed 3 years ago by exarkun

Hmm, I'm thinking about the issue raised by strank now. What if you want to cancel the deferLater's DelayedCall?

Changed 3 years ago by jknight

Hmm, I think I heard something about adding cancelable Deferreds at some point. Of course, that still doesn't let you reschedule/etc, but that seems okay to me: the more verbose API is still available for such special uses.

Changed 3 years ago by jknight

If it helps, I'll just state I don't care anymore which of the API alternatives is added, they're really both fine. Just, please don't add both or a hybrid, in some attempt to please me.

Changed 3 years ago by exarkun

  • branch set to branches/task-deferlater-1875-2
  • author set to exarkun

(In [22507]) Branching to 'task-deferlater-1875-2'

Changed 3 years ago by exarkun

  • keywords review added

Okay

Changed 3 years ago by exarkun

  • keywords review removed

Missed a thing...

Changed 3 years ago by exarkun

  • keywords review added

Missed thing was r22515.

Changed 3 years ago by therve

  • owner set to exarkun
  • keywords review removed
  • deferLater doesn't document its clock argument
  • in writing-standard.xhtml, the example is wrong because it doesn't pass the reactor as argument

Changed 3 years ago by exarkun

  • owner exarkun deleted
  • keywords review added

Doh, I messed that up pretty good. :)

Fixed those things.

Changed 3 years ago by therve

  • owner set to exarkun
  • keywords review removed

Please merge.

Changed 3 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(In [22548]) Merge task-deferlater-1875-2

Author: mithrandi, jerub, exarkun Reviewer: therve Fixes #1875

Add twisted.internet.task.deferLater, an API for scheduling a future call and getting a Deferred which is called back with its result.

Note: See TracTickets for help on using tickets.