wil::wait_for_completion_or_timeout_nothrow() returns E_ILLEGAL_METHOD_CALL (0x8000000e) for timeouts

[brbeec]

Why is this change being made?

Async operation implementations that inherit from Microsoft::WRL::AsyncBase call Microsoft::WRL::AsyncBase::CheckValidStateForResultsCall() in GetResults() which will return E_ILLEGAL_METHOD_CALL (0x8000000e) when the resultType is AsyncResultType::SingleResult and the async operation hasn't completed yet.

The wil::details::WaitForCompletion() overload with a result output parameter calls GetResults() on the async operation without checking if it timed out first which results in E_ILLEGAL_METHOD_CALL being returned to the caller when an operation times out, rather than S_OK as documented in wil\winrt.h.

What changed?

I updated wil::details::WaitForCompletion() to return S_OK and not call GetResults() on the async operation if it timed out.

Resolves #322

How was the change tested?

I updated FakeAsyncOperation::GetResults() to return E_ILLEGAL_METHOD_CALL when the operation has not completed yet, and I added a new test, WinRTTests::WaitForCompletionTimeoutWithResult(), to exercise the timeout behavior for the wil::details::WaitForCompletion() overload with a result output parameter.

I verified that WinRTTests::WaitForCompletionTimeoutWithResult() fails without the change to wil::details::WaitForCompletion() and passes with it.

[dunhor]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[dunhor]

This is unfortunately a breaking change and searching through uses, it looks like it could have real impact. I need to look at it more, but there's a real chance we cannot take this change. An alternative might be to change the return value to HRESULT_FROM_WIN32(ERROR_TIMEOUT)

[brbeec]

This is unfortunately a breaking change and searching through uses, it looks like it could have real impact. I need to look at it more, but there's a real chance we cannot take this change. An alternative might be to change the return value to HRESULT_FROM_WIN32(ERROR_TIMEOUT)

IMO the alternative is actually preferable and how the API should have worked in the first place rather than returning S_OK and requiring the caller to check the returned HRESULT and the timedOut output parameter. Having the wil::wait_for_completion_or_timeout_nothrow() overloads behave differently on timeouts would probably be just as confusing and surprising to developers as the current behavior though :(

Could we potentially deprecate the existing wil::wait_for_completion_or_timeout_nothrow() overloads and create a new set that don't take a timedOut output parameter and return HRESULT_FROM_WIN32(ERROR_TIMEOUT) on timeout? That would avoid breaking existing consumers and align the behavior.

If we can't change any of the existing behavior and don't want to create new overloads, at a minimum we should update the On timeout, S_OK is returned, with timedOut set to true comment as it's inaccurate and misleading.