Skip to content

Commit

Permalink
Attempt to fix a deadlock in ContextManager.saveDerivedContext
Browse files Browse the repository at this point in the history
When `wait` argument is true, `saveDerivedContext` method essentially
translates into this:

```
derivedContext.performBlockAndWait {
    derivedContext.save()

    mainContext.performBlockAndWait {
        mainContext.save()
    }
}
```

This change moves the inner `performBlockAndWait` out, which achieves
some thing as before - save derived context first, then save main context,
the method returns after both context are saved.
  • Loading branch information
crazytonyli committed May 1, 2022
1 parent 98dc3b7 commit aca7dab
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 10 deletions.
2 changes: 1 addition & 1 deletion WordPress/Classes/Utility/ContextManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,8 @@ - (void)saveDerivedContext:(NSManagedObjectContext *)context andWait:(BOOL)wait
if (wait) {
[context performBlockAndWait:^{
[self internalSaveContext:context];
[self saveContext:self.mainContext andWait:wait withCompletionBlock:completionBlock];
}];
[self saveContext:self.mainContext andWait:wait withCompletionBlock:completionBlock];
} else {
[context performBlock:^{
[self internalSaveContext:context];
Expand Down
9 changes: 0 additions & 9 deletions WordPress/WordPressTest/ContextManagerMock.m
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,6 @@ - (NSManagedObjectContext *)mainContext
return _mainContext;
}

- (void)saveContextAndWait:(NSManagedObjectContext *)context
{
[super saveContextAndWait:context];
// FIXME: Remove this method to use superclass one instead
// This log magically resolves a deadlock in
// `ZDashboardCardTests.testShouldNotShowQuickStartIfDefaultSectionIsSiteMenu`
NSLog(@"Context save completed");
}

- (NSURL *)storeURL
{
NSString *documentsDirectory = [NSSearchPathForDirectoriesInDomains(NSDocumentDirectory,
Expand Down
17 changes: 17 additions & 0 deletions WordPress/WordPressTest/ContextManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,23 @@ class ContextManagerTests: XCTestCase {
}
}

func testNoDeadlockWhenSavingDerivedContext() {
// The purpose of this test is checking if it executes without getting
// stuck. That's why there is no assertion in this test method.
//
// But it the test does get stuck, the result is kind of catastrophic,
// as it will block the rest of the test suite from being executed.

let context = contextManager.newDerivedContext()
let blog = BlogBuilder(context).build()
let guide = QuickStartTourGuide.shared
guide.setup(for: blog, type: .newSite)
// This method calls `ContextManager.saveContextAndWait` twice with
// a derived context, which caused a deadlock - the second call was
// stuck at `performBlockAndWait`.
guide.remove(from: blog)
}

// MARK: - Helper Methods

fileprivate func startupCoredataStack(_ modelName: String) {
Expand Down

0 comments on commit aca7dab

Please sign in to comment.