Wait().until() is not trustworthy for its intervals

RESOLVED FIXED in Firefox 51

Status

Testing
Marionette
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Version 3
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
As seen on bug 1272589 the interval Wait().until() uses to check for the condition is not trustworthy. It actually does not check in fixed intervals but depends on the timing of the condition callback to actually return the value, which in some cases mean a huge delay for slow running applications.

Here an example of a call to until() with an interval of 1s and a timeout of 10s, whereby the callback takes about .5s to return:

> 1470739996.79
> 1470739998.29
> 1470739999.79
> 1470740001.29
> 1470740002.79
> 1470740004.29
> 1470740005.8

The result as it can be seen is that until() actually checks the condition callback each 1.5s, which is the sum of interval + callback_duration.

This is not acceptable and we should fix this behavior to at least ensure to sleep only the necessary time or call the condition right away in case the callback duration is longer than the specified interval.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8779358 - Flags: review?(ato)
(Assignee)

Updated

2 years ago
Attachment #8779358 - Flags: review?(ato) → review?(dburns)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8779358 [details]
Bug 1293614 - Ensure that Wait().until() aligns as best as possible to the original interval sequence.

https://reviewboard.mozilla.org/r/70346/#review68140

::: testing/marionette/harness/marionette/tests/unit/test_wait.py:54
(Diff revision 1)
>          if self.waited == wait:
>              if e is None:
>                  e = Exception
>              raise e
>  
> +    def delay(self, secs):

What is using this method?

::: testing/marionette/harness/marionette/tests/unit/test_wait.py:304
(Diff revision 1)
> +            return mn.false()
> +
> +        with self.assertRaisesRegexp(errors.TimeoutException,
> +                                     "Timed out after 10.0 seconds"):
> +            self.wt.until(callback)
> +        # With a elayed conditional return which takes twice that long than the interval,

delayed?
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
Comment on attachment 8779358 [details]
Bug 1293614 - Ensure that Wait().until() aligns as best as possible to the original interval sequence.

https://reviewboard.mozilla.org/r/70346/#review68140

> What is using this method?

See some lines below in the newly added tests. This methods delays the return of the contitional function, which is needed to re re-calculate the left-over time vs. interval.

> delayed?

I will fix it in my next push.
(Assignee)

Updated

2 years ago
Blocks: 1294353
(Assignee)

Comment 5

2 years ago
David, we could do even more here if we would sum up the delays we had in those cases when the conditional returned later than the specified interval. With that we could make the next interval shorter by default, and could try to keep up with the original interval sequence. As result we would execute the conditional even more.

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8779358 [details]
Bug 1293614 - Ensure that Wait().until() aligns as best as possible to the original interval sequence.

https://reviewboard.mozilla.org/r/70346/#review68140

> See some lines below in the newly added tests. This methods delays the return of the contitional function, which is needed to re re-calculate the left-over time vs. interval.

I can't seem to see it, can you tell me which line exactly
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8779358 [details]
Bug 1293614 - Ensure that Wait().until() aligns as best as possible to the original interval sequence.

https://reviewboard.mozilla.org/r/70346/#review68140

> I can't seem to see it, can you tell me which line exactly

Ok, sorry. That was clearly my fault. With a last minute change for the second test I had to switch from a real time.sleep() to a self.clock.sleep(). Otherwise it would not be testable given this custom clock and it's usage in Wait().until(). I will remove this method in the next commit.
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8779358 [details]
Bug 1293614 - Ensure that Wait().until() aligns as best as possible to the original interval sequence.

https://reviewboard.mozilla.org/r/70346/#review68482
Attachment #8779358 - Flags: review?(dburns) → review+

Comment 10

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a12a0aa72d0
Ensure that Wait().until() aligns as best as possible to the original interval sequence. r=automatedtester

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1a12a0aa72d0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.