Singleton property returns different values depending on invocation

Question

Background

In my app, I have class called FavoritesController that manages objects that the user's marked as favorites, and this favorite status is then used throughout the app. The FavoritesController is designed as a singleton class as there are a number of UI elements throughout the app that needs to know the 'favorite status' for objects in different places, also network requests need to be able to signal that a favorite needs to be invalidated if the server says so.

This invalidation part happens when the server responds with a 404 error, indicating that the favorite object must be removed from the user's favorites. The network fetch function throws an error, which triggers the FavoritesController to remove the object and then send a notification to interested parties that they need to refresh.

The problem

When using a unit test to check the quality of the 404 implementation, all methods are triggered as intended – the error is thrown and caught, the FavoritesController deletes the object and sends the notification. In some instances though, the deleted favorite is still there – but it depends on from where the query is done!

If I query inside the singleton the deletion went OK, but if I query from a class that makes use of the singleton, the deletion didn't happen.

Design details

• The FavoritesController property favorites uses an ivar with all accesses @synchronized(), and the value of the ivar is backed by a NSUserDefaults property.
• A favorite object is an NSDictionary with two keys: id and name.

Other info

• One weird thing which I fail to understand why it happens: in some deletion attempts, the name value for the favorite object gets set to "" but the id key retains its value.

• I've written unit tests that add an invalid favorite and checks that it gets removed on first server query. This test passes when starting with empty set of favorites, but fails when there is an instance of 'semi-deleted' object as above (that retains its id value)

• The unit test now consistently passes, but in live usage the failure to delete remains. I suspect that this is due to NSUserDefaults not saving to disk immediately.

Steps I've tried

• Making sure that the singleton implementation is a 'true' singleton, i.e. sharedController always returns the same instance.
• I thought there was some sort of 'capture' problem, where a closure would keep its own copy with outdated favorites, but I think not. When NSLogging the object ID it returns the same.

Code

FavoritesController main methods

- (void) serverCanNotFindFavorite:(NSInteger)siteID {

    NSLog(@"Server can't find favorite");
    NSDictionary * removedFavorite = [NSDictionary dictionaryWithDictionary:[self favoriteWithID:siteID]];
    NSUInteger index = [self indexOfFavoriteWithID:siteID];
    [self debugLogFavorites];

    dispatch_async(dispatch_get_main_queue(), ^{

        [self removeFromFavorites:siteID completion:^(BOOL success) {
            if (success) {
                NSNotification * note = [NSNotification notificationWithName:didRemoveFavoriteNotification object:nil userInfo:@{@"site" : removedFavorite, @"index" : [NSNumber numberWithUnsignedInteger:index]}];
                NSLog(@"Will post notification");
            
                [self debugLogFavorites];
                [self debugLogUserDefaultsFavorites];
                [[NSNotificationCenter defaultCenter] postNotification:note];
                NSLog(@"Posted notification with name: %@", didRemoveFavoriteNotification);
            }
        }];
    });

}

- (void) removeFromFavorites:(NSInteger)siteID completion:(completionBlock) completion {
    if ([self isFavorite:siteID]) {
        NSMutableArray * newFavorites = [NSMutableArray arrayWithArray:self.favorites];
    
        NSIndexSet * indices = [newFavorites indexesOfObjectsPassingTest:^BOOL(NSDictionary * entryUnderTest, NSUInteger idx, BOOL * _Nonnull stop) {
            NSNumber * value = (NSNumber *)[entryUnderTest objectForKey:@"id"];
            if ([value isEqualToNumber:[NSNumber numberWithInteger:siteID]]) {
                return YES;
            }
            return NO;
        }];
    
        __block NSDictionary* objectToRemove = [[newFavorites objectAtIndex:indices.firstIndex] copy];
    
        dispatch_async(dispatch_get_main_queue(), ^{
            NSLog(@"Will remove %@", objectToRemove);
            [newFavorites removeObject:objectToRemove];
            [self setFavorites:[NSArray arrayWithArray:newFavorites]];

            if ([self isFavorite:siteID]) {
                NSLog(@"Failed to remove!");
            
                if (completion) {
                    completion(NO);
                }
            } else {
                NSLog(@"Removed OK");
                
                if (completion) {
                    completion(YES);
                }
            }
        });
    
    } else {
        NSLog(@"Tried removing site %li which is not a favorite", (long)siteID);
        if (completion) {
            completion(NO);
        }
    }
}

- (NSArray *) favorites
{
    @synchronized(self) {
        if (!internalFavorites) {
            static dispatch_once_t onceToken;
            dispatch_once(&onceToken, ^{
                self->internalFavorites = [self.defaults objectForKey:k_key_favorites];
            });
            if (!internalFavorites) {
                internalFavorites = [NSArray array];
            }
        }
        
        return internalFavorites;
    }

}

- (void) setFavorites:(NSArray *)someFavorites {

    @synchronized(self) {
        internalFavorites = someFavorites;
    [self.defaults setObject:internalFavorites forKey:k_key_favorites];
    }


}

- (void) addToFavorites:(NSInteger)siteID withName:(NSString *)siteName {
    if (![self isFavorite:siteID]) {
        NSDictionary * newFavorite = @{
                                       @"name"  : siteName,
                                       @"id"    : [NSNumber numberWithInteger:siteID]
                                   };
        dispatch_async(dispatch_get_main_queue(), ^{
            NSArray * newFavorites = [self.favorites arrayByAddingObject:newFavorite];
            [self setFavorites:newFavorites];

        });
    
        NSLog(@"Added site %@ with id %ld to favorites", siteName, (long)siteID);
    
    } else {
        NSLog(@"Tried adding site as favorite a second time");
    }
}

- (BOOL) isFavorite:(NSInteger)siteID
{
 
    @synchronized(self) {
        
        NSNumber * siteNumber = [NSNumber numberWithInteger:siteID];
        NSArray * favs = [NSArray arrayWithArray:self.favorites];
        if (favs.count == 0) {
            NSLog(@"No favorites");
            return NO;
        }
        
        NSIndexSet * indices = [favs indexesOfObjectsPassingTest:^BOOL(NSDictionary * entryUnderTest, NSUInteger idx, BOOL * _Nonnull stop) {
            if ([[entryUnderTest objectForKey:@"id"] isEqualToNumber:siteNumber]) {
                return YES;
            }
            
            return NO;
        }];
        
        if (indices.count > 0) {
            return YES;
        }
    }
    
    return NO;
}

Singleton implementation of FavoritesController

- (instancetype) init {
    static PKEFavoritesController *initedObject;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        initedObject = [super init];
        self.defaults = [NSUserDefaults standardUserDefaults];
    });
    return initedObject;
}

+ (instancetype) sharedController
{
    return [self new];
}

Unit testing code

func testObsoleteFavoriteRemoval() {
    
    let addToFavorites = self.expectation(description: "addToFavorites")
    let networkRequest = self.expectation(description: "network request")
    
    unowned let favs = PKEFavoritesController.shared()
    favs.clearFavorites()
    
    XCTAssertFalse(favs.isFavorite(313), "Should not be favorite initially")
    
    if !favs.isFavorite(313) {
        NSLog("Adding 313 to favorites")
        favs.add(toFavorites: 313, withName: "Skatås")
    }
    
    let notification = self.expectation(forNotification: NSNotification.Name("didRemoveFavoriteNotification"), object: nil) { (notification) -> Bool in
        NSLog("Received notification: \(notification.name.rawValue)")

        return true
    }
    
    DispatchQueue.main.asyncAfter(deadline: .now() + 1) {
        NSLog("Verifying 313 is favorite")
        XCTAssertTrue(favs.isFavorite(313))
        addToFavorites.fulfill()
    }
    
    self.wait(for: [addToFavorites], timeout: 5)
    
    NSLog("Will trigger removal for 313")
    let _ = SkidsparAPI.fetchRecentReports(forSite: 313, session: SkidsparAPI.session()) { (reports) in
        NSLog("Network request completed")
        networkRequest.fulfill()
    }
    

    self.wait(for: [networkRequest, notification], timeout: 10)

    XCTAssertFalse(favs.isFavorite(313), "Favorite should be removed after a 404 error from server")
    
}

Answer 1

To give context around my answers, this is what the code in question looked like when suggesting the change:

- (NSArray *)favorites {
    @synchronized(internalFavorites) {
        if (!internalFavorites) {
            static dispatch_once_t onceToken;
            dispatch_once(&onceToken, ^{
                internalFavorites = [self.defaults objectForKey:k_key_favorites];
            });
            if (!internalFavorites) {
                internalFavorites = [NSArray array];
            }
        }
    }

    return internalFavorites;
}

I was suspicious of the check if (!internalFavorites) { that followed @synchronized(internalFavorites) because that meant that there was an expectation of @synchronized being passed nil, which results in a noop.

This meant multiple calls to to favorites or setFavorites could happen in funny ways since they wouldn't actually be synchronized. Giving @sychronized an actual object to synchronize on was crucial for thread safety. Synchronizing on self is fine, but for a particular class, you have to be careful not to synchronize too many things on self or you'll be bound to create needless blocking. Providing simple NSObjects to @sychronized is a good way to narrow the scope of what you're protecting.

Here's how you can avoid using self as your lock.

- (instancetype)init {
    static PKEFavoritesController *initedObject;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        initedObject = [super init];
        self.lock = [NSObject new];
        self.defaults = [NSUserDefaults standardUserDefaults];
    });
    return initedObject;
}

+ (instancetype)sharedController {
    return [self new];
}

- (NSArray *)favorites {
    @synchronized(_lock) {
        if (!internalFavorites) {
            static dispatch_once_t onceToken;
            dispatch_once(&onceToken, ^{
                self->internalFavorites = [self.defaults objectForKey:k_key_favorites];
            });
            if (!internalFavorites) {
                internalFavorites = [NSArray array];
            }
        }
    }

    return internalFavorites;
}

Regarding the abnormalities between test runs, definitely calling synchronize on the NSUserDefaults will help because calls to change the defaults are asynchronous, which means other threads are involved. There are like 3 layers of caching as well, and specifically for the purposes of running tests synchronize should ensure that things are completely and cleanly committed before Xcode pulls the plug on the test run. The documentation very abruptly insists that it's not a necessary call, but if it truly weren't necessary it wouldn't exist :-). On my first iOS projects, we always called synchronize after every defaults change... so, I think that documentation is more aspirational on the Apple engineers' parts. I'm glad this intuition helped you.