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

Conversation

4 participants
@ry
Collaborator

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 {

This comment has been minimized.

@benjamingr

benjamingr Aug 1, 2018

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)

@benjamingr

benjamingr Aug 1, 2018

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)

This comment has been minimized.

@ry

ry Aug 1, 2018

Collaborator

No reason - actually I thought it was the spec.

@ry

ry Aug 1, 2018

Collaborator

No reason - actually I thought it was the spec.

This comment has been minimized.

@benjamingr

benjamingr Aug 1, 2018

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

@benjamingr

benjamingr Aug 1, 2018

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

Show outdated Hide outdated build_extra/rust/BUILD.gn Outdated

@robbym robbym referenced this pull request Aug 1, 2018

Closed

Implement handle_timer_clear #437

@ry ry referenced this pull request Aug 1, 2018

Closed

Tokio deps #425

@ry ry changed the base branch from master to source_map_with_asm Aug 1, 2018

Show outdated Hide outdated src/handlers.rs Outdated
deno.rt.spawn(interval_task);
} else {
let (delay_task, cancel_delay) = set_timeout(
move || {

This comment has been minimized.

@robbym

robbym Aug 1, 2018

Contributor

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

robbym Aug 1, 2018

Contributor

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

This comment has been minimized.

@ry

ry Aug 1, 2018

Collaborator

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

@ry

ry Aug 1, 2018

Collaborator

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

This comment has been minimized.

@robbym

robbym Aug 1, 2018

Contributor

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

@robbym

robbym Aug 1, 2018

Contributor

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

This comment has been minimized.

@ry

ry Aug 1, 2018

Collaborator

oops - thanks!

@ry

ry Aug 1, 2018

Collaborator

oops - thanks!

@ry ry changed the base branch from source_map_with_asm to master Aug 1, 2018

@ry ry changed the title from [WIP] setTimeout with Tokio to setTimeout with Tokio Aug 2, 2018

@ry ry changed the title from setTimeout with Tokio to First pass at setTimeout with Tokio Aug 2, 2018

@piscisaureus

This comment has been minimized.

Show comment
Hide comment
@piscisaureus

piscisaureus Aug 7, 2018

Collaborator

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

Collaborator

piscisaureus commented Aug 7, 2018

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

@ry

This comment has been minimized.

Show comment
Hide comment
@ry

ry Aug 9, 2018

Collaborator
Collaborator

ry commented Aug 9, 2018

@piscisaureus

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

Show outdated Hide outdated js/globals.ts Outdated
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;

This comment has been minimized.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

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

piscisaureus Aug 9, 2018

Collaborator

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

This comment has been minimized.

@ry

ry Aug 9, 2018

Collaborator

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

@ry

ry Aug 9, 2018

Collaborator

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

This comment has been minimized.

@piscisaureus

piscisaureus Aug 9, 2018

Collaborator

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

piscisaureus Aug 9, 2018

Collaborator

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.

Show outdated Hide outdated src/handlers.rs Outdated

@ry ry merged commit fb87cb3 into master Aug 9, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@ry ry deleted the tokio_timers branch Sep 1, 2018

@haoxli haoxli referenced this pull request Sep 5, 2018

Closed

Add unit tests for Timers #682

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