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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1293614 - Ensure that Wait().until() aligns as best as possible to the original interval sequence.

Review commit: https://reviewboard.mozilla.org/r/70346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70346/

Comment 2

[David Burns :automatedtester]

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 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70346/diff/1-2/

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[David Burns :automatedtester]

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

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70346/diff/2-3/

Comment 9

[David Burns :automatedtester]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1a12a0aa72d0