Explicitly initialize default runloop on Darwin #6613
Conversation
|
@kkaefer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @1ec5 and @incanus to be potential reviewers. |
| @@ -22,6 +21,7 @@ RunLoop* RunLoop::Get() { | |||
|
|
|||
| RunLoop::RunLoop(Type) | |||
| : impl(std::make_unique<Impl>()) { | |||
| assert(!current.get()); | |||
There was a problem hiding this comment.
Allows us to quickly catch duplicate RunLoops in the future.
| /// Initializes the run loop shim that lives on the main thread. | ||
| void MGLinitializeRunLoop() { | ||
| static mbgl::util::RunLoop mainRunLoop; | ||
| } |
There was a problem hiding this comment.
Static function to avoid the fiasco
There was a problem hiding this comment.
Also means that this RunLoop isn't destructed. We could use weak_ptr/shared_ptr and tie them to the lifetime of the MGLMapView object
There was a problem hiding this comment.
Another option would’ve been to have this be a global static, unset, and set it in a dispatch_once() block. This is a common pattern in Objective-C code.
| @@ -15,6 +16,7 @@ class RasterTileTest { | |||
| public: | |||
| FakeFileSource fileSource; | |||
| TransformState transformState; | |||
| util::RunLoop loop; | |||
There was a problem hiding this comment.
Tests were missing an explicit RunLoop. If running these tests separately, this is not an issue because we had the default RunLoop, but when running after other unit tests that create/destroy a RunLoop, we got a null RunLoop further down the line.
#6579 introduced a unit test crash on macOS that exposed a larger issue: In the Darwin
RunLoopimplementation, we are statically initializing aRunLoopobject for the main thread. However, this object is overwritten, and subsequently destructed/nulled by most unit tests that use their ownRunLoop. This moves the static initialization to the macOS and iOS SDK. The GLFW implementation which also runs on macOS already creates its ownRunLoop./cc @jfirebaugh