search

CesiumGS / cesium-native

Apache License 2.0
402 stars 205 forks source link

Add AsyncSystem::waitInMainThread #825

Closed kring closed 4 months ago

kring commented 5 months ago

Adds a new method, waitInMainThread, to AsyncSystem.

Calling asyncSystem.waitInMainThread(std::move(future)) is similar to calling future.wait(). Both wait for the future to resolve or reject, and then either return the value or throw an exception. waitInMainThread also continues to dispatch main thread tasks while waiting, however. This means that it is safe to call from the main thread even if the future has thenInMainThread continuations. future.wait(), on the other hand, is very likely to deadlock in this situation.

kring commented 4 months ago

Alright Brian, you've successfully nerd sniped me here.

Your questions reminded me that the whole sleep_for thing was a cop out. There shouldn't be any spin waiting; it should use a mutex and a condition variable to suspend the thread until there's work to do.

Conceptually this is easy-ish, but it's hard because some details we need to fiddle with are buried in Async++. Basically, to do this, we need access to the std::mutex that Async++ locks when new work is added to the main thread queue. But it's not available through the public interface of fifo_scheduler. Ok, well, we already have a wrapper for fifo_scheduler (called QueuedScheduler). And fifo_scheduler is actually pretty simple, so let's copy/paste it into QueuedScheduler. Ok it's implemented with fifo_queue, which is internal, but we can use it with just a slight hack (#include <async++/../../src/fifo_queue.h>). Our QueuedScheduler now becomes the new fifo_queue rather than delegating to it. Now to build up a signaling mechanism based on a std::condition_variable that gets notified when something new is added to the main thread task queue. Add a function to dispatch the main thread queue, waiting on the condition variable when there's nothing else to do. And make sure the condition variable is notified when the task we're waiting for completes (just in case the future is not resolved in the main thread).

Yep, that's much better! 🎉

Huh... where did the last 6 hours of my day go?

Well, at least I've addressed all your feedback. I also added support for waiting on SharedFuture as well and merged in main. This is ready for another look.

kring commented 4 months ago

Don't merge this yet. The Future resolving from a worker while main thread is waiting test is consistently deadlocking on Windows (other platforms are either fine or are much less consistent about it). I need to figure out why.

kring commented 4 months ago

I found and fixed the race condition causing the deadlock. This is ready for another review.

csciguy8 commented 4 months ago

Looks great @kring ! You've addressed all my concerns.

Async code can definitely have nuanced bugs, so appreciate the new tests go to along with this feature.

Merging...