Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calculation methods call completion handlers on inconsistent queues #411

Closed
1ec5 opened this issue Feb 11, 2020 · 2 comments · Fixed by #410
Closed

Calculation methods call completion handlers on inconsistent queues #411

1ec5 opened this issue Feb 11, 2020 · 2 comments · Fixed by #410
Labels

Comments

@1ec5
Copy link
Contributor

1ec5 commented Feb 11, 2020

As of #382, sometimes calculate(_:completionHandler:) and calculateRoutes(matching:completionHandler:) call the completion handler on the URLSession delegate queue:

guard let response = possibleResponse, ["application/json", "text/html"].contains(response.mimeType) else {
completionHandler(nil, nil, .invalidResponse)
return
}
guard let data = possibleData else {
completionHandler(nil, nil, .noData)
return
}
if let error = possibleError {
completionHandler(nil, nil, .unknown(response: possibleResponse, underlying: error, code: nil, message: nil))
return
}

whereas sometimes these methods call the completion handler on the main queue:

DispatchQueue.main.async {
completionHandler(nil, nil, apiError)
}
DispatchQueue.main.async {
completionHandler(result.waypoints, routes, nil)
}
DispatchQueue.main.async {
let bailError = Directions.informativeError(code: nil, message: nil, response: response, underlyingError: error)
completionHandler(nil, nil, bailError)
}

The methods should consistently call the completion handler from a single queue, and we should document which queue that is. Previously, in v0.30.0, these methods always called the completion handler on the main queue:

DispatchQueue.main.async {
errorHandler(apiError)
}
DispatchQueue.main.async {
completionHandler(json)
}

As it is, a developer doesn’t know whether their completion handler implementation needs to dispatch to the main queue or stay on the current queue. So, for example, a completion handler that presents a UIAlertController would result in a crash like this:

#0	(null) in __exceptionPreprocess ()
#1	(null) in objc_exception_throw ()
#2	(null) in _AssertAutolayoutOnAllowedThreadsOnly ()
#3	(null) in -[NSISEngine withBehaviors:performModifications:] ()
#4	(null) in -[UIView(UIConstraintBasedLayout) _calculatedSystemLayoutSizeFittingSize:withHorizontalFittingPri... ()
#5	(null) in -[UIView(AdditionalLayoutSupport) _systemLayoutSizeFittingSize:withHorizontalFittingPriority:vert... ()
#6	(null) in -[_UIAlertControllerView _minimumSizeForAllActions] ()
#7	(null) in -[_UIAlertControllerView _itemSizeForHorizontalLayout:visualStyleRequiresActionRepresentationToFi... ()
#8	(null) in -[_UIAlertControllerView _canLayOutActionsHorizontally] ()
#9	(null) in -[_UIAlertControllerView _actionLayoutDirectionChanged] ()
#10	(null) in -[_UIAlertControllerView _updateStyleForIdiomChange:] ()
#11	(null) in -[_UIAlertControllerView _setVisualStyle:] ()
#12	(null) in -[UIAlertController loadView] ()
#13	(null) in -[UIViewController loadViewIfRequired] ()
#14	(null) in -[UIViewController view] ()
#15	(null) in -[UIViewController _setPresentationController:] ()
#16	(null) in -[UIViewController _presentViewController:modalSourceViewController:presentationController:animat... ()
#17	(null) in -[UIViewController _presentViewController:withAnimationController:completion:] ()
#18	(null) in __63-[UIViewController _presentViewController:animated:completion:]_block_invoke ()
#19	(null) in -[UIViewController _performCoordinatedPresentOrDismiss:animated:] ()
#20	(null) in -[UIViewController _presentViewController:animated:completion:] ()
#21	(null) in -[UIViewController presentViewController:animated:completion:] ()

/cc @mapbox/navigation-ios

@1ec5 1ec5 added the bug label Feb 11, 2020
@1ec5
Copy link
Contributor Author

1ec5 commented Feb 14, 2020

Oh cool, I hadn’t noticed #410, which fixes this issue.

@1ec5 1ec5 linked a pull request Feb 14, 2020 that will close this issue
@1ec5
Copy link
Contributor Author

1ec5 commented Feb 14, 2020

Fixed in #410.

@1ec5 1ec5 closed this as completed Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant