Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Initialize offline storage on main thread before snapshotting #259

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Apr 6, 2020

Ensure that offline storage has been initialized on the main thread, as MGLMapView and MGLOfflineStorage do when used directly. This fixes a regression introduced in #210 as well as fixing some potential undefined behavior from previous releases if MGLMapSnapshotter had been used on a background thread before any direct MGLOfflineStorage or MGLMapView usage.

Fixes #227.

/cc @mapbox/maps-ios

@1ec5 1ec5 added the bug Something isn't working label Apr 6, 2020
@1ec5 1ec5 added this to the release-vanillashake milestone Apr 6, 2020
@1ec5 1ec5 requested review from julianrex and a team April 6, 2020 16:43
@1ec5 1ec5 self-assigned this Apr 6, 2020
@1ec5 1ec5 force-pushed the 1ec5-snapshotter-main-227 branch from c6820bc to 17ad571 Compare April 7, 2020 01:16
// https://github.com/mapbox/mapbox-gl-native-ios/issues/227
if ([NSThread.currentThread isMainThread]) {
(void)[MGLOfflineStorage sharedOfflineStorage];
} else {
[NSException raise:NSInvalidArgumentException
format:@"startWithQueue:completionHandler: must be called from a thread with an active run loop."];
Copy link
Contributor

@nishant-karajgikar nishant-karajgikar Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably change this exception to something like "startWithQueue:completionHandler: must be called from main thread."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. This reminds me that we have an MGLAssertIsMainThread() macro, but I think we do want to raise an exception explicitly (across all build configurations) instead of potentially only asserting in Debug.

Copy link
Contributor

@nishant-karajgikar nishant-karajgikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, but we should probably change the exception that is raised.

@1ec5 1ec5 force-pushed the 1ec5-snapshotter-main-227 branch from 17ad571 to 9449b65 Compare April 7, 2020 16:43
@1ec5 1ec5 merged commit 64f0a54 into master Apr 7, 2020
@1ec5 1ec5 deleted the 1ec5-snapshotter-main-227 branch April 7, 2020 19:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught exception starting MGLMapSnapshotter before using MGLMapView or MGLOfflineStorage
2 participants