Adding in timer Futures (e.g. setTimeout and setInterval)

[Pauan]

@koute @CryZe This is not ready to merge yet. It still needs an implementation of setInterval, and also documentation.

I want to get feedback on two things:

1. The code for WaitFuture. In particular, is it okay to call .drop() on a Once callback multiple times?

2. I want to implement setInterval, however there are actually many different ways to implement it.

   Let's say you use interval(1000) to create a Stream, it might behave in any of these ways:

   • Every 1 second it sends a () message, regardless of whether the consumer is ready or not. These messages will be buffered, and when the consumer is ready it might pull multiple () messages at once.

   • Same behavior as above, except it only buffers a single message, so when the consumer is ready it will never pull more than 1 message at once.

   • After the consumer pulls a message, it will wait 1 second before it sends another message. In other words, it's guaranteed that the consumer will not receive a message more than once every 1 second (no matter how fast or slowly it pulls).

   It seems to me that all three of these behaviors are useful (in different situations), so we should probably have three separate functions. What should these functions be called?

[koute]

In particular, is it okay to call .drop() on a Once callback multiple times?

Yes. After the first call to .drop() the .drop() itself is replaced with an empty function, precisely to simplify code which uses callbacks like this.

It seems to me that all three of these behaviors are useful (in different situations), so we should probably have three separate functions. What should these functions be called?

I agree. As for the names, well... I guess first we should look at other promise libraries and see how they named similar functionality? If there's an existing convention we should probably stick to it.

[Pauan]

Yes. After the first call to .drop() the .drop() itself is replaced with an empty function, precisely to simplify code which uses callbacks like this.

Great!

I guess first we should look at other promise libraries and see how they named similar functionality? If there's an existing convention we should probably stick to it.

Well, this problem isn't really encountered by Promise libraries (Promises don't support streaming). And most stream implementations in JavaScript are push-based, not pull-based, so they also don't have this problem.

In any case, this isn't about copying a JavaScript API, it's about providing a Rust Stream API, so we can call it whatever we want. It should have a Rust-y name, not a JavaScript-y name, especially because JavaScript has only 1 API (setInterval) but we're creating 3.

[Pauan]

Requesting code review on this. The non-buffered and throttled versions of interval are a lot trickier to implement than I thought, so I'm starting with interval_buffered.

Also, I think interval and throttled can (and probably should) be implemented in the futures crate:
rust-lang/futures-rs#210
rust-lang/futures-rs#409

[Diggsey]

We should think carefully about how we go about this. Javascript promises are eager unlike rust's futures and are not cancellable, so the PromiseFuture abstraction has a few quirks, and as it stands is not a perfect foundation for exposing other APIs (like setTimeout) which are cancellable.

Some more concrete goals:

• I think setTimeout should return a future which cancels the timeout if it is dropped. The same goes for setInterval. This is more inline with normal futures, where failing to "spawn" a future to an executor and dropping it instead results in everything being cleaned up.
• We should try to reduce/short-circuit the number of times we cross between javascript and wasm where possible - at the moment, when an interval fires, it completes a promise, which calls a javascript callback, which calls into rust, which calls to javascript, creates a new promise, marshalling in a callback, returns to javascript, and then that promise completes and calls into rust again, which sends a message on a channel, which marshals another callback to javascript, returns to javascript and then calls into rust again to actually receive the value from the stream. A simple setInterval operation crosses the javascript/wasm boundary 8 times! (4 times for each wake-up, and there are two wake-ups).

The first goal is most important: to achieve that, either we have PromiseFuture carry around an arbitrary "cancel" callback, or we have custom future types (eg. SetTimeoutFuture and SetIntervalStream) which carry the identifier returned by the corresponding methods, and implement Drop. Personally I think the second option is more rust-like.

The second goal is a bit trickier. First, the MPSC stream really is overkill in this case: all we need is an integer counter indicating how many outstanding times the stream should return an item. The poll method then just checks if the counter is more than zero and if so decrements it and returns Ready. The setInterval callback increments the counter and then wakes the task.

Lastly, we can optimise task wake-up by checking if we are already running asynchronously: for example, the callbacks invoked by setTimeout and setInterval are guaranteed to have an empty call-stack, and so we can optimise out the Promise.resolve(...).then(...) stuff in that specific situation. I view this as less important to do now, but worth thinking about for the future 😛 .

[Diggsey]

Every 1 second it sends a () message, regardless of whether the consumer is ready or not. These messages will be buffered, and when the consumer is ready it might pull multiple () messages at once.

Yep, this seems like the default

Same behavior as above, except it only buffers a single message, so when the consumer is ready it will never pull more than 1 message at once.

This is easier to implement if you get rid of the MPSC queue - if you use a counter as I described above, you can just replace the counter with a boolean flag.

After the consumer pulls a message, it will wait 1 second before it sends another message. In other words, it's guaranteed that the consumer will not receive a message more than once every 1 second (no matter how fast or slowly it pulls).

This probably cannot be implemented well on top of setInterval - you would need to repeatedly call setTimeout each time you yield an item.

[Pauan]

@Diggsey Did you actually look at the code? It's not using Promises at all.

[Pauan]

@Diggsey The second goal is a bit trickier. First, the MPSC stream really is overkill in this case: all we need is an integer counter indicating how many outstanding times the stream should return an item. The poll method then just checks if the counter is more than zero and if so decrements it and returns Ready. The setInterval callback increments the counter and then wakes the task.

That is a good point, I'll make that change.

Lastly, we can optimise task wake-up by checking if we are already running asynchronously: for example, the callbacks invoked by setTimeout and setInterval are guaranteed to have an empty call-stack, and so we can optimise out the Promise.resolve(...).then(...) stuff in that specific situation. I view this as less important to do now, but worth thinking about for the future 😛 .

But it already does that: because Promises run on the microtask queue, and the microtask queue is immediately run after macrotasks (such as setTimeout and setInterval), they will run synchronously.

I'm fine with changing the Executor to guarantee that it will run synchronously, but that's a change that should be made in a different PR.

[Diggsey]

Did you actually look at the code?

Yes, and enough with the passive aggressive comments. Please stop and think before you reply.

It's not using Promises at all.

https://github.com/koute/stdweb/pull/121/files#diff-a044f9ff5e011566c96f4c8b9be09879R17

You are using promises for sleep, but I had not noticed that it was only used in an example. Any particular reason for using this instead of another call to wait?

On another note, it would be helpful to split out cancellation from futures-compatibility: have a public wrapper around setInterval and setTimeout which implements cancellation (via returning a handle with a Drop implementation) and then building the futures layer on top. What do you think of that?

But it already does that: because Promises run on the microtask queue, and the microtask queue is immediately run after macrotasks (such as setTimeout and setInterval), they will run synchronously.

Yes, this is not a correctness thing, purely an optimisation to avoid extra interop between wasm and javascript which is currently a huge bottleneck, and is always going to be relatively slow, even with improvements to stdweb. Each time a micro-task is scheduled it involves an extra 4 passes through the interop layer, so we should at least start thinking about how to deal with that.

[koute]

@Pauan @Diggsey Apologies for the late reply. I very much appreciate your contibutions and great insight from the comments you make, but I'd like to ask you both one extra favor - let's get along, okay? (: We all have strong opinions, but it's usually more productive for all involved to limit the passive-aggressiveness to a minimum. Thanks!

[robert-j-webb]

Just curious - is there going to be any updates to this? Right now I really wanted to use a setInterval but there was no such code. I'm just a beginning rust developer so I don't think I could help much on the PR but I do think it's a very exciting project.

Actually I got around it by making a recursive function with set_timeout. If anyone else gets here and wants a quick and dirty solution, here it is:

fn recurse(){
    console!(log, "ddd");
    stdweb::web::set_timeout(recurse, 500)
}

[Pauan]

@robert-j-webb I'll see if I can get this done soon.

[Pauan]

Sorry for the delay, I got this all fixed, so it should be ready for review.

I don't like that people searching for setTimeout and setInterval won't find these functions, but I don't have any good ideas for fixing that.

[Pauan]

@koute Ping.

[Pauan]

@koute Ping. This is ready for review.

[koute]

It doesn't really matter but I guess let _ = sender.send(()) would be prettier than a match.

[koute]

Is there any reason you can't use Void now?

[koute]

@Pauan I'm terribly sorry about the delay! This PR slipped by me somehow.

Anyhow, it'd be nice to have some tests, although since it's not feasible yet it looks mostly good to me!

Thanks!