mirror of
https://github.com/JohnEstropia/CoreStore.git
synced 2026-01-11 20:00:30 +01:00
Main Thread Deadlock #84
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @colinmorelli on GitHub (Sep 16, 2016).
It looks like it's possible to get the main thread stuck in a deadlock - seen on iOS 10 GM on an iPhone 5S.
There's nothing crazy about this case. There are two transactions, one synchronous, one "implicit" (reading directly from the main context through
dataStack.fetchOne(). The resulting stacks at the time of the deadlock are at the bottom of this.As best I can tell:
performBlockAndWait, and begins executing its closureperformBlockAndWaitto merge changes into the main context, and parks waiting for a lock on the contextexecuteFetchRequest, and parks waiting for the same lock that the background thread is trying to get in step 4I would think that when using a context with a main queue concurrency type on the main thread, there would be optimizations avoiding the need to acquire locks. The fact that you're executing in the main thread is enough to ensure nobody else can be executing on the main thread.
However, it appears that, while
performBlockAndWaitpasses through just fine,executeFetchRequestattempts to lock (the context? the thread?). However, at this point the background is already first in line for that lock.I can work around this issue by using asynchronous transactions on the background thread, but this raises a number of questions:
SynchronousDataTransaction, you're really asking that the write to the root saving context is performed synchronously. This can be done without merging the changes to the main queue synchronously. Given that CoreStore ensures transactions run synchronously, and GCD ensures fairness in the execution of dispatch blocks, as long as each transaction executesperformBlock(async) to merge their changes into the main queue, they should all merge in the order they were committed, which means the only reason for failure should be caused memory pressure, as I understand it. Is this assumption wrong? If it can't really get out of sync, why force the merge to be synchronous?SynchronousDataTransactionsafe at all? HavingUnsafeDataTransactionimplies that either the sync/async variants are safe counterparts, but given such a simple example as this - it seems that the only truly safe transaction is theAsynchronousDataTransaction.Stack
Main Thread:
Background Thread:
@JohnEstropia commented on GitHub (Sep 16, 2016):
If we don't do merging synchronously (or more accurately, immediately), there might be changes that sneaked into the context by the time the actual merging occurs. You'll then end up with possible corrupted data.
Transactions are generally fast. So the probability that your synchronous fetch collides with the transaction's save is really low. But it's frequent enough to be observed, as you have experienced.
Personally, I really do encourage to only use
AsynchronousDataTransactions everywhere when possible. That said,AsynchronousDataTransactions can also deadlock if it collides with aSynchronousDataTransaction. In general:AsynchronousDataTransaction, don't useSynchronousDataTransactionSynchronousDataTransaction, use onlySynchronousDataTransactions everywhere elseUnsafeDataTransactions being called as such have no relation to deadlocks. They're more prone to data collision and merge conflicts because their changes can overlap with other transactions, unlikeAsynchronousDataTransactions andSynchronousDataTransactions which run serially.@JohnEstropia commented on GitHub (Sep 16, 2016):
Sorry, I misunderstood your use case. So you only use SynchronousDataTransaction and just fetch through the DataStack?
@colinmorelli commented on GitHub (Sep 16, 2016):
Right, I get this. However, my point here is that Core Data guarantees (as far as I'm aware), that blocks passed to
performBlockwill be executed in the order that they were called. Additionally, it guarantees that the blocks are executed one at a time for any given context (due to the core data concurrency requirements). Further, the main context isn't written to by Core Store. So, then, isn't callingperformBlock:enough to ensure that changes will be merged in the order in whichperformBlock:was called? What additional guarantees does waiting for the merge to complete actually provide. I should note here that, when using asynchronous data transactions, changes aren't merged synchronously - and it seems to be fine there.Fair, but this probably deserves more prominent featuring in the docs. That said, I think if it's possible for a
SynchronousDataTransactionto get caught in a deadlock with a fetch executing on the DataStack, that's probably a bug.@colinmorelli commented on GitHub (Sep 16, 2016):
That's correct. I can switch the other transactions to be async. I chose to keep them as sync because they're already running on background threads anyway, so going async would just be another layer of unnecessary callbacks.
@JohnEstropia commented on GitHub (Sep 16, 2016):
Have you observed this deadlock on iOS 9 and others?
performBlockAndWaitis documented asso I'm under the impression it shouldn't wait on it's own lock. Maybe we need to wrap
executeFetchRequestcalls inperformBlockAndWaitblocks, which looks weird when we're sure we're already running on the main thread.Do you have test cases we can reliably reproduce deadlocks with so we can check if any particular fixes work?
@colinmorelli commented on GitHub (Sep 16, 2016):
I haven't seen it in iOS9, but I've been testing on 10 for quite some time now. That said, I've been developing with CoreStore for a couple months now and this is the first time I've seen it on 10 either. So I think it's safe to say it's not a common occurrence.
Check out the stack above, though.
performBlockAndWaitdoesn't wait on its own lock. The background thread callsperformBlockAndWaiton the main context, which (correctly) waits for that lock. The main thread passes intoperformBlockAndWaitjust fine (see #11 of the first stack above). It'sexecuteFetchRequestwhich gets caught waiting for a lock. Interestingly, that happens inside of theperformBlockAndWaitblock.@JohnEstropia commented on GitHub (Sep 16, 2016):
Yeah, it's already running on the block the context itself designated.
Try submitting a bug report to Apple with your iOS 10 stack traces. For now, although the behaviour we're seeing is reasonable, I can't think of how we can work around this within CoreStore. I'll keep this issue open for a while, please share if you have ideas.
Thanks for the report!
@colinmorelli commented on GitHub (Sep 16, 2016):
It must be much more nuanced than I originally thought. I'm struggling to come up with a reproduction even in a very contrived scenario (using dispatch groups to try to push them into a deadlock). This is without using CoreStore. So it's possible there are differences in how it's setup. My reproduction app has a root saving context, main context as a child of the root saving, and a "transaction" context was a private concurrency child of the root saving context.
Scratch that: it appears I was able to reproduce. Will file the bug report and commit the test project as well.
In case you're curious: https://github.com/colinmorelli/CoreDataDeadlockTest
@colinmorelli commented on GitHub (Sep 17, 2016):
Filed radar 28346032
After investigating further, my best guess is that the
NSManagedObjectContextDidSaveNotificiationis fired while theperformBlockAndWaitmethod of the rootSavingContext still holds a lock on the PSC. If the save of the root context is not wrapped inperformBlockAndWait, the problem is avoided. Additionally, if the merge is executed asperformBlock(non-sync), the problem is avoided.@JohnEstropia given the discussion above, is there any reason not to make the default behavior of CoreStore to merge the changes into the main queue asynchronously, as long as they're submitted for merging in the proper order (which CoreStore's concurrency guarantees should promise).
@JohnEstropia commented on GitHub (Sep 17, 2016):
Wow that was great isolation work, thanks!
This was actually the behavior long ago, but we we hit a case where there's a mismatch between the persistent store's state and context's state (that should have been updated with a merge) during the "pre-merge" window. In essence, all faulted objects during that "pre-merge" state are liabilities. See https://github.com/JohnEstropia/CoreStore/issues/65
I'm wondering if this is a good idea:
It looks scary though
@colinmorelli commented on GitHub (Sep 17, 2016):
Got it. If I understand #65 correctly it seems like it was less of a bug than a feature/improvement/clarification? Basically, a synchronous transaction could save with
commitAndWaitbut when querying from the UI context immediately after the data might not be there yet.At this point, there seems no doubt in my mind that this is a core data bug (and not a core store one), but to the extent we could get core store to work around it, I feel like it'd be beneficial. There's a very easy option (though possibly not the cleanest), where
commitAndWait()can take an optional parameter about whether the merge should also be performed synchronously (defaulting to true, for backwards compat)@JohnEstropia commented on GitHub (Sep 17, 2016):
We might not need a parameter at all, just provide a specialized
on
SynchonousDataTransactionthat still performs a blocking save, but notifies observers asynchronously. I'm pretty sure people will tend to usecommit()more, though, and I'm not sure if that's a good thing or not. Will put enough exclamation marks on bothcommitandcommitAndWait's documentation 😄I'll try to get an implementation working within the weekend.
@colinmorelli commented on GitHub (Sep 17, 2016):
Do you think that's potentially confusing as it conflicts with the definition of
commit()on AsynchronousDataTransaction and UnsafeDataTransaction, which both perform an async commit?@JohnEstropia commented on GitHub (Sep 17, 2016):
I don't know, but I personally think it works to our advantage. The asynchronicity of
commit()is an implementation detail, and not notifying observers immediately is as asynchronous asSynchonousDataTransactions can get, while still satisfying its transaction contract.@JohnEstropia commented on GitHub (Sep 30, 2016):
PR was merged. Will update master branch and Cococapods trunk within a day
@JohnEstropia commented on GitHub (Sep 30, 2016):
@colinmorelli Thanks a lot!