First pass at setTimeout with Tokio #434

ry · 2018-07-31T20:44:02Z

Depends on #429

Fixes #413

benjamingr · 2018-08-01T07:39:32Z

js/timers.ts

@@ -76,13 +83,20 @@ export function setInterval(
 // tslint:disable-next-line:no-any
 ...args: any[]
 ): number {


I realize it's not from the Tokio stuff but was also the case before - but Is there any reason you prefer returning numbers from timers and not objects like Node does? Do you think the .unref stuff is not a concern/not interesting?

(Not related to the comments about maybe returning promises - which might not be 100% compatible but is certainly how it would have been done if they did these APIs today)

No reason - actually I thought it was the spec.

@ry well, I help maintain lolex so I dug into this quite a bit at times.

browsers "implement" the Timers spec - I say "implement" because browsers vary a lot between themselves in how they handle overflow and timer behaviour for edge cases.

Node doesn't implement that spec at all and returns objects from timers to expose optimizations.

Other platforms that run JavaScript don't do this or vary.

I think timer libs already expect either Node or browser timers mostly - we get some wiggle room - the important thing (other than timer ordering) is that setTimeout returns an object that clearTimeout clears.

robbym · 2018-08-01T07:58:09Z

build_extra/rust/BUILD.gn

+ extern = [ ":libc" ]
+}
+
+tokio_root = "/Users/rld/src/deno/third_party/rust_crates/git/checkouts/tokio-377c595163f99a10/5d0d2a2/"


tokio_root = "$crates/git/checkouts/tokio-377c595163f99a10/5d0d2a2/"

robbym · 2018-08-01T17:20:07Z

src/handlers.rs

@@ -212,6 +212,10 @@ where
 }

 fn send_timer_ready(d: *const DenoC, cmd_id: u32, timer_id: u32, done: bool) {
+ // Remove the cancel from the timers table.
+ let deno = from_c(d);
+ deno.timers.remove(&timer_id);


This will remove the ability to clear a running interval.

robbym · 2018-08-01T17:23:24Z

src/handlers.rs

+ deno.rt.spawn(interval_task);
+ } else {
+ let (delay_task, cancel_delay) = set_timeout(
+ move || {


Should be here

let (delay_task, cancel_delay) = set_timeout( move || { send_timer_ready(d, cmd_id, timer_id, true); let deno = from_c(d); deno.timers.remove(&timer_id); }, delay, );

done - albeit in a hacky way. (thanks!)

Almost. remove_timer needs to be called from the delay task, not the interval task.

oops - thanks!

piscisaureus · 2018-08-07T12:59:12Z

Still working on getting winapi-0.3 and 0.3 to coexist.

ry · 2018-08-09T04:25:27Z

@piscisaureus PTAL

piscisaureus

LGTM, please look at the comments.
I must say I am a little nervous about how tightly coupled to Tokio this gets us.

piscisaureus · 2018-08-09T18:17:45Z

js/globals.ts

-// window["setInterval"] = timer.setInterval;
-// window["clearTimeout"] = timer.clearTimer;
-// window["clearInterval"] = timer.clearTimer;
+window["setTimeout"] = timers.setTimeout;


I don't really understand why this needs to be window["foo"] instead of window.foo.
If there's reason maybe add a comment? I'm a bit surprised that prettier leaves it as-is.

piscisaureus · 2018-08-09T18:31:46Z

js/timers.ts

- const { timerReadyId, timerReadyDone } = msg;
+export function onMessage(msg: fbs.TimerReady) {
+ const timerReadyId = msg.id();
+ const timerReadyDone = msg.done();
 const timer = timers.get(timerReadyId);
 if (!timer) {
 return;


I would really like to see a test that exercises this code path. Maybe something like:

const timers = []; let timeouts = 0; for (let i = 0; i < 10; i++) { timers[i] = setTimeout(onTimeout, 20); } function onTimeout() { ++timeouts; for (let i = 1; i < 10; i++) { clearTimeout(timers[i]); } } assert(timeouts === 1);

I will start adding more tests when unit_tests.ts lands ( #448 )
added an issue for it #497

piscisaureus · 2018-08-09T18:35:15Z

src/main.rs

@@ -204,4 +212,7 @@ fn main() {
 error!("{}", err);
 std::process::exit(1);
 });
+
+ // Start the Tokio event loop
+ d.rt.run().expect("err");


Ok for now, I think, fingers crossed.
But we'll have to contain this soon or we might as well rename to tokio.js already.

piscisaureus · 2018-08-09T18:38:21Z

src/handlers.rs

+ F: Fn() -> (),
+{
+ let (cancel_tx, cancel_rx) = oneshot::channel::<()>();
+ let when = Duration::from_millis(delay.into());


This is misnamed - when suggests that this is an absolute time, but the variable actually contains a time delta.
If it had said let when = Instant::now() + Duration::from_millis(delay.into()) it'd be accurate.

benjamingr reviewed Aug 1, 2018

View reviewed changes

robbym reviewed Aug 1, 2018

View reviewed changes

robbym mentioned this pull request Aug 1, 2018

Implement handle_timer_clear #437

Closed

ry force-pushed the tokio_timers branch 2 times, most recently from dc10a26 to 61d5e4a Compare August 1, 2018 15:43

ry mentioned this pull request Aug 1, 2018

Tokio deps #425

Closed

ry changed the base branch from master to source_map_with_asm August 1, 2018 15:44

ry force-pushed the source_map_with_asm branch 2 times, most recently from 5badb98 to 7556f14 Compare August 1, 2018 16:24

ry force-pushed the tokio_timers branch from 61d5e4a to ede42e6 Compare August 1, 2018 16:59

ry force-pushed the source_map_with_asm branch from 7556f14 to 38422b2 Compare August 1, 2018 17:03

ry force-pushed the tokio_timers branch from ede42e6 to f622125 Compare August 1, 2018 17:04

robbym reviewed Aug 1, 2018

View reviewed changes

ry force-pushed the tokio_timers branch 2 times, most recently from ebed252 to ccedaab Compare August 1, 2018 20:39

ry changed the base branch from source_map_with_asm to master August 1, 2018 20:40

ry force-pushed the tokio_timers branch from ccedaab to 7f3c290 Compare August 1, 2018 23:42

ry changed the title ~~[WIP] setTimeout with Tokio~~ setTimeout with Tokio Aug 2, 2018

ry changed the title ~~setTimeout with Tokio~~ First pass at setTimeout with Tokio Aug 2, 2018

ry force-pushed the tokio_timers branch 11 times, most recently from cd77dd3 to b54ae27 Compare August 2, 2018 19:18

ry force-pushed the tokio_timers branch 2 times, most recently from b8d1337 to b54ae27 Compare August 2, 2018 19:37

This was referenced Aug 3, 2018

Adds js/unit_tests.ts #448

Merged

Promise exit behaviour #449

Closed

import via http #416

Closed

Add hyper #460

Closed

ry force-pushed the tokio_timers branch from b54ae27 to fb00056 Compare August 7, 2018 00:27

This was referenced Aug 7, 2018

Suggestion: Look into porting to Rust and using Tokio #205

Closed

Add hyper and hyper-rustls dependencies #477

Closed

ry force-pushed the tokio_timers branch from fb00056 to 8551164 Compare August 9, 2018 04:05

First pass at setTimeout and friends.

1fcc498

ry force-pushed the tokio_timers branch from 8551164 to 1fcc498 Compare August 9, 2018 04:11

This was referenced Aug 9, 2018

readFileSync #452

Merged

[WIP] Implement window.fetch #478

Closed

[WIP] separate vm and backend threads #462

Closed

piscisaureus approved these changes Aug 9, 2018

View reviewed changes

Review comments

7f1a0a5

ry merged commit fb87cb3 into master Aug 9, 2018

ry deleted the tokio_timers branch September 1, 2018 16:05

haoxli mentioned this pull request Sep 5, 2018

Add unit tests for Timers #682

Closed

piscisaureus pushed a commit to piscisaureus/deno that referenced this pull request Oct 7, 2019

add encoding/hex module (denoland#434)

418cdff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass at setTimeout with Tokio #434

First pass at setTimeout with Tokio #434

ry commented Jul 31, 2018 •

edited

Loading

benjamingr Aug 1, 2018

ry Aug 1, 2018

benjamingr Aug 1, 2018

robbym Aug 1, 2018

ry Aug 1, 2018

robbym Aug 1, 2018

robbym Aug 1, 2018

ry Aug 1, 2018 •

edited

Loading

robbym Aug 1, 2018

ry Aug 1, 2018

piscisaureus commented Aug 7, 2018

ry commented Aug 9, 2018

piscisaureus left a comment

piscisaureus Aug 9, 2018

ry Aug 9, 2018

piscisaureus Aug 9, 2018

ry Aug 9, 2018

piscisaureus Aug 9, 2018 •

edited

Loading

piscisaureus Aug 9, 2018

ry Aug 9, 2018

First pass at setTimeout with Tokio #434

First pass at setTimeout with Tokio #434

Conversation

ry commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus commented Aug 7, 2018

ry commented Aug 9, 2018

piscisaureus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry commented Jul 31, 2018 •

edited

Loading

ry Aug 1, 2018 •

edited

Loading

piscisaureus Aug 9, 2018 •

edited

Loading