CODEWITHSUNDEEP
javamultithreadingsingleton
WestFarmer
WestFarmer

Reputation: 765

Is there any thread-safety bugs in such code?

I have a singleton Service implementaion class using Enum in my java web applcaition. It start once at application start and shutdown when applcaition was undeployed. and It provide some service method to client:

public enum SingletonService{
   INSTANCE;
  private boolean isStarted;

  public synchronized void start(){
     if(!isStarted){
         // do initialization stuff
         isStarted = true;
     }
  }

  public void stop(){
      checkStarted();
      // do stop jobs
      isStarted = false;
  }

 private synchronized void checkStarted(){
    if(!isStarted)
       throw new RuntimeException("SingletonService is not ready");
 }

  public void service(){
      checkStarted();
      // do service job
  }

}

Threading is a bit hard for me, I am worrying that I missed tricky bugs in my code. Is that necessary to make start and checkStarted synchronized? Please tell me any bad thing in such code. I also want to know If there is a common pattern for doing such thing in java?

Upvotes: 2

Views: 118

Answers (2)

korolar
korolar

Reputation: 1535

Code is thread-safe when it is guaranteed to behave as expected when used from multiple threads at once. Therefore it's hard to judge whether this particular code is thread-safe not knowing what it's expected behavior is. But I'll try and guess. I guess, that you expected, among others, that if checkStarted() called from INSTANCE.service() does not throw an exception, than the service() method may proceed safe in an assumption that your singleton will not get stopped before service() completes it's execution. With this assumption the answer is: your code is not thread-safe, you should, for example, add synchronize to both stop and service methods. You can then get rid of synchronized on checkStarted, as mentioned by @Guarava Agarwal's answer.

To see this, consider two threads. Thread A calls INSTANCE.start() followed by INSTANCE.stop() and thread B calls INSTANCE.service(). The following sequence of executions is not prohibited by your current synchronization:

  1. Thread A executes INSTANCE.start(). isStarted is set to true.
  2. Thread A enters INSTANCE.stop() and executes INSTANCE.checkStarted() (no exception thrown).
  3. Thread B enters INSTANCE.service() and executes INSTANCE.checkStarted() (again, no exception thrown).
  4. Thread A completes INSTANCE.stop(), setting isStarted to false.
  5. Thread B proceeds with it's service job, with singleton already stopped, against our initial assumption.

Upvotes: 2

Gaurava Agarwal
Gaurava Agarwal

Reputation: 974

Two comments with reasons:

  1. checkStarted is declared private. It's not changing state of service. It don't need to be declared synchronized if point#2 is addressed.

  2. stop is public, it's changing state of service. it should be declared synchronized instead. Two threads accessing stop/start at the same time should access isStarted in synchronized way.

    public enum SingletonService{
 INSTANCE;
private boolean isStarted;

public synchronized void start(){
 if(!isStarted){
     // do initialization stuff
     isStarted = true;
 }
}

public synchronized void stop(){
  checkStarted();
  // do stop jobs
  isStarted = false;
}

 private void checkStarted(){
  if(!isStarted)
      throw new RuntimeException("SingletonService is not ready");
 }

public void service(){
  checkStarted();
   // do service job
}

}

Upvotes: 1

Related Questions