Core Data Store: Handle concurrency properly

All concurrency from other threads should pass through the viewContext's performBlock or performBlockAndWait functions, and no other way. So now, all access to Core Data is either happening on the main thread, or by using these code blocks, all of which will wait for their access to proceed. Signed-off-by: Christopher Snowhill <[email protected]>
losnoco · Oct 30, 2022 · 4bed0af · 4bed0af
1 parent e3ae283
commit 4bed0af
Show file tree

Hide file tree

Showing 10 changed files with 366 additions and 450 deletions.
diff --git a/Application/AppController.m b/Application/AppController.m
@@ -223,12 +223,7 @@ - (void)awakeFromNib {
 		request.predicate = predicate;
 
 		NSError *error = nil;
-		[playlistController.persistentContainerLock lock];
 		NSArray *results = [playlistController.persistentContainer.viewContext executeFetchRequest:request error:&error];
-		if(results) {
-			results = [results copy];
-		}
-		[playlistController.persistentContainerLock unlock];
 
 		if(results && [results count] == 1) {
 			PlaylistEntry *pe = results[0];
@@ -434,21 +429,17 @@ - (void)applicationWillTerminate:(NSNotification *)aNotification {
 
 	for(PlaylistEntry *pe in playlistController.arrangedObjects) {
 		if(pe.deLeted) {
-			[playlistController.persistentContainerLock lock];
 			[moc deleteObject:pe];
-			[playlistController.persistentContainerLock unlock];
 			continue;
 		}
 		if([artLeftovers objectForKey:pe.artHash]) {
 			[artLeftovers removeObjectForKey:pe.artHash];
 		}
 	}
 
-	[playlistController.persistentContainerLock lock];
 	for(NSString *key in artLeftovers) {
 		[moc deleteObject:[artLeftovers objectForKey:key]];
 	}
-	[playlistController.persistentContainerLock unlock];
 
 	[playlistController commitPersistentStore];
 

diff --git a/Playlist/PlaylistController.h b/Playlist/PlaylistController.h
@@ -68,7 +68,6 @@ typedef NS_ENUM(NSInteger, URLOrigin) {
 @property(retain) NSString *_Nullable totalTime;
 @property(retain) NSString *_Nullable currentStatus;
 
-@property(strong, nonatomic, readonly) NSLock *_Nonnull persistentContainerLock;
 @property(strong, nonatomic, readonly) NSPersistentContainer *_Nonnull persistentContainer;
 @property(strong, nonatomic, readonly) NSMutableDictionary<NSString *, AlbumArtwork *> *_Nonnull persistentArtStorage;
 
@@ -143,7 +142,6 @@ typedef NS_ENUM(NSInteger, URLOrigin) {
 - (void)readShuffleListFromDataStore;
 
 + (NSPersistentContainer *_Nonnull)sharedPersistentContainer;
-+ (NSLock *_Nonnull)sharedPersistentContainerLock;
 
 // reload metadata of selection
 - (IBAction)reloadTags:(id _Nullable)sender;

diff --git a/Playlist/PlaylistController.m b/Playlist/PlaylistController.m
@@ -30,8 +30,6 @@
 
 extern BOOL kAppControllerShuttingDown;
 
-NSLock *kPersistentContainerLock = nil;
-
 NSPersistentContainer *kPersistentContainer = nil;
 
 @implementation PlaylistController
@@ -127,10 +125,6 @@ - (id)initWithCoder:(NSCoder *)decoder {
 	}];
 
 	kPersistentContainer = self.persistentContainer;
-
-	_persistentContainerLock = [[NSLock alloc] init];
-
-	kPersistentContainerLock = self.persistentContainerLock;
 
 	self.persistentContainer.viewContext.mergePolicy = NSMergeByPropertyObjectTrumpMergePolicy;
 
@@ -144,10 +138,6 @@ + (NSPersistentContainer *)sharedPersistentContainer {
 	return kPersistentContainer;
 }
 
-+ (NSLock *)sharedPersistentContainerLock {
-	return kPersistentContainerLock;
-}
-
 - (void)awakeFromNib {
 	[super awakeFromNib];
 
@@ -251,76 +241,76 @@ - (void)updateProgressText {
 }
 
 - (void)commitPersistentStore {
-	NSError *error = nil;
-	[self.persistentContainerLock lock];
-	[self.persistentContainer.viewContext save:&error];
-	[self.persistentContainerLock unlock];
-	if(error) {
-		ALog(@"Error committing playlist storage: %@", [error localizedDescription]);
-	}
+	[self.persistentContainer.viewContext performBlockAndWait:^{
+		NSError *error = nil;
+		[self.persistentContainer.viewContext save:&error];
+		if(error) {
+			ALog(@"Error committing playlist storage: %@", [error localizedDescription]);
+		}
+	}];
 }
 
 - (void)updatePlayCountForTrack:(PlaylistEntry *)pe {
 	if(pe.countAdded) return;
 	pe.countAdded = YES;
 
-	PlayCount *pc = pe.playCountItem;
+	__block PlayCount *pc = pe.playCountItem;
 
 	if(pc) {
-		[self.persistentContainerLock lock];
-		pc.count += 1;
-		pc.lastPlayed = [NSDate date];
-		[self.persistentContainerLock unlock];
+		[self.persistentContainer.viewContext performBlockAndWait:^{
+			pc.count += 1;
+			pc.lastPlayed = [NSDate date];
+		}];
 	} else {
-		[self.persistentContainerLock lock];
-		pc = [NSEntityDescription insertNewObjectForEntityForName:@"PlayCount" inManagedObjectContext:self.persistentContainer.viewContext];
-		pc.count = 1;
-		pc.firstSeen = pc.lastPlayed = [NSDate date];
-		pc.album = pe.album;
-		pc.artist = pe.artist;
-		pc.title = pe.title;
-		pc.filename = pe.filenameFragment;
-		[self.persistentContainerLock unlock];
+		[self.persistentContainer.viewContext performBlockAndWait:^{
+			pc = [NSEntityDescription insertNewObjectForEntityForName:@"PlayCount" inManagedObjectContext:self.persistentContainer.viewContext];
+			pc.count = 1;
+			pc.firstSeen = pc.lastPlayed = [NSDate date];
+			pc.album = pe.album;
+			pc.artist = pe.artist;
+			pc.title = pe.title;
+			pc.filename = pe.filenameFragment;
+		}];
 	}
 
 	[self commitPersistentStore];
 }
 
 - (void)firstSawTrack:(PlaylistEntry *)pe {
-	PlayCount *pc = pe.playCountItem;
+	__block PlayCount *pc = pe.playCountItem;
 
 	if(!pc) {
-		[self.persistentContainerLock lock];
-		pc = [NSEntityDescription insertNewObjectForEntityForName:@"PlayCount" inManagedObjectContext:self.persistentContainer.viewContext];
-		pc.count = 0;
-		pc.firstSeen = [NSDate date];
-		pc.album = pe.album;
-		pc.artist = pe.artist;
-		pc.title = pe.title;
-		pc.filename = pe.filenameFragment;
-		[self.persistentContainerLock unlock];
-	}
-}
-
-- (void)ratingUpdatedWithEntry:(PlaylistEntry *)pe rating:(CGFloat)rating {
-	if(pe && !pe.deLeted) {
-		PlayCount *pc = pe.playCountItem;
-
-		if(!pc) {
-			[self.persistentContainerLock lock];
+		[self.persistentContainer.viewContext performBlockAndWait:^{
 			pc = [NSEntityDescription insertNewObjectForEntityForName:@"PlayCount" inManagedObjectContext:self.persistentContainer.viewContext];
 			pc.count = 0;
 			pc.firstSeen = [NSDate date];
 			pc.album = pe.album;
 			pc.artist = pe.artist;
 			pc.title = pe.title;
 			pc.filename = pe.filenameFragment;
-			[self.persistentContainerLock unlock];
+		}];
+	}
+}
+
+- (void)ratingUpdatedWithEntry:(PlaylistEntry *)pe rating:(CGFloat)rating {
+	if(pe && !pe.deLeted) {
+		__block PlayCount *pc = pe.playCountItem;
+
+		if(!pc) {
+			[self.persistentContainer.viewContext performBlockAndWait:^{
+				pc = [NSEntityDescription insertNewObjectForEntityForName:@"PlayCount" inManagedObjectContext:self.persistentContainer.viewContext];
+				pc.count = 0;
+				pc.firstSeen = [NSDate date];
+				pc.album = pe.album;
+				pc.artist = pe.artist;
+				pc.title = pe.title;
+				pc.filename = pe.filenameFragment;
+			}];
 		}
 
-		[self.persistentContainerLock lock];
-		pc.rating = rating;
-		[self.persistentContainerLock unlock];
+		[self.persistentContainer.viewContext performBlockAndWait:^{
+			pc.rating = rating;
+		}];
 
 		[self commitPersistentStore];
 	}
@@ -330,15 +320,19 @@ - (void)resetPlayCountForTrack:(PlaylistEntry *)pe {
 	PlayCount *pc = pe.playCountItem;
 
 	if(pc) {
-		pc.count = 0;
+		[self.persistentContainer.viewContext performBlockAndWait:^{
+			pc.count = 0;
+		}];
 	}
 }
 
 - (void)removeRatingForTrack:(PlaylistEntry *)pe {
 	PlayCount *pc = pe.playCountItem;
 
 	if(pc) {
-		pc.rating = 0;
+		[self.persistentContainer.viewContext performBlockAndWait:^{
+			pc.rating = 0;
+		}];
 	}
 }
 
@@ -1380,19 +1374,16 @@ - (void)readQueueFromDataStore {
 	request.predicate = predicate;
 	request.sortDescriptors = @[sortDescriptor];
 
-	NSError *error = nil;
-	[self.persistentContainerLock lock];
-	NSArray *results = [self.persistentContainer.viewContext executeFetchRequest:request error:&error];
-    if(results) {
-		results = [results copy];
-    }
-	[self.persistentContainerLock unlock];
+	[self.persistentContainer.viewContext performBlockAndWait:^{
+		NSError *error = nil;
+		NSArray *results = [self.persistentContainer.viewContext executeFetchRequest:request error:&error];
 
-	if(results && [results count] > 0) {
-		[queueList removeAllObjects];
-		NSIndexSet *indexSet = [NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [results count])];
-		[queueList insertObjects:results atIndexes:indexSet];
-	}
+		if(results && [results count] > 0) {
+			[queueList removeAllObjects];
+			NSIndexSet *indexSet = [NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [results count])];
+			[queueList insertObjects:results atIndexes:indexSet];
+		}
+	}];
 }
 
 - (void)readShuffleListFromDataStore {
@@ -1404,19 +1395,16 @@ - (void)readShuffleListFromDataStore {
 	request.predicate = predicate;
 	request.sortDescriptors = @[sortDescriptor];
 
-	NSError *error = nil;
-	[self.persistentContainerLock lock];
-	NSArray *results = [self.persistentContainer.viewContext executeFetchRequest:request error:&error];
-    if(results) {
-		results = [results copy];
-    }
-	[self.persistentContainerLock unlock];
+	[self.persistentContainer.viewContext performBlockAndWait:^{
+		NSError *error = nil;
+		NSArray *results = [self.persistentContainer.viewContext executeFetchRequest:request error:&error];
 
-	if(results && [results count] > 0) {
-		[shuffleList removeAllObjects];
-		NSIndexSet *indexSet = [NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [results count])];
-		[shuffleList insertObjects:results atIndexes:indexSet];
-	}
+		if(results && [results count] > 0) {
+			[shuffleList removeAllObjects];
+			NSIndexSet *indexSet = [NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [results count])];
+			[shuffleList insertObjects:results atIndexes:indexSet];
+		}
+	}];
 }
 
 - (void)addShuffledListToFront {
@@ -1946,16 +1934,18 @@ - (IBAction)removeRatings:(id)sender {
 }
 
 - (BOOL)pathSuggesterEmpty {
-	BOOL rval;
+	__block BOOL rval;
 
 	NSFetchRequest *request = [NSFetchRequest fetchRequestWithEntityName:@"SandboxToken"];
 
-	NSError *error = nil;
-	[self.persistentContainerLock lock];
-	NSArray *results = [self.persistentContainer.viewContext executeFetchRequest:request error:&error];
-	if(!results || [results count] < 1) rval = YES;
-	else rval = NO;
-	[self.persistentContainerLock unlock];
+	[self.persistentContainer.viewContext performBlockAndWait:^{
+		NSError *error = nil;
+		NSArray *results = [self.persistentContainer.viewContext executeFetchRequest:request error:&error];
+		if(!results || [results count] < 1)
+			rval = YES;
+		else
+			rval = NO;
+	}];
 
 	return rval;
 }

diff --git a/Playlist/PlaylistEntry.m b/Playlist/PlaylistEntry.m
@@ -16,7 +16,6 @@
 #import "SHA256Digest.h"
 #import "SecondsFormatter.h"
 
-extern NSLock *kPersistentContainerLock;
 extern NSPersistentContainer *kPersistentContainer;
 extern NSMutableDictionary<NSString *, AlbumArtwork *> *kArtworkDictionary;
 
@@ -364,11 +363,9 @@ - (void)setAlbumArtInternal:(NSData *)albumArtInternal {
 	self.artHash = imageCacheTag;
 
 	if(![kArtworkDictionary objectForKey:imageCacheTag]) {
-		[kPersistentContainerLock lock];
 		AlbumArtwork *art = [NSEntityDescription insertNewObjectForEntityForName:@"AlbumArtwork" inManagedObjectContext:kPersistentContainer.viewContext];
 		art.artHash = imageCacheTag;
 		art.artData = albumArtInternal;
-		[kPersistentContainerLock unlock];
 
 		[kArtworkDictionary setObject:art forKey:imageCacheTag];
 	}
@@ -585,30 +582,30 @@ - (PlayCount *)playCountItem {
 
 	NSCompoundPredicate *predicate = [NSCompoundPredicate andPredicateWithSubpredicates:@[albumPredicate, artistPredicate, titlePredicate]];
 
-	NSFetchRequest *request = [NSFetchRequest fetchRequestWithEntityName:@"PlayCount"];
-	request.predicate = predicate;
+	__block PlayCount *item = nil;
 
-	NSError *error = nil;
-	[kPersistentContainerLock lock];
-	NSArray *results = [kPersistentContainer.viewContext executeFetchRequest:request error:&error];
+	[kPersistentContainer.viewContext performBlockAndWait:^{
+		NSFetchRequest *request = [NSFetchRequest fetchRequestWithEntityName:@"PlayCount"];
+		request.predicate = predicate;
 
-	if(!results || [results count] < 1) {
-		NSPredicate *filenamePredicate = [NSPredicate predicateWithFormat:@"filename == %@", self.filenameFragment];
+		NSError *error = nil;
+		NSArray *results = [kPersistentContainer.viewContext executeFetchRequest:request error:&error];
 
-		request = [NSFetchRequest fetchRequestWithEntityName:@"PlayCount"];
-		request.predicate = filenamePredicate;
+		if(!results || [results count] < 1) {
+			NSPredicate *filenamePredicate = [NSPredicate predicateWithFormat:@"filename == %@", self.filenameFragment];
 
-		results = [kPersistentContainer.viewContext executeFetchRequest:request error:&error];
-	}
-	
-	if(results) {
-		results = [results copy];
-	}
-	[kPersistentContainerLock unlock];
+			request = [NSFetchRequest fetchRequestWithEntityName:@"PlayCount"];
+			request.predicate = filenamePredicate;
+
+			results = [kPersistentContainer.viewContext executeFetchRequest:request error:&error];
+		}
+
+		if(!results || [results count] < 1) return;
 
-	if(!results || [results count] < 1) return nil;
+		item = results[0];
+	}];
 
-	return results[0];
+	return item;
 }
 
 @dynamic playCount;