Or: You Could Be Forgiven For Assuming That This Is A Singleton

Screenshot of initializer code
Smokey, my friend, if you don't use dependency injection, you are entering a world of pain.

I was recently asked to debug a very mysterious crash in a customer’s code. It was a nasty bug – hard to reproduce, and with a very confusing stack trace.

Note: XYZ is the app where the crash is occurring. FRMWK is this nifty framework that they use in a bunch of their apps.

Last Exception Backtrace:
0 CoreFoundation 0x187d96084 __exceptionPreprocess + 132
1 libobjc.A.dylib 0x1983cc0e4 objc_exception_throw + 60
2 CoreFoundation 0x187d95fc4 +[NSException raise:format:] + 128
3 Foundation 0x188be6900 NSKVODeallocate + 340
4 XYZApp 0x10007d998 __60-[XYZAppDelegate application:didFinishLaunchingWithOptions:]_block_invoke_2 (in XYZApp) (XYZAppDelegate.m:103) + 170392
5 XYZApp 0x10072cc30 __59-[FRMWKConfigURLProvider getSelectedConfigURLWithCompletion:]_block_invoke (in XYZApp) (FRMWKConfigURLProvider.m:113) + 7179312
6 XYZApp 0x10072cf70 __45-[FRMWKConfigURLProvider fetchAvailableServers]_block_invoke (in XYZApp) (FRMWKConfigURLProvider.m:139) + 7180144
7 XYZApp 0x1007c8434 __66-[FRMWKTrafficControlService completionBlockWithATCCompletionBlock:]_block_invoke (in XYZApp) (FRMWKTrafficControlService.m:214) + 7816244
8 libdispatch.dylib 0x198a1145c _dispatch_client_callout + 16
9 libdispatch.dylib 0x198a1e1fc _dispatch_barrier_sync_f_slow_invoke + 488
10 libdispatch.dylib 0x198a1145c _dispatch_client_callout + 16
11 libdispatch.dylib 0x198a15a70 _dispatch_main_queue_callback_4CF + 932
12 CoreFoundation 0x187d4d8dc __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12
13 CoreFoundation 0x187d4b984 __CFRunLoopRun + 1492
14 CoreFoundation 0x187c79664 CFRunLoopRunSpecific + 396
15 GraphicsServices 0x190db35a4 GSEventRunModal + 168
16 UIKit 0x18c57e984 UIApplicationMain + 1488
17 XYZApp 0x1000707b4 main (in XYZApp) (main.m:24) + 116660
18 libdyld.dylib 0x198a3aa08 start + 4

Ugh, a dreaded KVO exception. Guaranteed to never give you useful information. So what exactly is going in in XYZAppDelegate at line 103?

[FRMWKConfigManager setSharedManager:[[XYZConfigManager alloc] init]];

Erm, wait. Isn’t FRMWKConfigManager a singleton? Why is it being set to something? Well it turns out that FRMWKConfigManager is not really a singleton. It almost certainly started out life as one, but at some point an app needed to be able to replace it with a customized subclass. So someone just edited FRMWKConfigManager to add a setter for the shared instance. And if that setter was never called, it would just lazily create the shared instance as normal.

At this point, there was no indication why setting this shared instance might cause a crash. But because of the code smell of the quasi-singleton, I was 99% certain that this unusual architectural choice was the root of the problem.

And indeed, this was the case. Digging deeper, I found that FRMWKConfigManager, in its init method, was KVOing itself (ಠ_ಠ) to know when one of its properties was changed. But it never removed the observation in its dealloc method. And you could forgive the original author for that, since they thought they were writing a singleton which would never be deallocated.

But still, why was this crashing? No instances of FRMWKConfigManager should ever be created, because that line in the app delegate was replacing it with an instance of the subclass. Well it turned out to be a timing issue. Every once in a while, some service call would complete faster than usual and access that singleton before it was replaced. This caused a default instance of the normal singleton class to be initialized. Then, a fraction of a second later, it would be replaced by the subclass instance. This caused the singleton to be deallocated, and the KVO crash to be triggered.

How To Fix This Problem

First of all, just don’t use KVO. Seriously, it’s a terrible API that should only be used when absolutely necessary. Not in a situation like this where there are much better options available.

Also, maybe just don’t use singletons? As I mentioned in this other article, singletons are considered by many to be an anti-pattern.

However, in this case the problem was enmeshed within a framework that I was not in a position to make major architectural changes to. So in actuality, the crash itself was fixed by having a member of the framework team remove the KVO observation in the singleton’s dealloc method. And that did fix the crash, but who knows if there is still a lingering problem with certain parts of the code occasionally getting the wrong “singleton”.

There are some better ways to fix the underlying architectural issues. I’ve broken them up by the magnitude of the change.

Minimally Intrusive Change

Probably the least disruptive change you could make and still end up with a better architecture, is to just make that class a real singleton again. This would ensure that nobody ever got the wrong version of it. But how can you do that, and still make it “dynamic”? That is, allow a framework consumer to use it’s own subclass of that singleton.

I would recommend taking a page from Java’s playbook, and using a config file. Maybe just a plist where you can set the name of the class you want to use for this singleton. And then the sharedManager method could look something like this:

+ (instancetype)sharedManager
{
    static dispatch_once_t once;
    static id sharedManager;

    dispatch_once(&once, ^{
        NSString* filePath = [[NSBundle mainBundle] pathForResource: @"config" ofType: @"plist"];
        NSDictionary* configDict = [NSDictionary dictionaryWithContentsOfFile:filePath];
        NSString* className = configDict[@"ConfigManagerClass"];
        Class managerClass = NSClassFromString(className);

        if (!managerClass) {
            managerClass = self;
        }

        sharedManager = [[managerClass alloc] init];
    });

    return sharedManager;
}

You are still using a singleton, but at least it’s a real singleton. And obviously, if you were going to be configuring a lot of classes this way, it would probably make sense to abstract all that config file loading into it’s own class.

Really Doing This The Right Way

Let’s say you really want to do this properly. You don’t want to have those undeclared dependencies on the singleton class. And you are willing to make the significant architectural change this requires (or are lucky enough to be making this choice while designing the app to begin with). What’s the best way to do this?

I’m a big fan of dependency injection. Now I’m not talking about some fancy framework that magically fills in your dependencies for you. I’m just talking about good old “constructor injection”. If you’re not familiar with that term, it really just means designing your code so that no object ever “reaches out” to grab something that it needs (like this singleton). Instead, every thing that it depends on is passed into its constructor (or initializer, as we would say in Obj-C).

What changes would that require in this app? Well first, you would instantiate your singleton subclass somewhere, probably in the app delegate. Then you would need to take every class that is currently calling the sharedManager method, and change its initializer to accept the singleton class as a parameter.

This sort of dependency injection has many advantages. The two most frequently cited are:

  1. It makes it obvious what the dependencies of every class are. You don’t have to dig into the implementation of a class to find out what services it is using.
  2. It makes testing much easier. It’s trivial to pass in a mock version of whatever services the class depends on.

Some perceived disadvantages:

  1. It makes my initializers sooooo long with all those parameters. This is actually a hint that maybe your class is doing too many different things, and should be broken up.
  2. I have to add that parameter to this whole long chain of objects, just to get it to the one deeply nested object that really needs it. This might also be a code smell indicating a problem with your design. But sometimes, this does just happen. And you might be best off using a globally accessible object, like a singleton or a service locator.

Sometimes the pragmatic thing is to make the smallest change possible, to avoid introducing additional bugs. But it is satisfying to fix things the right way.