Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Future enhancements for timeouts and cancellation #106

Closed
bjouhier opened this issue Jun 12, 2012 · 15 comments
Closed

Future enhancements for timeouts and cancellation #106

bjouhier opened this issue Jun 12, 2012 · 15 comments
Assignees

Comments

@bjouhier
Copy link
Member

Futures should support timeouts and cancellation.

I have prototyped this in the futures branch. It needs more unit tests and debugging but the basics are working.

The proposed API consists in allowing an optional timeout argument when waiting on a future. This optional argument is a small object with the following fields:

  • millis: number of milliseconds that the future will wait (>= 0)
  • value: the value returned on timeout. If throws is true and if value != null then value will be thrown.
  • throws: if true, the future will throw on timeout, otherwise it will return value.
  • probe: if true, the future is only probed and continues to run if a timeout occurs. Otherwise it is cancelled upon timeout.

Note: node APIs aren't cancellable so cancellation means that the callbacks of the pending operations initiated by the future will be inhibited. Integration with true cancellable APIs is for future study.

Here is an example:

function hello(_) {
    setTimeout(1000, _);
    return "hello world";
}

var future, result;

future = hello();
result = future(_);
// result is 'hello world' after 1000 millis

future = hello();
result = future(_, { millis: 500, value: 'bye' });
// result is 'bye' after 500 millis

future = hello();
result = future(_, { millis: 500, throws: true });
// new Error('timeout') is thrown after 500 millis

future = hello();
result = future(_, { millis: 500, value: 'bye', throws: true });
// 'bye' is thrown after 500 millis

future = hello();
result = future(_, { millis: 300, probe: true });
// result is undefined after 300 millis
// continue with the same future
result = future(_, { millis: 1000, probe: true });
// result is 'hello world' after the 700 remaining millis

As a shortcut, the millis, value, throws and probe values may be passed directly as separate arguments (in this order). Then:

future = hello();
result = future(_, 500, 'bye');
// result is 'bye' after 500 millis

future = hello();
result = future(_, 500);
// result is undefined after 500 millis

I would also like to introduce a flows.race(futures, count) function in the flows module. This function would return the count fastest results of an array of futures, as an array of results (count is optional and defaults to 1).

Together with flows.collect(futures) and the proposed timeout feature, this should provide a reasonable basis for workflows.

@aseemk
Copy link
Contributor

aseemk commented Jun 13, 2012

Cool stuff, Bruno! May I suggest/request some API tweaks?

  • Timeouts are generally errors in most cases that I've experienced, so having the default be to throw (if a timeout was specified) might be nicer.
  • It might be nicer to call the option timeout instead of millis, so it's clear when reading that the purpose of this number is a timeout threshold. This is consistent w/ a lot of other APIs, e.g. jQuery.ajax and node-request.
  • Particularly if throwing becomes the default, throws and value could probably be combined, e.g. throw: "timeout error". That also feels clearer that you're specifying what you want thrown in that case.
  • To go with the last point, maybe the option to return something rather than throw could be return: "timeout error".
  • (Minor, but In both last points, I thought the singular verbs throw and return read better than the plurals throws and returns. You're specifying options; you're specifying what you want the API to do.)

Cool stuff again! Looking forward to it.

@bjouhier
Copy link
Member Author

Thanks for the feedback.

Regarding the default behavior, I was more focused on workflow scenarios where you will usually want to distinguish three outcomes: 1) call returns normally, 2) call times out, 3) call fails for other reasons and I thought that returning a special value like undefined upon timeout would be the most practical (especially in the common case where the call returns a non null value).

But on the other hand, there are also lots of scenarios where you don't do anything special with timeouts and you'd want them to be treated like any other exception. So throwing by default makes sense too. It also makes the API safer and aligned on lots of other APIs that throw on timeout.

{ timeout: 1000 } is more explicit than { value: 1000 }

I also like { throw: "timeout error" } and { return: null }. Nice way to specify the behavior and the value in one shot. I would set it up so that the message specified with throw is thrown as new Error(message). I would also allow the throw and return values to be functions. This way you'd be able to instantiate your own TimeoutError class without paying the price of an error allocation (this is costly) when the call does not time out.

I will then limit the short form (passing the timeout millis directly as a number argument) to 1 argument: future(_, 1000) would then throw on timeout and you'd have to use the object to specify any other behavior.

I chose throws rather than throw because my beautifier does not understand ES5 rules (keywords being valid property names) and it messes up my code. But I should rather fix my beautifier.

So thanks. This is very useful feedback.

@aseemk
Copy link
Contributor

aseemk commented Jun 13, 2012

Great! Happy to help as always.

I think wrapping string values of throw in Error objects is a really good idea. Functions is a good idea too, but for cases where perf isn't a concern, it'd also be nice to just pass instantiated Error objects directly.

@bjouhier
Copy link
Member Author

I made the changes and cleaned up the implementation. It now passes slightly more complex unit tests in the 3 modes (callbacks, fibers and generators).

I could easily add a pause/resume API that would let you put a future on hold and resume it later. Just a few lines of code to add (and more unit tests).

@aseemk
Copy link
Contributor

aseemk commented Jun 15, 2012

Mind explaining what pausing and resuming would do / what it means, given that the actual operation wouldn't be paused/resumed?

Without knowing the answer, I'll toss out that it may be good enough just to have cancel — parity with native timers/intervals.

(Edit: even canceling doesn't seem super necessary to me, given that again, unlike timers, the actual async operations can't be canceled. Just guarding for timeouts is helpful though!)

@bjouhier
Copy link
Member Author

The scenario here is macro operations that execute several low level I/O operations underneath. If you cancel/pause a macro operation, the low level I/O operations that are currently in progress won't be interrupted but subsequent I/O operations (their callbacks) will be cancelled/queued. When you resume the macro operation the queued operations will be resumed.

The simplest case is:

function foo(_) {
  setTimeout(_, 1000);
  setTimeout(_, 1000);
}

var fut = foo();

if you pause fut after 500ms, the first setTimeout will complete but the second one will be queued and only executed when you resume fut. If you cancel, the second setTimeout won't be executed.

@aseemk
Copy link
Contributor

aseemk commented Jun 15, 2012

Ah, thanks for explaining! I haven't personally come across a need for this, but if you or others have, makes sense. If not, might be worth holding off until a need arises. =)

@bjouhier
Copy link
Member Author

I don't really have a need for it right now but I'd rather do it while I have my head at it. It should not introduce any runtime overhead and it falls logically into the current code. It might be a nice feature for workflows (suspending a task and resuming it later).

My current need was more for timeout and probe.

@ghost ghost assigned bjouhier Jun 22, 2012
@bjouhier
Copy link
Member Author

1a0734d

Moved this experiment to an advanced_futures branch because I should be able to do something more clever with generators.

Master and v0.4 are now identical and do not contain this experimental API.

@jwalton
Copy link

jwalton commented Nov 7, 2013

The advanced_futures branch appears to be identical to master right now? Anyways, here is some quick coffee code for anyone who needs a timeout in a hurry:

# Call a future with a timeout.
# Throws an error if the timeout happens before the future completes.
withTimeout = (future, timeoutInMs, _) ->
    go = (done) ->
        finished = false

        doTimeout = () ->
            if !finished
                finished = true
                done new Error("Timeout")
        setTimeout doTimeout, timeoutInMs

        future (err, args...) ->
            if !finished
                finished = true
                done err, args...
    go _

You can call this with:

hello = (_) ->
    setTimeout _, 1000
    return "hello world"

future = hello()
try
    console.log withTimeout(future, 100, _)
catch err
    console.log err.message or err.stack

@jwalton
Copy link

jwalton commented Nov 7, 2013

I should point out, you can also call this with:

future = hello()
futureWithTimeout = withTimeout future, 100
console.log futureWithTimeout _

Or, in other words, you can turn a regular future into a future with a timeout, and then multiple callers can all wait on futureWithTimeout, and they'll all timeout at the same time.

@bjouhier
Copy link
Member Author

There is actually a callWithTimeout API in the flows package :-) It got a bit burried in a commit that I did 4 months ago (4343856) and I forgot to advertize it. Similar to your implementation but I'm also clearing the timeout so that the closure gets GCed if the callback comes before the timeout.

Regarding the "advanced_futures" branch being an ancestor of the current master, this is because I did not manage it properly as a feature branch. Instead, I kept developing new features in this branch for a while. Then it was easier to merge it into master and remove "advanced" code than to cherry-pick all the other commits done on this branch.

I wanted to be a bit more ambitious with this branch and give the ability to cancel a future, or suspend/resume it. But this has a bit of overhead and it makes the whole thing more complex. So I gave up for now. I can always resurrect it later from the last commit before this branch got merged into master.

@spollack
Copy link
Contributor

just looked at callWithTimeout, makes sense. I have something similar i had built locally too.

It would be slick though if the timeout somehow flowed down to any nested async calls that the top level function is making. For example, if the top level function does steps A, B, C in sequence, and when the timeout fires it is in the middle of step A, it would be great if B and C don't get run. Right now, all the remaining nested work still runs. But i realize this is a bit tricky and probably has a perf impact. Perhaps this is equivalent to the idea of cancelling a future that you were persuing a while back Bruno.

@bjouhier bjouhier modified the milestone: 0.5.0 Sep 27, 2014
@aseemk
Copy link
Contributor

aseemk commented Oct 8, 2014

Wow @bjouhier, just discovered flows.callWithTimeout. Beautiful and super useful!

You should document it!

@bjouhier
Copy link
Member Author

This experiment is in an old branch. I'm dropping this: it is intellectually challenging but it adds a lot of complexity and nobody really asked for it;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants