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

First pass at setTimeout with Tokio #434

Merged
merged 2 commits into from
Aug 9, 2018
Merged

First pass at setTimeout with Tokio #434

merged 2 commits into from
Aug 9, 2018

Conversation

ry
Copy link
Member

@ry ry commented Jul 31, 2018

Depends on #429

Fixes #413

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason - actually I thought it was the spec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

extern = [ ":libc" ]
}

tokio_root = "/Users/rld/src/deno/third_party/rust_crates/git/checkouts/tokio-377c595163f99a10/5d0d2a2/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops :)

@ry ry force-pushed the tokio_timers branch 2 times, most recently from dc10a26 to 61d5e4a Compare August 1, 2018 15:43
@ry ry mentioned this pull request Aug 1, 2018
@ry ry changed the base branch from master to source_map_with_asm August 1, 2018 15:44
@ry ry force-pushed the source_map_with_asm branch 2 times, most recently from 5badb98 to 7556f14 Compare August 1, 2018 16:24
src/handlers.rs Outdated
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will remove the ability to clear a running interval.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
    );

Copy link
Member Author

@ry ry Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops - thanks!

@ry ry force-pushed the tokio_timers branch 2 times, most recently from ebed252 to ccedaab Compare August 1, 2018 20:39
@ry ry changed the base branch from source_map_with_asm to master August 1, 2018 20:40
@ry ry changed the title [WIP] setTimeout with Tokio setTimeout with Tokio Aug 2, 2018
@ry ry changed the title setTimeout with Tokio First pass at setTimeout with Tokio Aug 2, 2018
@ry ry force-pushed the tokio_timers branch 11 times, most recently from cd77dd3 to b54ae27 Compare August 2, 2018 19:18
@piscisaureus
Copy link
Member

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

@ry
Copy link
Member Author

ry commented Aug 9, 2018

@piscisaureus PTAL

Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

js/globals.ts Outdated
// window["setInterval"] = timer.setInterval;
// window["clearTimeout"] = timer.clearTimer;
// window["clearInterval"] = timer.clearTimer;
window["setTimeout"] = timers.setTimeout;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -204,4 +212,7 @@ fn main() {
error!("{}", err);
std::process::exit(1);
});

// Start the Tokio event loop
d.rt.run().expect("err");
Copy link
Member

@piscisaureus piscisaureus Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/handlers.rs Outdated
F: Fn() -> (),
{
let (cancel_tx, cancel_rx) = oneshot::channel::<()>();
let when = Duration::from_millis(delay.into());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@ry ry merged commit fb87cb3 into master Aug 9, 2018
@ry ry deleted the tokio_timers branch September 1, 2018 16:05
@haoxli haoxli mentioned this pull request Sep 5, 2018
piscisaureus pushed a commit to piscisaureus/deno that referenced this pull request Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants