3

I recently stumbled upon piece of code similar to the one below. This code does reek right off. Looks like singleton but its not because there is no private constructor. I know for sure this is going to have thread safety issue given big enough load to it. Specially given class instance. Can someone please point out thread safety issues with this code?

public class AClass extends AnotherClass {

  public static final AClass instance = new AClass();

  public static SomeObject doSomethingThatCallsAService(Params params) {
       return methodThatCallsService(params, instance);
  }

  public static SomeObject methodThatCallsService(Params params, AClass instance) {
      -----call service here ---------
      instance.doSomethingElse();
  }

  private void doSomethingElse() {
      --- do some trivial work -----
  }
}
ringadingding
  • 475
  • 5
  • 14
  • 3
    The object carries no state. Where does the thread-safety concern come from? – ernest_k Jul 13 '18 at 14:06
  • Thanks Ernest, the concern is that what happens when multiple calls are being made to static method that is accessing local instance of the class that is declared as static. – ringadingding Jul 13 '18 at 14:10

1 Answers1

3

Given that the object does not carry state, there's no concern for thread safety, regardless of the number of threads calling methods on or having a reference to the singleton object.

All the methods in the class, including static ones, don't use any shared data. So whether they call methods on the singleton object or they pass the instance around, there's no need for access to anything to be synchronized.

As the code is, the only data that could possibly need synchronization is params in the argument to methodThatCallsService, and that's only if this method modifies the data and multiple threads hold a reference to the same Params object.

But as far as this class is concerned, it's thread-safe, even if the singleton implementation is vulnerable.

ernest_k
  • 44,416
  • 5
  • 53
  • 99
  • 3
    Just in case, if singleton has some mutating variables then his concerns is correct but looking at sample code, your answers is perfect. – Rahul B Jul 13 '18 at 14:27
  • 1
    If `params` variable is read and modified then there might be issues. Since we do not see any usages of `params` we could only guess – Ivan Jul 13 '18 at 14:47