Bug 1538416: Check time only once per iteration of wait loop. r=bytesized
authorAdam Gashlin <agashlin@mozilla.com>
Fri, 26 Apr 2019 22:53:48 +0000
changeset 530421 75a68e719820b37c5adf3d80cb92a810efe4d535
parent 530420 71f09f4113cde86e02d6a5f9826d8590e2c2603a
child 530422 5e69a320a5a1d56962c8928c650da450f9dbbc31
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbytesized
bugs1538416
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1538416: Check time only once per iteration of wait loop. r=bytesized Differential Revision: https://phabricator.services.mozilla.com/D29083
toolkit/components/bitsdownload/bits_client/src/in_process/mod.rs
toolkit/components/bitsdownload/bits_client/src/in_process/tests.rs
--- a/toolkit/components/bitsdownload/bits_client/src/in_process/mod.rs
+++ b/toolkit/components/bitsdownload/bits_client/src/in_process/mod.rs
@@ -364,56 +364,60 @@ impl InProcessMonitor {
 
     pub fn get_status(
         &mut self,
         timeout_millis: u32,
     ) -> Result<Result<JobStatus, HResultMessage>, Error> {
         let timeout = Duration::from_millis(u64::from(timeout_millis));
 
         let started = Instant::now();
+        let timeout_end = started + timeout;
 
         {
             let mut s = self.vars.1.lock().unwrap();
             loop {
+                let wait_start = Instant::now();
+
                 if s.shutdown {
                     // Disconnected, immediately return error.
                     // Note: Shutdown takes priority over simultaneous notification.
                     return Err(Error::NotConnected);
                 }
 
-                if started.elapsed() > timeout {
+                if wait_start >= timeout_end {
                     // Timed out, immediately return timeout error.
-                    // This should not normally happen with the in-process monitor, but e.g. the
+                    // This should not normally happen with the in-process monitor, but the
                     // monitor interval could be longer than the timeout.
                     s.shutdown = true;
                     return Err(Error::Timeout);
                 }
 
                 // Get the interval every pass through the loop, in case it has changed.
                 let interval = Duration::from_millis(u64::from(s.interval_millis));
 
-                let wait_until = self.last_status_time.map(|last_status_time| {
-                    cmp::min(last_status_time + interval, started + timeout)
-                });
+                let wait_until = self
+                    .last_status_time
+                    .map(|last_status_time| cmp::min(last_status_time + interval, timeout_end));
 
                 if s.notified {
                     // Notified, exit loop to get status.
                     s.notified = false;
                     break;
                 }
 
                 if wait_until.is_none() {
                     // First status report, no waiting, exit loop to get status.
                     break;
                 }
 
                 let wait_until = wait_until.unwrap();
-                let wait_start = Instant::now();
 
                 if wait_until <= wait_start {
+                    // No time left to wait. This can't be due to timeout because
+                    // `wait_until <= wait_start < timeout_end`.
                     // Status report due, exit loop to get status.
                     break;
                 }
 
                 // Wait.
                 // Do not attempt to recover from poisoned Mutex.
                 s = self
                     .vars
--- a/toolkit/components/bitsdownload/bits_client/src/in_process/tests.rs
+++ b/toolkit/components/bitsdownload/bits_client/src/in_process/tests.rs
@@ -395,19 +395,18 @@ test! {
 
         let (_, mut monitor) =
             client.start_job(server.format_url(name), name.into(), BitsProxyUsage::Preconfig, interval).unwrap();
 
         // Start the timer now, the initial job creation may be delayed by BITS service startup.
         let start = Instant::now();
 
         // First report, immediate
-        let report_1 = monitor.get_status(timeout);
+        let report_1 = monitor.get_status(timeout).expect("should initially be ok").unwrap();
         let elapsed_to_report_1 = start.elapsed();
-        report_1.as_ref().expect("should initially be ok").as_ref().unwrap();
 
         // Transferred notification should come when the job completes in ~250 ms, otherwise we
         // will be stuck until timeout.
         let report_2 = monitor.get_status(timeout).expect("should get status update").unwrap();
         let elapsed_to_report_2 = start.elapsed();
         assert!(elapsed_to_report_2 < Duration::from_millis(9_000));
         assert_eq!(report_2.state, BitsJobState::Transferred);