search

Quick / Nimble

A Matcher Framework for Swift and Objective-C
https://quick.github.io/Nimble/documentation/nimble/
Apache License 2.0
4.8k stars 601 forks source link

Get toEventually et al working in async contexts. #1007

Closed younata closed 1 year ago

younata commented 2 years ago

Resolves https://github.com/Quick/Nimble/issues/1006

The Problem

1006 is a result of RunLoop not being available on Swift Async contexts. I found that, effectively, these methods turn into no-ops if you run them in a swift async context (even if the function itself is not marked as async, if it's in a call hierarchy that has an async function, then that's still an async context). Which means that when the wait function continuously calls RunLoop.run(mode:before:) to spin the runloop while the matcher polling happens Await.swift:264-267, what's actually happening is that it's just continuously checking self.promise.asyncResult.isIncomplete() - almost as if the run(mode:before:) call isn't even in there. Which is a problem because using the RunLoop.run methods is how you can yield control of the current thread to other tasks.

Backing up a bit further, toEventually and waitUntil both work by combining 2 dispatch sources with control of the RunLoop. They set up one dispatch source as the timeout source and the other as the polling source (note that waitUntil does not use the polling source, but the done function argument is the equivalent of a polling source).

The timeout source uses a background queue, and runs once the timeout occurs. When the timeout happens, sets the result of the entire matcher to .timedOut and then stops the runloop (there's also logic for detecting a stalled runloop, which is unimportant to this discussion) [](). It's important to note that, because the timeout source is attached to a background queue, its handler is called on a background thread. This is why #1006 is "always results in a timeout error" instead of "hangs the test".

The polling source uses the main queue, and runs every pollInterval. This is where the actual matcher checking happens. This reruns the matcher, checking if it's passed. If the matcher has passed (or thrown an error), then the poller sets the result of the entire matcher to .completed and stops the runloop Awaiter.poll.

As noted earlier, waitUntil doesn't use a polling source. What it does is, when you call the done function argument, it runs logic on the main thread which sets the await result to .completed and stops the runloop Awaiter.performBlock. Which, for this bug, is effectively the same as the "continuously poll the matcher on the main thread" logic that toEventually uses. It also shows that the root of the issue is not the dispatch sources.

The dispatch sources work great in either async or synchronous contexts. As I stated earlier, the issue is that, while waiting for the matcher to get polled (or for done() to be called), AwaitPromiseBuilder.wait is refusing to yield control of the main thread.

For those curious why toEventually works when called in an async context that's not the main actor? Because in that case, wait is not running on the main thread. It's still pegging the CPU and refusing to yield control, but since it's not running on the main thread, the only consequence is unnecessarily using resources. So, it's spinning and continuously checking if the m matcher has completed/done() has been called, but not blocking the main thread.

The Solution

So, the solution? Stop using RunLoop.run() to yield control of the main thread.

Simpler said then done. The only other way I know of is to use Task.yield, but that would require me to make Wait async, and everything that calls it also async. Which is exactly what I did. I effectively duplicated the await methods, marked the old ones as noasync, and adjusted these new ones to work well in an async world.

The async version of toEventually uses a new async version of execute. The noasync version uses a Predicate under the hood so that it can re-use execute. Given that predicates aren't (yet?) Async-aware, I couldn't use that same approach. So, instead, this async version of execute is almost entirely the same as the synchronous version, but instead of taking in a predicate, it takes in a predicateExecutor (which is an async function), which returns the predicate result. This allows me to use an async version of Polling/async (this PR renames that function to poll) to call into the now async-aware versions of the old polling logic.

This was a wild bug.

jessesquires commented 1 year ago

seriously, what an exemplary PR this is. 🙌🏼

thanks for the thorough explanations and documentation!