From c9181f571fffc7170b4658164b23b2f49f365e1e Mon Sep 17 00:00:00 2001 From: Christopher Snowhill Date: Tue, 11 Oct 2022 22:58:36 -0700 Subject: [PATCH] Better locking behavior for playlist storage This should fix up potential locking issues with maintaining a copy of the results set while certain other background actions may happen, such as the player updating play counts while playing. Signed-off-by: Christopher Snowhill --- Application/AppController.m | 3 +++ Playlist/PlaylistController.m | 23 +++++++++++++++---- Playlist/PlaylistEntry.m | 10 ++++---- Playlist/PlaylistLoader.m | 19 +++++++++++---- Preferences/Preferences/PathSuggester.m | 3 +++ .../SandboxPathBehaviorController.m | 7 ++++-- Spotlight/SpotlightPlaylistEntry.m | 2 +- Utils/SQLiteStore.m | 7 +++++- Utils/SandboxBroker.m | 23 +++++++++++++++---- 9 files changed, 74 insertions(+), 23 deletions(-) diff --git a/Application/AppController.m b/Application/AppController.m index 447589fa7..7046de862 100644 --- a/Application/AppController.m +++ b/Application/AppController.m @@ -225,6 +225,9 @@ static AppController *kAppController = nil; 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) { diff --git a/Playlist/PlaylistController.m b/Playlist/PlaylistController.m index 8c03edbd2..e54808f00 100644 --- a/Playlist/PlaylistController.m +++ b/Playlist/PlaylistController.m @@ -267,18 +267,20 @@ static void *playlistControllerContext = &playlistControllerContext; PlayCount *pc = pe.playCountItem; if(pc) { + [self.persistentContainerLock lock]; pc.count += 1; pc.lastPlayed = [NSDate date]; + [self.persistentContainerLock unlock]; } else { [self.persistentContainerLock lock]; pc = [NSEntityDescription insertNewObjectForEntityForName:@"PlayCount" inManagedObjectContext:self.persistentContainer.viewContext]; - [self.persistentContainerLock unlock]; 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 commitPersistentStore]; @@ -290,13 +292,13 @@ static void *playlistControllerContext = &playlistControllerContext; if(!pc) { [self.persistentContainerLock lock]; pc = [NSEntityDescription insertNewObjectForEntityForName:@"PlayCount" inManagedObjectContext:self.persistentContainer.viewContext]; - [self.persistentContainerLock unlock]; 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]; } } @@ -307,16 +309,18 @@ static void *playlistControllerContext = &playlistControllerContext; if(!pc) { [self.persistentContainerLock lock]; pc = [NSEntityDescription insertNewObjectForEntityForName:@"PlayCount" inManagedObjectContext:self.persistentContainer.viewContext]; - [self.persistentContainerLock unlock]; 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]; } + [self.persistentContainerLock lock]; pc.rating = rating; + [self.persistentContainerLock unlock]; [self commitPersistentStore]; } @@ -1380,6 +1384,9 @@ static void *playlistControllerContext = &playlistControllerContext; NSError *error = nil; [self.persistentContainerLock lock]; NSArray *results = [self.persistentContainer.viewContext executeFetchRequest:request error:&error]; + if(results) { + results = [results copy]; + } [self.persistentContainerLock unlock]; if(results && [results count] > 0) { @@ -1401,6 +1408,9 @@ static void *playlistControllerContext = &playlistControllerContext; NSError *error = nil; [self.persistentContainerLock lock]; NSArray *results = [self.persistentContainer.viewContext executeFetchRequest:request error:&error]; + if(results) { + results = [results copy]; + } [self.persistentContainerLock unlock]; if(results && [results count] > 0) { @@ -1937,15 +1947,18 @@ static inline void dispatch_sync_reentrant(dispatch_queue_t queue, dispatch_bloc } - (BOOL)pathSuggesterEmpty { + 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]; - if(!results || [results count] < 1) return YES; - else return NO; + return rval; } @end diff --git a/Playlist/PlaylistEntry.m b/Playlist/PlaylistEntry.m index 492524f98..3199bb3c5 100644 --- a/Playlist/PlaylistEntry.m +++ b/Playlist/PlaylistEntry.m @@ -366,9 +366,9 @@ extern NSMutableDictionary *kArtworkDictionary; if(![kArtworkDictionary objectForKey:imageCacheTag]) { [kPersistentContainerLock lock]; AlbumArtwork *art = [NSEntityDescription insertNewObjectForEntityForName:@"AlbumArtwork" inManagedObjectContext:kPersistentContainer.viewContext]; - [kPersistentContainerLock unlock]; art.artHash = imageCacheTag; art.artData = albumArtInternal; + [kPersistentContainerLock unlock]; [kArtworkDictionary setObject:art forKey:imageCacheTag]; } @@ -591,7 +591,6 @@ NSURL *_Nullable urlForPath(NSString *_Nullable path) { NSError *error = nil; [kPersistentContainerLock lock]; NSArray *results = [kPersistentContainer.viewContext executeFetchRequest:request error:&error]; - [kPersistentContainerLock unlock]; if(!results || [results count] < 1) { NSPredicate *filenamePredicate = [NSPredicate predicateWithFormat:@"filename == %@", self.filenameFragment]; @@ -599,10 +598,13 @@ NSURL *_Nullable urlForPath(NSString *_Nullable path) { request = [NSFetchRequest fetchRequestWithEntityName:@"PlayCount"]; request.predicate = filenamePredicate; - [kPersistentContainerLock lock]; results = [kPersistentContainer.viewContext executeFetchRequest:request error:&error]; - [kPersistentContainerLock unlock]; } + + if(results) { + results = [results copy]; + } + [kPersistentContainerLock unlock]; if(!results || [results count] < 1) return nil; diff --git a/Playlist/PlaylistLoader.m b/Playlist/PlaylistLoader.m index 9bfda679e..811e24532 100644 --- a/Playlist/PlaylistLoader.m +++ b/Playlist/PlaylistLoader.m @@ -569,11 +569,11 @@ static inline void dispatch_sync_reentrant(dispatch_queue_t queue, dispatch_bloc dispatch_sync_reentrant(dispatch_get_main_queue(), ^{ [self->playlistController.persistentContainerLock lock]; pe = [NSEntityDescription insertNewObjectForEntityForName:@"PlaylistEntry" inManagedObjectContext:self->playlistController.persistentContainer.viewContext]; - [self->playlistController.persistentContainerLock unlock]; pe.url = url; pe.index = index + i; pe.rawTitle = [[url path] lastPathComponent]; pe.queuePosition = -1; + [self->playlistController.persistentContainerLock unlock]; }); [entries addObject:pe]; @@ -593,10 +593,10 @@ static inline void dispatch_sync_reentrant(dispatch_queue_t queue, dispatch_bloc dispatch_sync_reentrant(dispatch_get_main_queue(), ^{ [self->playlistController.persistentContainerLock lock]; pe = [NSEntityDescription insertNewObjectForEntityForName:@"PlaylistEntry" inManagedObjectContext:self->playlistController.persistentContainer.viewContext]; - [self->playlistController.persistentContainerLock unlock]; [pe setValuesForKeysWithDictionary:entry]; pe.index = index + i; pe.queuePosition = -1; + [self->playlistController.persistentContainerLock unlock]; }); [entries addObject:pe]; @@ -797,6 +797,9 @@ NSURL *_Nullable urlForPath(NSString *_Nullable path); NSError *error; [playlistController.persistentContainerLock lock]; NSArray *results = [moc executeFetchRequest:request error:&error]; + if(results) { + results = [results copy]; + } [playlistController.persistentContainerLock unlock]; if(results && [results count] > 0) { @@ -926,8 +929,8 @@ NSURL *_Nullable urlForPath(NSString *_Nullable path); NSError *error = nil; [playlistController.persistentContainerLock lock]; NSArray *results = [moc executeFetchRequest:request error:&error]; - [playlistController.persistentContainerLock unlock]; if(!results) { + [playlistController.persistentContainerLock unlock]; ALog(@"Error fetching AlbumArtwork objects: %@\n%@", [error localizedDescription], [error userInfo]); abort(); } @@ -935,6 +938,7 @@ NSURL *_Nullable urlForPath(NSString *_Nullable path); for(AlbumArtwork *art in results) { [kArtworkDictionary setObject:art forKey:art.artHash]; } + [playlistController.persistentContainerLock unlock]; request = [NSFetchRequest fetchRequestWithEntityName:@"PlaylistEntry"]; @@ -944,15 +948,20 @@ NSURL *_Nullable urlForPath(NSString *_Nullable path); [playlistController.persistentContainerLock lock]; results = [moc executeFetchRequest:request error:&error]; - [playlistController.persistentContainerLock unlock]; if(!results) { + [playlistController.persistentContainerLock unlock]; ALog(@"Error fetching PlaylistEntry objects: %@\n%@", [error localizedDescription], [error userInfo]); abort(); } - if([results count] == 0) return NO; + if([results count] == 0) { + [playlistController.persistentContainerLock unlock]; + return NO; + } NSMutableArray *resultsCopy = [results mutableCopy]; + [playlistController.persistentContainerLock unlock]; + NSMutableIndexSet *pruneSet = [[NSMutableIndexSet alloc] init]; NSUInteger index = 0; for(PlaylistEntry *pe in resultsCopy) { diff --git a/Preferences/Preferences/PathSuggester.m b/Preferences/Preferences/PathSuggester.m index 2673990a3..128d20cec 100644 --- a/Preferences/Preferences/PathSuggester.m +++ b/Preferences/Preferences/PathSuggester.m @@ -69,6 +69,9 @@ NSError *error = nil; [lock lock]; NSArray *results = [pc.viewContext executeFetchRequest:request error:&error]; + if(results) { + results = [results copy]; + } [lock unlock]; if(!results || [results count] < 1) return; diff --git a/Preferences/Preferences/SandboxPathBehaviorController.m b/Preferences/Preferences/SandboxPathBehaviorController.m index 2f8f41e00..d9310f151 100644 --- a/Preferences/Preferences/SandboxPathBehaviorController.m +++ b/Preferences/Preferences/SandboxPathBehaviorController.m @@ -44,6 +44,9 @@ NSError *error = nil; [lock lock]; NSArray *results = [pc.viewContext executeFetchRequest:request error:&error]; + if(results) { + results = [results copy]; + } [lock unlock]; if(results && [results count] > 0) { @@ -69,12 +72,10 @@ [lock lock]; SandboxToken *token = [NSEntityDescription insertNewObjectForEntityForName:@"SandboxToken" inManagedObjectContext:pc.viewContext]; - [lock unlock]; if(token) { token.path = [url path]; token.bookmark = bookmark; - [lock lock]; [pc.viewContext save:&err]; [lock unlock]; if(err) { @@ -84,6 +85,8 @@ [NSClassFromString(@"SandboxBroker") cleanupFolderAccess]; [self refresh]; } + } else { + [lock unlock]; } } diff --git a/Spotlight/SpotlightPlaylistEntry.m b/Spotlight/SpotlightPlaylistEntry.m index c6cd2a703..33f63d8e1 100644 --- a/Spotlight/SpotlightPlaylistEntry.m +++ b/Spotlight/SpotlightPlaylistEntry.m @@ -40,7 +40,6 @@ extern NSPersistentContainer *kPersistentContainer; + (PlaylistEntry *)playlistEntryWithMetadataItem:(NSMetadataItem *)metadataItem { [kPersistentContainerLock lock]; PlaylistEntry *entry = [NSEntityDescription insertNewObjectForEntityForName:@"PlaylistEntry" inManagedObjectContext:kPersistentContainer.viewContext]; - [kPersistentContainerLock unlock]; entry.deLeted = YES; @@ -85,6 +84,7 @@ extern NSPersistentContainer *kPersistentContainer; entry.url = url; [entry setMetadata:dict]; + [kPersistentContainerLock unlock]; return entry; } diff --git a/Utils/SQLiteStore.m b/Utils/SQLiteStore.m index c92bb394d..bc7c2ff3c 100644 --- a/Utils/SQLiteStore.m +++ b/Utils/SQLiteStore.m @@ -1446,12 +1446,12 @@ static SQLiteStore *g_sharedStore = nil; - (PlaylistEntry *_Nonnull)getTrack:(int64_t)trackId { [kPersistentContainerLock lock]; PlaylistEntry *entry = [NSEntityDescription insertNewObjectForEntityForName:@"PlaylistEntry" inManagedObjectContext:kPersistentContainer.viewContext]; - [kPersistentContainerLock unlock]; if(trackId < 0) { entry.error = YES; entry.errorMessage = NSLocalizedString(@"ErrorInvalidTrackId", @""); entry.deLeted = YES; + [kPersistentContainerLock unlock]; return entry; } @@ -1462,6 +1462,7 @@ static SQLiteStore *g_sharedStore = nil; entry.error = YES; entry.errorMessage = NSLocalizedString(@"ErrorSqliteProblem", @""); entry.deLeted = YES; + [kPersistentContainerLock unlock]; return entry; } @@ -1472,6 +1473,7 @@ static SQLiteStore *g_sharedStore = nil; entry.error = YES; entry.errorMessage = NSLocalizedString(@"ErrorSqliteProblem", @""); entry.deLeted = YES; + [kPersistentContainerLock unlock]; return entry; } @@ -1547,6 +1549,7 @@ static SQLiteStore *g_sharedStore = nil; entry.errorMessage = NSLocalizedString(@"ErrorTrackMissing", @""); entry.deLeted = YES; } + [kPersistentContainerLock unlock]; sqlite3_reset(st); @@ -1863,8 +1866,10 @@ static SQLiteStore *g_sharedStore = nil; int64_t entryId = sqlite3_column_int64(st, select_playlist_out_entry_id); entry = [self getTrack:trackId]; if(!entry.deLeted && !entry.error) { + [kPersistentContainerLock lock]; entry.index = index; entry.entryId = entryId; + [kPersistentContainerLock unlock]; } else { [kPersistentContainerLock lock]; [kPersistentContainer.viewContext deleteObject:entry]; diff --git a/Utils/SandboxBroker.m b/Utils/SandboxBroker.m index 27659104f..4a60c89ad 100644 --- a/Utils/SandboxBroker.m +++ b/Utils/SandboxBroker.m @@ -166,6 +166,9 @@ static SandboxBroker *kSharedSandboxBroker = nil; NSError *error = nil; [lock lock]; NSArray *results = [pc.viewContext executeFetchRequest:request error:&error]; + if(results) { + results = [results copy]; + } [lock unlock]; if(results && [results count] > 0) { ret = [[SandboxEntry alloc] initWithToken:results[0]]; @@ -183,6 +186,9 @@ static SandboxBroker *kSharedSandboxBroker = nil; error = nil; [lock lock]; results = [pc.viewContext executeFetchRequest:request error:&error]; + if(results) { + results = [results copy]; + } [lock unlock]; if(results && [results count] > 0) { @@ -253,12 +259,14 @@ static inline void dispatch_sync_reentrant(dispatch_queue_t queue, dispatch_bloc [lock lock]; SandboxToken *token = [NSEntityDescription insertNewObjectForEntityForName:@"SandboxToken" inManagedObjectContext:pc.viewContext]; - [lock unlock]; if(token) { token.path = [folderUrl path]; token.bookmark = bookmark; + [lock unlock]; [SandboxBroker cleanupFolderAccess]; + } else { + [lock unlock]; } }); } @@ -301,13 +309,13 @@ static inline void dispatch_sync_reentrant(dispatch_queue_t queue, dispatch_bloc dispatch_sync_reentrant(dispatch_get_main_queue(), ^{ [lock lock]; SandboxToken *token = [NSEntityDescription insertNewObjectForEntityForName:@"SandboxToken" inManagedObjectContext:pc.viewContext]; - [lock unlock]; if(token) { token.path = [url path]; token.bookmark = bookmark; token.folder = NO; } + [lock unlock]; }); } } @@ -369,12 +377,14 @@ static inline void dispatch_sync_reentrant(dispatch_queue_t queue, dispatch_bloc [lock lock]; SandboxToken *token = [NSEntityDescription insertNewObjectForEntityForName:@"SandboxToken" inManagedObjectContext:pc.viewContext]; - [lock unlock]; if(token) { token.path = [folderUrl path]; token.bookmark = bookmark; + [lock unlock]; [SandboxBroker cleanupFolderAccess]; + } else { + [lock unlock]; } } }); @@ -391,14 +401,17 @@ static inline void dispatch_sync_reentrant(dispatch_queue_t queue, dispatch_bloc request.sortDescriptors = @[sortDescriptor]; NSError *error = nil; + NSMutableArray *resultsCopy = nil; [lock lock]; NSArray *results = [pc.viewContext executeFetchRequest:request error:&error]; + if(results) { + resultsCopy = [results mutableCopy]; + } [lock unlock]; BOOL isUpdated = NO; - if(results && [results count]) { - NSMutableArray *resultsCopy = [results mutableCopy]; + if(resultsCopy && [resultsCopy count]) { for(NSUInteger i = 0; i < [resultsCopy count] - 1; ++i) { SandboxToken *token = resultsCopy[i]; NSURL *url = [NSURL fileURLWithPath:token.path];