Main Thread Deadlock #84

Closed
opened 2025-12-29 15:23:52 +01:00 by adam · 16 comments
Owner

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:

  1. Main thread executes a performBlockAndWait, and begins executing its closure
  2. Background thread saves a synchronous transaction
  3. Background thread observer monitors the change on the background context
  4. Background thread executes performBlockAndWait to merge changes into the main context, and parks waiting for a lock on the context
  5. Main thread attempt to acquire a lock in executeFetchRequest, and parks waiting for the same lock that the background thread is trying to get in step 4

I 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 performBlockAndWait passes through just fine, executeFetchRequest attempts 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:

  1. Is there a reason merging changes should be done synchronously? Even in a 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 executes performBlock (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?
  2. How am I not seeing this more often? If I'm right about the cause, it seems like this deadlock should happen any time that a background synchronous context attempts to save before, but on the same runloop cycle, as the main context attempts to execute a block. In practice, I've seen this one time after launching my app hundreds of times. Is there possibly something more nuanced here that I'm missing?
  3. Is SynchronousDataTransaction safe at all? Having UnsafeDataTransaction implies 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 the AsynchronousDataTransaction.

Stack

Main Thread:

#0  0x1b36c55c in __ulock_wait ()
#1  0x02aa8b14 in _dispatch_ulock_wait ()
#2  0x02aa8c12 in _dispatch_thread_event_wait_slow ()
#3  0x02a92854 in _dispatch_barrier_sync_f_slow ()
#4  0x1dbb4b7c in _perform ()
#5  0x1dbc584e in -[NSManagedObjectContext(_NestedContextSupport) executeRequest:withContext:error:] ()
#6  0x1db1d590 in -[NSManagedObjectContext executeFetchRequest:error:] ()
#7  0x01014a94 in NSManagedObjectContext.(fetchOne<A where ...> (NSFetchRequest) -> A?).(closure #1) at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Querying.swift:108
#8  0x01014d28 in partial apply for NSManagedObjectContext.(fetchOne<A where ...> (NSFetchRequest) -> A?).(closure #1) ()
#9  0x00fa4a88 in thunk ()
#10 0x1dbba27e in developerSubmittedBlockToNSManagedObjectContextPerform ()
#11 0x1dbba11e in -[NSManagedObjectContext performBlockAndWait:] ()
#12 0x01014460 in NSManagedObjectContext.fetchOne<A where ...> (NSFetchRequest) -> A? at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Querying.swift:114
#13 0x01014038 in NSManagedObjectContext.fetchOne<A where ...> (From<A>, [FetchClause])

Background Thread:

Thread 4Queue : NSManagedObjectContext 0x14ec1950: com.corestore.rootcontext (serial)
#0  0x1b36c55c in __ulock_wait ()
#1  0x02aa8b14 in _dispatch_ulock_wait ()
#2  0x02aa8c12 in _dispatch_thread_event_wait_slow ()
#3  0x02a92854 in _dispatch_barrier_sync_f_slow ()
#4  0x1dbba144 in -[NSManagedObjectContext performBlockAndWait:] ()
#5  0x0101edfc in static NSManagedObjectContext.(mainContextForRootContext(NSManagedObjectContext) -> NSManagedObjectContext).(closure #1) at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Setup.swift:126
#6  0x0100a638 in thunk ()
#7  0x1c3f6fb0 in -[__NSObserver _doit:] ()
#8  0x1bb6d760 in __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ ()
#9  0x1bb6d09c in _CFXRegistrationPost ()
#10 0x1bb6ce80 in ___CFXNotificationPost_block_invoke ()
#11 0x1bbc881c in -[_CFXNotificationRegistrar find:object:observer:enumerator:] ()
#12 0x1bacd09c in _CFXNotificationPost ()
#13 0x1c3f4782 in -[NSNotificationCenter postNotificationName:object:userInfo:] ()
#14 0x1dbc01a4 in -[NSManagedObjectContext(_NSInternalNotificationHandling) _postContextDidSaveNotificationWithUserInfo:] ()
#15 0x1db517d6 in -[NSManagedObjectContext(_NSInternalAdditions) _didSaveChanges] ()
#16 0x1db3df98 in -[NSManagedObjectContext save:] ()
#17 0x01020c00 in NSManagedObjectContext.(saveSynchronously() -> SaveResult).(closure #1) at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Transaction.swift:118
#18 0x00fa4a88 in thunk ()
#19 0x1dbba27e in developerSubmittedBlockToNSManagedObjectContextPerform ()
#20 0x02a83d52 in _dispatch_client_callout ()
#21 0x02a8f2e2 in _dispatch_barrier_sync_f_invoke ()
#22 0x1dbba144 in -[NSManagedObjectContext performBlockAndWait:] ()
#23 0x010207bc in NSManagedObjectContext.saveSynchronously() -> SaveResult at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Transaction.swift:147
#24 0x01020d08 in NSManagedObjectContext.(saveSynchronously() -> SaveResult).(closure #1) at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Transaction.swift:134
#25 0x00fa4a88 in thunk ()
#26 0x1dbba27e in developerSubmittedBlockToNSManagedObjectContextPerform ()
#27 0x02a8f2e2 in _dispatch_barrier_sync_f_invoke ()
#28 0x1dbba144 in -[NSManagedObjectContext performBlockAndWait:] ()
#29 0x010207bc in NSManagedObjectContext.saveSynchronously() -> SaveResult at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Transaction.swift:147
#30 0x010531b4 in SynchronousDataTransaction.commitAndWait() -> SaveResult at
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: 1. Main thread executes a `performBlockAndWait`, and begins executing its closure 2. Background thread saves a synchronous transaction 3. Background thread observer monitors the change on the background context 4. Background thread executes `performBlockAndWait` to merge changes into the main context, and parks waiting for a lock on the context 5. Main thread attempt to acquire a lock in `executeFetchRequest`, and parks waiting for the same lock that the background thread is trying to get in step 4 I 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 `performBlockAndWait` passes through just fine, `executeFetchRequest` attempts 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: 1. Is there a reason merging changes should be done synchronously? Even in a `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 executes `performBlock` (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? 2. How am I not seeing this more often? If I'm right about the cause, it seems like this deadlock should happen _any_ time that a background synchronous context attempts to save before, but on the same runloop cycle, as the main context attempts to execute a block. In practice, I've seen this one time after launching my app hundreds of times. Is there possibly something more nuanced here that I'm missing? 3. Is `SynchronousDataTransaction` safe at all? Having `UnsafeDataTransaction` implies 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 the `AsynchronousDataTransaction`. ## Stack Main Thread: ``` #0 0x1b36c55c in __ulock_wait () #1 0x02aa8b14 in _dispatch_ulock_wait () #2 0x02aa8c12 in _dispatch_thread_event_wait_slow () #3 0x02a92854 in _dispatch_barrier_sync_f_slow () #4 0x1dbb4b7c in _perform () #5 0x1dbc584e in -[NSManagedObjectContext(_NestedContextSupport) executeRequest:withContext:error:] () #6 0x1db1d590 in -[NSManagedObjectContext executeFetchRequest:error:] () #7 0x01014a94 in NSManagedObjectContext.(fetchOne<A where ...> (NSFetchRequest) -> A?).(closure #1) at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Querying.swift:108 #8 0x01014d28 in partial apply for NSManagedObjectContext.(fetchOne<A where ...> (NSFetchRequest) -> A?).(closure #1) () #9 0x00fa4a88 in thunk () #10 0x1dbba27e in developerSubmittedBlockToNSManagedObjectContextPerform () #11 0x1dbba11e in -[NSManagedObjectContext performBlockAndWait:] () #12 0x01014460 in NSManagedObjectContext.fetchOne<A where ...> (NSFetchRequest) -> A? at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Querying.swift:114 #13 0x01014038 in NSManagedObjectContext.fetchOne<A where ...> (From<A>, [FetchClause]) ``` Background Thread: ``` Thread 4Queue : NSManagedObjectContext 0x14ec1950: com.corestore.rootcontext (serial) #0 0x1b36c55c in __ulock_wait () #1 0x02aa8b14 in _dispatch_ulock_wait () #2 0x02aa8c12 in _dispatch_thread_event_wait_slow () #3 0x02a92854 in _dispatch_barrier_sync_f_slow () #4 0x1dbba144 in -[NSManagedObjectContext performBlockAndWait:] () #5 0x0101edfc in static NSManagedObjectContext.(mainContextForRootContext(NSManagedObjectContext) -> NSManagedObjectContext).(closure #1) at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Setup.swift:126 #6 0x0100a638 in thunk () #7 0x1c3f6fb0 in -[__NSObserver _doit:] () #8 0x1bb6d760 in __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ () #9 0x1bb6d09c in _CFXRegistrationPost () #10 0x1bb6ce80 in ___CFXNotificationPost_block_invoke () #11 0x1bbc881c in -[_CFXNotificationRegistrar find:object:observer:enumerator:] () #12 0x1bacd09c in _CFXNotificationPost () #13 0x1c3f4782 in -[NSNotificationCenter postNotificationName:object:userInfo:] () #14 0x1dbc01a4 in -[NSManagedObjectContext(_NSInternalNotificationHandling) _postContextDidSaveNotificationWithUserInfo:] () #15 0x1db517d6 in -[NSManagedObjectContext(_NSInternalAdditions) _didSaveChanges] () #16 0x1db3df98 in -[NSManagedObjectContext save:] () #17 0x01020c00 in NSManagedObjectContext.(saveSynchronously() -> SaveResult).(closure #1) at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Transaction.swift:118 #18 0x00fa4a88 in thunk () #19 0x1dbba27e in developerSubmittedBlockToNSManagedObjectContextPerform () #20 0x02a83d52 in _dispatch_client_callout () #21 0x02a8f2e2 in _dispatch_barrier_sync_f_invoke () #22 0x1dbba144 in -[NSManagedObjectContext performBlockAndWait:] () #23 0x010207bc in NSManagedObjectContext.saveSynchronously() -> SaveResult at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Transaction.swift:147 #24 0x01020d08 in NSManagedObjectContext.(saveSynchronously() -> SaveResult).(closure #1) at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Transaction.swift:134 #25 0x00fa4a88 in thunk () #26 0x1dbba27e in developerSubmittedBlockToNSManagedObjectContextPerform () #27 0x02a8f2e2 in _dispatch_barrier_sync_f_invoke () #28 0x1dbba144 in -[NSManagedObjectContext performBlockAndWait:] () #29 0x010207bc in NSManagedObjectContext.saveSynchronously() -> SaveResult at /Project/Pods/CoreStore/Sources/Internal/NSManagedObjectContext+Transaction.swift:147 #30 0x010531b4 in SynchronousDataTransaction.commitAndWait() -> SaveResult at ```
adam added the enhancementfixedios/compiler bug labels 2025-12-29 15:23:52 +01:00
adam closed this issue 2025-12-29 15:23:52 +01:00
Author
Owner

@JohnEstropia commented on GitHub (Sep 16, 2016):

1 Is there a reason merging changes should be done synchronously? Even in a 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 executes performBlock (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?

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.

2 How am I not seeing this more often? If I'm right about the cause, it seems like this deadlock should happen any time that a background synchronous context attempts to save before, but on the same runloop cycle, as the main context attempts to execute a block. In practice, I've seen this one time after launching my app hundreds of times. Is there possibly something more nuanced here that I'm missing?

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.

3 Is SynchronousDataTransaction safe at all? Having UnsafeDataTransaction implies 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 the AsynchronousDataTransaction.

Personally, I really do encourage to only use AsynchronousDataTransactions everywhere when possible. That said, AsynchronousDataTransactions can also deadlock if it collides with a SynchronousDataTransaction. In general:

  • If you'll use AsynchronousDataTransaction, don't use SynchronousDataTransaction
  • If you'll use SynchronousDataTransaction, use only SynchronousDataTransactions everywhere else

UnsafeDataTransactions 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, unlike AsynchronousDataTransactions and SynchronousDataTransactions which run serially.

@JohnEstropia commented on GitHub (Sep 16, 2016): > 1 Is there a reason merging changes should be done synchronously? Even in a 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 executes performBlock (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? 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. > 2 How am I not seeing this more often? If I'm right about the cause, it seems like this deadlock should happen any time that a background synchronous context attempts to save before, but on the same runloop cycle, as the main context attempts to execute a block. In practice, I've seen this one time after launching my app hundreds of times. Is there possibly something more nuanced here that I'm missing? 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. > 3 Is SynchronousDataTransaction safe at all? Having UnsafeDataTransaction implies 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 the AsynchronousDataTransaction. Personally, I really do encourage to only use `AsynchronousDataTransaction`s everywhere when possible. That said, `AsynchronousDataTransaction`s can also deadlock if it collides with a `SynchronousDataTransaction`. In general: - If you'll use `AsynchronousDataTransaction`, don't use `SynchronousDataTransaction` - If you'll use `SynchronousDataTransaction`, use only `SynchronousDataTransaction`s everywhere else `UnsafeDataTransaction`s 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, unlike `AsynchronousDataTransaction`s and `SynchronousDataTransaction`s which run serially.
Author
Owner

@JohnEstropia commented on GitHub (Sep 16, 2016):

Sorry, I misunderstood your use case. So you only use SynchronousDataTransaction and just fetch through the DataStack?

@JohnEstropia commented on GitHub (Sep 16, 2016): Sorry, I misunderstood your use case. So you only use SynchronousDataTransaction and just fetch through the DataStack?
Author
Owner

@colinmorelli 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.

Right, I get this. However, my point here is that Core Data guarantees (as far as I'm aware), that blocks passed to performBlock will 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 calling performBlock: enough to ensure that changes will be merged in the order in which performBlock: 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.

Personally, I really do encourage to only use AsynchronousDataTransactions everywhere when possible. That said, AsynchronousDataTransactions can also deadlock if it collides with a SynchronousDataTransaction...

Fair, but this probably deserves more prominent featuring in the docs. That said, I think if it's possible for a SynchronousDataTransaction to get caught in a deadlock with a fetch executing on the DataStack, that's probably a bug.

@colinmorelli 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. Right, I get this. However, my point here is that Core Data guarantees (as far as I'm aware), that blocks passed to `performBlock` will 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 calling `performBlock:` enough to ensure that changes will be merged in the order in which `performBlock:` 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. > Personally, I really do encourage to only use AsynchronousDataTransactions everywhere when possible. That said, AsynchronousDataTransactions can also deadlock if it collides with a SynchronousDataTransaction... Fair, but this probably deserves more prominent featuring in the docs. That said, I think if it's possible for a `SynchronousDataTransaction` to get caught in a deadlock with a fetch executing on the DataStack, that's probably a bug.
Author
Owner

@colinmorelli commented on GitHub (Sep 16, 2016):

Sorry, I misunderstood your use case. So you only use SynchronousDataTransaction and just fetch through the DataStack?

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.

@colinmorelli commented on GitHub (Sep 16, 2016): > Sorry, I misunderstood your use case. So you only use SynchronousDataTransaction and just fetch through the DataStack? 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.
Author
Owner

@JohnEstropia commented on GitHub (Sep 16, 2016):

Have you observed this deadlock on iOS 9 and others? performBlockAndWait is documented as

This method may safely be called reentrantly.

so I'm under the impression it shouldn't wait on it's own lock. Maybe we need to wrap executeFetchRequest calls in performBlockAndWait blocks, 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?

@JohnEstropia commented on GitHub (Sep 16, 2016): Have you observed this deadlock on iOS 9 and others? `performBlockAndWait` is documented as ``` This method may safely be called reentrantly. ``` so I'm under the impression it shouldn't wait on it's own lock. Maybe we need to wrap `executeFetchRequest` calls in `performBlockAndWait` blocks, 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?
Author
Owner

@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. performBlockAndWait doesn't wait on its own lock. The background thread calls performBlockAndWait on the main context, which (correctly) waits for that lock. The main thread passes into performBlockAndWait just fine (see #11 of the first stack above). It's executeFetchRequest which gets caught waiting for a lock. Interestingly, that happens inside of the performBlockAndWait block.

@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. `performBlockAndWait` _doesn't_ wait on its own lock. The background thread calls `performBlockAndWait` on the main context, which (correctly) waits for that lock. The main thread passes into `performBlockAndWait` just fine (see #11 of the first stack above). It's `executeFetchRequest` which gets caught waiting for a lock. Interestingly, that happens _inside_ of the `performBlockAndWait` block.
Author
Owner

@JohnEstropia commented on GitHub (Sep 16, 2016):

Interestingly, that happens inside of the performBlockAndWait block.

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!

@JohnEstropia commented on GitHub (Sep 16, 2016): > Interestingly, that happens inside of the performBlockAndWait block. 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!
Author
Owner

@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 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
Author
Owner

@colinmorelli commented on GitHub (Sep 17, 2016):

Filed radar 28346032

After investigating further, my best guess is that the NSManagedObjectContextDidSaveNotificiation is fired while the performBlockAndWait method of the rootSavingContext still holds a lock on the PSC. If the save of the root context is not wrapped in performBlockAndWait, the problem is avoided. Additionally, if the merge is executed as performBlock (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).

@colinmorelli commented on GitHub (Sep 17, 2016): Filed radar 28346032 After investigating further, my best guess is that the `NSManagedObjectContextDidSaveNotificiation` is fired while the `performBlockAndWait` method of the rootSavingContext still holds a lock on the PSC. If the save of the root context is not wrapped in `performBlockAndWait`, the problem is avoided. Additionally, if the merge is executed as `performBlock` (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).
Author
Owner

@JohnEstropia commented on GitHub (Sep 17, 2016):

Wow that was great isolation work, thanks!

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).

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:

    // SynchronousDataTransaction.swift
    internal func performAndWait() -> SaveResult? {

        self.context.performBlockAndWait {} // <<<<< Here
        self.transactionQueue.sync {

            self.closure(transaction: self)
            // ...
        }
        return self.result
    }

It looks scary though

@JohnEstropia commented on GitHub (Sep 17, 2016): Wow that was great isolation work, thanks! > 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). 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: ``` swift // SynchronousDataTransaction.swift internal func performAndWait() -> SaveResult? { self.context.performBlockAndWait {} // <<<<< Here self.transactionQueue.sync { self.closure(transaction: self) // ... } return self.result } ``` It looks scary though
Author
Owner

@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 commitAndWait but 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)

@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 `commitAndWait` but 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)
Author
Owner

@JohnEstropia commented on GitHub (Sep 17, 2016):

We might not need a parameter at all, just provide a specialized

func commit() -> SaveResult?

on SynchonousDataTransaction that still performs a blocking save, but notifies observers asynchronously. I'm pretty sure people will tend to use commit() more, though, and I'm not sure if that's a good thing or not. Will put enough exclamation marks on both commit and commitAndWait's documentation 😄

I'll try to get an implementation working within the weekend.

@JohnEstropia commented on GitHub (Sep 17, 2016): We might not need a parameter at all, just provide a specialized ``` swift func commit() -> SaveResult? ``` on `SynchonousDataTransaction` that still performs a blocking save, but notifies observers asynchronously. I'm pretty sure people will tend to use `commit()` more, though, and I'm not sure if that's a good thing or not. Will put enough exclamation marks on both `commit` and `commitAndWait`'s documentation 😄 I'll try to get an implementation working within the weekend.
Author
Owner

@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?

@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?
Author
Owner

@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 as SynchonousDataTransactions can get, while still satisfying its transaction contract.

@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 as `SynchonousDataTransaction`s can get, while still satisfying its transaction contract.
Author
Owner

@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): PR was merged. Will update master branch and Cococapods trunk within a day
Author
Owner

@JohnEstropia commented on GitHub (Sep 30, 2016):

@colinmorelli Thanks a lot!

@JohnEstropia commented on GitHub (Sep 30, 2016): @colinmorelli Thanks a lot!
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/CoreStore#84