Remove flake inducing timeouts

[dnfield]

Fixes flutter/flutter#64087
Fixes flutter/flutter#66265

Per style guide (https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#never-check-if-a-port-is-available-before-using-it-never-add-timeouts-and-other-race-conditions), we shouldn't have timeouts.

This changes all short timeouts that were intended to cause a timeout to zero.

It changes all long timeouts that were intended to be good enough to fml::TimeDelta::Max(). On a sufficiently slow machine, 1s was not enough time to get the first frame drawn, for example. The test will still get killed by the harness if it exceeds the total time budget (currently at 90s).

It also deletes one test entirely. This test is measuring how long the test itself takes to execute, and asserting that those times are greater or less than a certain number depending on the runtime mode. This is inherently flaky - it should not have been allowed in to begin with, and if we really need coverage of this it should be covered in some way that fakes out the clock. I assume, however, that this is covered by integration tests downstream.

[dnfield]

Oh, one other problem with these timeouts - if they trigger in an unexpected way, the shell gets collected on the wrong thread, and the test process crashes with no backtrace.

[gaaclarke]

Seems like you could get what you want by just removing this assert right? That would eliminate all timeout except the one that's for the whole test.

[gaaclarke]

I'm just realizing that's the whole point of this test. It doesn't really test much without it =T

[gaaclarke]

Deleting this LGTM, I'd double check with the author of it just to see if they can think of another way to assert what they want.

[dnfield]

Author is @liyuqian

[dnfield]

Yeah the more I think about this, this either needs to be a benchmark and run as one, or it needs to mock out the clock to assert how many ticks can pass before events are emitted.

[gaaclarke]

LGTM modulo pinging that test's author.

[blasten]

is the assert below right? It seems to be expecting a timeout indeed.

[dnfield]

Yes. We're saying we want to wait zero ms now instead of waiting 10. Theoretically something could actually produce a frame in 10ms. Even this isn't bullet proof but it's better.

[blasten]

are we sure that the intention is to time out as soon as possible, or to wait enough time? I wonder why 10 was chosen.

[dnfield]

It's very likely because that was short enough on the developer's machine to trigger the timeout. The problem is, on a fast enough machine or with enough improvements to the code under test, 10ms might not be long enough.

[dnfield]

IOW, this test is currently trying to wait 10ms of monotonic time and seeing if it can or can't do work in that time. But that's susceptible to flakes.

[dnfield]

@gaaclarke - mind taking another look at this?

I had to change this because otherwise we're susceptible to overflows when adding timeout to std::chrono::steady_clock::now().

wait_for does that addition behind the scenes and I guess treats the overflow as UB. I'm using wait_until to avoid the steady clock advancing while we do other calcuations and overflowing even though we checked.

This is covered by tests now that the timeouts use max/zero values.

[dnfield]

FWIW, this is an actual bug for us today - if someone were to pass a very large value into here, they'd get a timeout response due to overflow.

[zanderso]

It sounds like the API docs in the header file here https://github.com/flutter/engine/blob/master/shell/common/shell.h#L301 could use some more details or a warning about this.

[dnfield]

Adding a doc.

[gaaclarke]

Good catch. I think an easier fix would just be to say auto duration = min(max_duration, desired_duration); and use wait_for. Since that time is so long, no one will ever be able to tell the difference in their lifetime. That removes a branch and is much easier to verify it is correct assuming what was there before was correct.

[dnfield]

Done. Though technically isn't std::min a branch? :) Anyway I agree the code is more readable/maintainable this way.

[dnfield]

Oh. We can't do that. wait_for is still going to add to stead_clock::now(), which has incremented since checked for overflow, and now might overflow again.

[dnfield]

We could do this:

  auto now = std::chrono::steady_clock::now();
  auto max_duration = std::chrono::steady_clock::time_point::max() - now;
  auto desired_duration = std::chrono::milliseconds(timeout.ToMilliseconds());

  std::unique_lock<std::mutex> lock(waiting_for_first_frame_mutex_);
  bool success = waiting_for_first_frame_condition_.wait_until(
      lock, now + std::min(desired_duration, max_duration),
      [&waiting_for_first_frame = waiting_for_first_frame_] {
        return !waiting_for_first_frame.load();
      });

[gaaclarke]

That sounds good to me too, as long as there isn't that branch I think it's more straightforward.

[zanderso]

It sounds like the API docs in the header file here https://github.com/flutter/engine/blob/master/shell/common/shell.h#L301 could use some more details or a warning about this.

[zanderso]

Should this be fml::TimeDelta::Max() ?

[dnfield]

That would make us wait forever and time out the test. I'm not sure how much value this test adds over the other test(s) that make sure we timeout if we don't wait long enough.

The problem is, even if we keep it at 1s, there's no good way of knowing whether we timed out because it fails to produce a frame by design, or we timed out because we were running on hardware slow enough that 1s isn't long enough to produce a frame.

[dnfield]

@iskakaushik is in the blame for this, might have ideas.

[zanderso]

Ah, sorry, I missed the ASSERT below this line saying that the wait is expected to timeout. So, is this testing that PumpOneFrame doesn't do anything for a zero size frame, and that's why the wait will time out? Is there another way for PumpOneFrame to communicate that it isn't going to do anything?

[dnfield]

My understanding is that this test wants to assert that WaitForFirstFrame will time out if you pump a zero sized frame.

I think a better place to test this would be e.g. in the animator unittests, making sure that OnAnimatorDidDraw isn't called under this condition.

[dnfield]

(that's where the shell would flip the wait condition)