parse-community / Parse-SDK-iOS-OSX

The Apple SDK for Parse Platform (iOS, macOS, watchOS, tvOS)
https://parseplatform.org
Other
2.81k stars 872 forks source link

IconBadgeNumber has to be set on main thread #1191

Closed mtrezza closed 7 years ago

mtrezza commented 7 years ago

After updating to iOS 11, Swift 4 and Xcode 9, calling PFInstallation.saveInBackground throws the error:

Main Thread Checker: UI API called on a background thread: -[UIApplication applicationIconBadgeNumber] PID: 69841, TID: 15989163, Thread name: (none), Queue name: com.apple.root.default-qos, QoS: 21 Backtrace: 4 MyApp 0x000000010e29dcba -[PFApplication iconBadgeNumber] + 48 5 MyApp 0x000000010e2984d3 -[PFInstallation _updateBadgeFromDevice] + 73 6 MyApp 0x000000010e2983a3 -[PFInstallation _objectWillSave] + 98 7 MyApp 0x000000010e23f633 31-[PFObject(Private) saveAsync:]_block_invoke.569 + 116 8 MyApp 0x000000010e3f574c 55-[BFTask continueWithExecutor:block:cancellationToken:]_block_invoke + 91 9 MyApp 0x000000010e3fbe47 29+[BFExecutor defaultExecutor]_block_invoke_2 + 125 10 MyApp 0x000000010e3fc36e -[BFExecutor execute:] + 65 11 MyApp 0x000000010e3f5328 -[BFTask runContinuations] + 390 12 MyApp 0x000000010e3f4d44 -[BFTask trySetResult:] + 151 13 MyApp 0x000000010e3f187d -[BFTaskCompletionSource setResult:] + 79 14 MyApp 0x000000010e3f587e __55-[BFTask continueWithExecutor:block:cancellationToken:]_block_invoke + 397 15 libclang_rt.asan_iossim_dynamic.dylib 0x000000010f5c14b2 wrap_dispatch_async_block_invoke + 290 16 libdispatch.dylib 0x0000000118d1e3f7 _dispatch_call_block_and_release + 12 17 libdispatch.dylib 0x0000000118d1f43c _dispatch_client_callout + 8 18 libdispatch.dylib 0x0000000118d24352 _dispatch_queue_override_invoke + 1458 19 libdispatch.dylib 0x0000000118d2b1f9 _dispatch_root_queue_drain + 772 20 libdispatch.dylib 0x0000000118d2ae97 _dispatch_worker_thread3 + 132 21 libsystem_pthread.dylib 0x00000001191dd5a2 _pthread_wqthread + 1299 22 libsystem_pthread.dylib 0x00000001191dd07d start_wqthread + 13

Error seems to be related to this, which should be executed on the main thread:

https://github.com/parse-community/Parse-SDK-iOS-OSX/blob/7a820b75c6726808475af9ce705053ff0e8cbd11/Parse/Internal/PFApplication.m#L74-L82

flovilmart commented 7 years ago

can you tryout that branch modernization-without-bolts with cocoapods?

mtrezza commented 7 years ago

I'm not using pods, but looking at the code I think it would solve the issue.

flovilmart commented 7 years ago

@mtrezza this should also work with Carthage!

mtrezza commented 7 years ago

@flovilmart works without error. what is this branch - is it safe for production?

flovilmart commented 7 years ago

Should be, I was wondering if it could be tested before merge/release!

mtrezza commented 7 years ago

Looks good so far, no compile or runtime warnings/errors in Xcode 9, iOS 11, Swift 4. I'll be using it for development from now on. Anything specific that should be tested?

flovilmart commented 7 years ago

I,m usure about the dispatch_sync call there: https://github.com/parse-community/Parse-SDK-iOS-OSX/pull/1186/files#diff-0ca981762244e8376faf320ec0704646R129

if something else is blocking the main queue (like a task.wait()) this could deadlock

mtrezza commented 7 years ago

Right, the wait() did pause the main queue and the location manager block never executed:

let dispatch = DispatchGroup()

dispatch.enter()

PFGeoPoint.geoPointForCurrentLocation { (geoPoint, error) in
    // Never got to here...
    dispatch.leave()
}

dispatch.wait()

However since CLLocationManager has to be called on the main queue I think it is simply not an option to pause the main queue when calling PFGeoPoint.geoPointForCurrentLocation. Maybe it would be enough to mention this in the declaration of PFGeoPoint.geoPointForCurrentLocation.

flovilmart commented 7 years ago

Yeah, calling wait on the main queue will block :/ also, in normal usage, do you think it will be an issue?

I’ll use dispatch async instead of dispatch sync

mtrezza commented 7 years ago

Yeah, I think async is the way to go. Do you see any reason why it should be sync rather than async?

flovilmart commented 7 years ago

I just pushed on the branch, but I'm not sure the semaphore will not deadlock your app as it still needs to jump back on the main queue to check the application state.

On 23 sept. 2017 09:00 -0400, Manuel notifications@github.com, wrote:

Yeah, I think async is the way to go. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mtrezza commented 7 years ago

Actually, I think it doesn't make any difference whether it's sync or async, in any case the main queue is blocked. I will try out your push.

mtrezza commented 7 years ago

Main queue is blocked also with async. Maybe just add to the docs of geoPointForCurrentLocation that it executes on the main thread and thus main cannot be paused. It's justifiable as the constraint exists by iOS design.

flovilmart commented 7 years ago

@mtrezza what do you thinks about that? https://github.com/parse-community/Parse-SDK-iOS-OSX/pull/1186/commits/18e002d40fccb9e6ce8d41ae30681d6f05ab911a#diff-eb4040482e1391e16621c54aab7cded9R60

mtrezza commented 7 years ago

I'd say perfect and ready for merge

mtrezza commented 7 years ago

What is the Bolts change in this branch about?

flovilmart commented 7 years ago

there's not changes in bolts yet. There's a PR pending to use queue for synchronization instead of locks so the thread sanitizer is happy :)

mtrezza commented 7 years ago

Allright then 👍

flovilmart commented 7 years ago

I'm still waiting for the review on Bolts and a release to be able to bump to the latest version. I'll merge this one and release

mtrezza commented 7 years ago

Fixed in 0ca981762244e8376faf320ec0704646R129

flovilmart commented 7 years ago

Thanks for the testing!

mtrezza commented 7 years ago

Thanks @flovilmart, that makes the SDK iOS 11 ready.

flovilmart commented 7 years ago

A tad late, sorry about that :/

mtrezza commented 7 years ago

Still in good time I think :)

decoyfox commented 7 years ago

Found my way here. Once I can get Parse 1.15.3 working (already posted in relevant thread), hopefully this "Main Thread Checker" warning will be fixed.