WiP/RFC time: remove return-value from TestTimeSystem::waitFor().

[jmarantz]

Description: The return value (timeout vs no-timeout) from condition-variables is not reliable, so remove it entirely. Per absl::CondVar doc
https://github.com/abseil/abseil-cpp/blob/d43b7997c0ca0f3312a51d1057fb73cfe934703b/absl/synchronization/mutex.h#L795
a timeout/signal race can be hard to reason about, and I think it's pretty error-prone to look at the value. So this PR proposes to change TestTimeSystem::waitFor to have it be a void function: the caller must check the condition.

A better pattern would be to include the condition function, ie Mutex::Await(Condition).

This PR is intended to seed the discussion of which way to go for the test infrastructure. Changing to Mutex::Await(Condition) as a pattern exposed to the integration test infrastructure might be the best way to go, but that's a slightly deeper change.

One of the controversial things in this PR is that there's a param to some Fake[Up]Stream methods ignore_spurious_wakeups and this PR makes that param be ignored.
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

[jmarantz]

I meant this to be a Draft PR; sorry about that.

Anyway I'd love to get opinions on this direction from @alyssawilk and @mattklein123 .

[mattklein123]

One of the controversial things in this PR is that there's a param to some Fake[Up]Stream methods ignore_spurious_wakeups and this PR makes that param be ignored.

I think we need to understand why this is being used? If it's not needed can we remove it? @alyssawilk would know best.

This PR is intended to seed the discussion of which way to go for the test infrastructure. Changing to Mutex::Await(Condition) as a pattern exposed to the integration test infrastructure might be the best way to go, but that's a slightly deeper change.

Is it possible to mock up what this would look like at 1 call site? I agree this sounds better and more explicit, but it's hard for me to get a feeling of what this would look like in practice and whether it's work it or not.

[jmarantz]

@mattklein123 see 49f8649 for a mock-up. This doesn't work yet obviously (and CI is expected to fail).

[alyssawilk]

at a glance, it looks much cleaner, and I don't like the spurious events checks anyway, so I'm fine doing away with them (as long as we really clean it up - consider removing it first and land this change second, else TODO)

[jmarantz]

Thanks; sounds like there's alignment. As this overlaps with #10551 I'll wait till that goes in and then work on this.

RE spurious wakeups: as part of the PR I'd just delete the param and clean up all call-sites; the PR was basically passing tests essentially, before I committed the non-functional strawman 'await' example.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!