Is it really a Singleton?
Recently, I have investigated a defect that is interesting to share. As part of our operations, we need to be able to run tools, which will invoke APIs on different AWS services. For example, one common tool is to run a certain CloudWatch query and analyze the results. The framework, which is running the tools, is designed to be flexible enough so any developer can easily add new tools, and on the other hand it provides a standardization and enforces certain safety rules.
In order to run a tool, the operator needs to provide the tool name, its unique input arguments, and choose the region where they want to run it. The framework will create a new injector, initiating all the required SDKs in the chosen region, create an instance of the requested tool, and invoke it, passing to it the input arguments from the command line.
Without going into too much details, you would expect seeing something like this:
1 2 3 4 5 6 7 8 | void createInjectorAndRun(final String region, final String[] args) { // Create a new Guice injector, and install the Amazon SDK module final Injector injector = createInjector(new AmazonSDKModule(region)); // Create a new instance of the requested tool, and run it final ToolName tool = injector.getInstance(ToolName.class); tool.run(args); } |
A sample AmazonSDKModule will look like this:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 | public class AmazonSDKModule extends AbstractModule { private String region; public AmazonSDKModule(final String region) { this.region = region; } @Override protected void configure() { } @Provides @Singleton public AWSCredentialsProvider getCredentials() { return DefaultAWSCredentialsProviderChain.getInstance(); } @Provides @Singleton public AWSLogs getAmazonCloudWatch(final AWSCredentialsProvider credentials, final String region) { return AWSLogsClientBuilder.standard() .withCredentials(credentials) .withRegion(region) .build(); } } |
Now, let’s imagine we want to add a new option to run the tool in multiple regions, at the same time. The simplest thing, which will maximize the code re-use would be to create a new thread per region, with a dedicated injector, which is initiated in the right region, and run a different instance of the tool each time. That would look like this:
1 2 3 4 5 6 7 8 9 10 | void runInMultipleRegions(final Set<string> regions, final String[] args, final long timeoutInMinutes) { final ExecutorService threadPool = Executors.newFixedThreadPool(regions.size()); regions.forEach(region -> threadPool.execute(() -> createInjectorAndRun(region, args))); threadPool.shutdown(); threadPool.awaitTermination(timeoutInMinutes, TimeUnit.MINUTES); } |
Wait. How come does it work if the CloudWatch client is annotated with @Singleton ? The answer is simple. As long as you are using one injector, you will have one instance of the CloudWatch client. Once you have different injectors, each one of them will have its own instance of the CloudWatch client, initiated with the provided region.
Everything was working as expected, when one day I tried running a tool in multiple regions, and I got some weird results. after further analysis, I discovered that all of them came from the same region. I re-reviewed the code and it looked similar to the example above, with one change – this specific tool was using a CloudWatch wrapper that added some extra functionality. Opening it revealed the following:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 | @Log4j2 public class CloudWatchClientHelper { private final AWSLogs client; private CloudWatchClientHelper(final AWSLogs client) { this.client = client; } private static CloudWatchClientHelper instance; public static CloudWatchClientHelper getInstance(final AWSLogs client) { if (instance == null) { instance = new CloudWatchClientHelper(client); } return instance; } // More code here. } |
And the AmazonSDKModule had another method:
1 2 3 4 5 | @Provides @Singleton public CloudWatchClientHelper getCloudWatchClientHelper(final AWSLogs client) { return CloudWatchClientHelper.getInstance(client); } |
Whoops. What happened here is that the CloudWatchClientHelper class was created and used in a way that made him a real singleton in the scope of the application, and not the injector. Meaning, once the first thread created the injector, the CloudWatchClientHelper.instance static variable was initiated with the CloudWatch client in that region, and all other threads were just re-using the same instance in the same region.
Another thing which you should notice is that although the static instance creation is not thread safe, and in some race conditions during the initialization stage, may lead to null pointer exceptions. This can be simply fixed by using a locking mechanism, like the “synchronized” keyword.
My advice, when you are thinking of implementing a real Singleton in your code, please re-think this decision. Singletons can lead to unexpected behaviour (as shown in this example) and are nightmare to use mock and unit test. It is ok to annotate classes with the @Singleton annotation when using injection, and it can also save you some CPU and GC work, but always make sure that these classes are stateless.
– Alexander
1 thought on “Is it really a Singleton?”
nice