From dd1cd4d95237359acd822143265d7d2365bf928c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 22 Sep 2020 19:33:29 +0200 Subject: [PATCH] fix: clearing timers race condition (#7617) --- cli/ops/timers.rs | 50 ++++++++++++++++++++++++++-------- cli/rt/11_timers.js | 11 ++++++-- cli/tests/integration_tests.rs | 33 ++++++++++++++++++++++ 3 files changed, 79 insertions(+), 15 deletions(-) diff --git a/cli/ops/timers.rs b/cli/ops/timers.rs index b7ea625397708..eb65611930866 100644 --- a/cli/ops/timers.rs +++ b/cli/ops/timers.rs @@ -5,7 +5,7 @@ //! As an optimization, we want to avoid an expensive calls into rust for every //! setTimeout in JavaScript. Thus in //js/timers.ts a data structure is //! implemented that calls into Rust for only the smallest timeout. Thus we -//! only need to be able to start and cancel a single timer (or Delay, as Tokio +//! only need to be able to start, cancel and await a single timer (or Delay, as Tokio //! calls it) for an entire Isolate. This is what is implemented here. use crate::permissions::Permissions; @@ -23,14 +23,19 @@ use deno_core::ZeroCopyBuf; use serde::Deserialize; use std::cell::RefCell; use std::future::Future; +use std::pin::Pin; use std::rc::Rc; use std::time::Duration; use std::time::Instant; + pub type StartTime = Instant; +type TimerFuture = Pin>>>; + #[derive(Default)] pub struct GlobalTimer { tx: Option>, + pub future: Option, } impl GlobalTimer { @@ -40,14 +45,12 @@ impl GlobalTimer { } } - pub fn new_timeout( - &mut self, - deadline: Instant, - ) -> impl Future> { + pub fn new_timeout(&mut self, deadline: Instant) { if self.tx.is_some() { self.cancel(); } assert!(self.tx.is_none()); + self.future.take(); let (tx, rx) = oneshot::channel(); self.tx = Some(tx); @@ -56,12 +59,16 @@ impl GlobalTimer { let rx = rx .map_err(|err| panic!("Unexpected error in receiving channel {:?}", err)); - futures::future::select(delay, rx).then(|_| futures::future::ok(())) + let fut = futures::future::select(delay, rx) + .then(|_| futures::future::ok(())) + .boxed_local(); + self.future = Some(fut); } } pub fn init(rt: &mut deno_core::JsRuntime) { super::reg_json_sync(rt, "op_global_timer_stop", op_global_timer_stop); + super::reg_json_sync(rt, "op_global_timer_start", op_global_timer_start); super::reg_json_async(rt, "op_global_timer", op_global_timer); super::reg_json_sync(rt, "op_now", op_now); } @@ -81,21 +88,40 @@ struct GlobalTimerArgs { timeout: u64, } -async fn op_global_timer( - state: Rc>, +// Set up a timer that will be later awaited by JS promise. +// It's a separate op, because canceling a timeout immediately +// after setting it caused a race condition (because Tokio timeout) +// might have been registered after next event loop tick. +// +// See https://github.com/denoland/deno/issues/7599 for more +// details. +fn op_global_timer_start( + state: &mut OpState, args: Value, - _zero_copy: BufVec, + _zero_copy: &mut [ZeroCopyBuf], ) -> Result { let args: GlobalTimerArgs = serde_json::from_value(args)?; let val = args.timeout; let deadline = Instant::now() + Duration::from_millis(val); - let timer_fut = { + let global_timer = state.borrow_mut::(); + global_timer.new_timeout(deadline); + Ok(json!({})) +} + +async fn op_global_timer( + state: Rc>, + _args: Value, + _zero_copy: BufVec, +) -> Result { + let maybe_timer_fut = { let mut s = state.borrow_mut(); let global_timer = s.borrow_mut::(); - global_timer.new_timeout(deadline).boxed_local() + global_timer.future.take() }; - let _ = timer_fut.await; + if let Some(timer_fut) = maybe_timer_fut { + let _ = timer_fut.await; + } Ok(json!({})) } diff --git a/cli/rt/11_timers.js b/cli/rt/11_timers.js index ee909dbd96ffd..8f6a7e04971cc 100644 --- a/cli/rt/11_timers.js +++ b/cli/rt/11_timers.js @@ -8,8 +8,12 @@ core.jsonOpSync("op_global_timer_stop"); } - async function opStartGlobalTimer(timeout) { - await core.jsonOpAsync("op_global_timer", { timeout }); + function opStartGlobalTimer(timeout) { + return core.jsonOpSync("op_global_timer_start", { timeout }); + } + + async function opWaitGlobalTimer() { + await core.jsonOpAsync("op_global_timer"); } function opNow() { @@ -314,7 +318,8 @@ // some timeout/defer is put in place to allow promise resolution. // Ideally `clearGlobalTimeout` doesn't return until this op is resolved, but // I'm not if that's possible. - await opStartGlobalTimer(timeout); + opStartGlobalTimer(timeout); + await opWaitGlobalTimer(); pendingEvents--; // eslint-disable-next-line @typescript-eslint/no-use-before-define prepareReadyTimers(); diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index 3853e6fe4a8d6..7e0e020d62e67 100644 --- a/cli/tests/integration_tests.rs +++ b/cli/tests/integration_tests.rs @@ -1506,6 +1506,39 @@ itest!(deno_test_only { output: "deno_test_only.ts.out", }); +#[test] +fn timeout_clear() { + // https://github.com/denoland/deno/issues/7599 + + use std::time::Duration; + use std::time::Instant; + + let source_code = r#" +const handle = setTimeout(() => { + console.log("timeout finish"); +}, 10000); +clearTimeout(handle); +console.log("finish"); +"#; + + let mut p = util::deno_cmd() + .current_dir(util::tests_path()) + .arg("run") + .arg("-") + .stdin(std::process::Stdio::piped()) + .spawn() + .unwrap(); + let stdin = p.stdin.as_mut().unwrap(); + stdin.write_all(source_code.as_bytes()).unwrap(); + let start = Instant::now(); + let status = p.wait().unwrap(); + let end = Instant::now(); + assert!(status.success()); + // check that program did not run for 10 seconds + // for timeout to clear + assert!(end - start < Duration::new(10, 0)); +} + #[test] fn workers() { let _g = util::http_server();