0

I have a class which I want to have only one instance. However, I don't want multiple threads to call getInstance(). So I have coded it in the following way

public class SomeService implements Provider<String, Claims>{

    private SomeService(String a, String b, String c) {
        this.a = a;
        this.b = b;
        this.c = c;
    }



    // single instance
    private static SomeService instance = null;

    private String a;
    private static AtomicInteger initialized = new AtomicInteger();
    private static CountDownLatch latch = new CountDownLatch(1);
    private String b;
    private String c;


    public static SomeService getInstance(String a, String b, String c) {
       if ( initialized.incrementAndGet() == 1) {
           instance = new SomeService(a, b, c);
           latch.countDown();
       } else {
           try {
            latch.await();
        } catch (InterruptedException e) {
            e.printStackTrace();
        } 
       }
       return instance;

    }

    // Other implementation code

    }

My intention (as well as understanding about this) is that:

  1. When a thread calls getInstance it will atomically increment and check whehther it's desired value.

  2. If not, it will wait for the other thread to open the latch which means initialization is in progress.

It would be helpful if someone helps me correct my mistakes. Seems to me that I can probably just do synchronized(someLockObject) {} block but I was wondering if this makes sense at all.

Mark Rotteveel
  • 100,966
  • 191
  • 140
  • 197
ha9u63a7
  • 6,233
  • 16
  • 73
  • 108

2 Answers2

1

My intention (as well as understanding about this) is that:

1) When a thread calls getInstance it will atomically increment and check whehther it's desired value.

Yes.

2) If not, it will wait for the other thread to open the latch which means initialization is in progress.

Yes.

Seems to me that I can probably just do synchronized(someLockObject) {} block

Yes, and that would be more typical.

I was wondering if this makes sense at all.

The code will work as you seem to intend, unless getInstance() is invoked enough times for initialized to wrap back around to 0. I'd be inclined to use an AtomicBoolean and its compareAndSet() method instead, however, as that seems to model the situation better (and it poses no risk whatever of wraparound).

I would be more inclined to use synchonization, however, until and unless that proved to present an unresolvable performance issue. It is clearer and more accessible.

But the approach seems poorly conceived overall, inasmuch as a thread that invokes getInstance() with one set of arguments may receive an instance that was initialized with a different set of parameters. That may be a sign that instead of this variation on the Singleton pattern, you would be better off with the "just use one" pattern. Note in particular that some people consider Singleton an antipattern.

John Bollinger
  • 160,171
  • 8
  • 81
  • 157
0

I guess the design is fundamentally wrong here. what I believe I should be doing is

1) Put more abstraction layer and ensure that a builder pattern is used.

2) When my service needs to serve, it can use the initialization properties to determine what builder to use and then do that job.

3) If possible, I can also combine it with a factory.

Don't know if the above makes sense, but this removes the need for antipattern or "Singleton".

ha9u63a7
  • 6,233
  • 16
  • 73
  • 108
  • On an unrelated note on your deleted question at https://stackoverflow.com/q/56961413/53897 (which you may want to undelete), I would suggest you look at Dagger2 for command line applications. I wrote a HelloWorld program at https://github.com/ravn/dagger2-hello-world. – Thorbjørn Ravn Andersen Jul 10 '19 at 13:19