Spring Boot - how to avoid concurrent access to controller

Question

We have a Spring Boot application which is linked to various client on the field. This application has a controller which is called from the clients and interact with the DB and with a physical switch, to turn off or on a light.

The problem comes when two or more clients access an API on the server, because the method checks if the light is on or off (on the DB) to change its status. It occurs that if the light is OFF, and 2 clients call the service at the same time, the first turns on the light and change the status on the db but the second access the light too, the status is OFF on the DB but the first client has already tuned on the light, so the seconds eventually shuts it down thinking to turn it on... Maybe my explanation is a little unclear, the problem is: can I tell spring to access a controller one request at the time?

Thanks to the answer below, we introduced pessimistic lock on the method that toggles the switch, but we continue to have a 200 status from our clients...

We are using spring boot + hibernate

now the controller has the exception for pessimistic lock

  try {

                                String pinName = interruttore.getPinName();
                                // logger.debug("Sono nel nuovo ciclo di
                                // gestione interruttore");
                                if (!interruttore.isStato()) { // solo se
                                                                // l'interruttore
                                                                // è
                                                                // spento

                                    GpioPinDigitalOutput relePin = interruttore.getGpio()
                                            .provisionDigitalOutputPin(RaspiPin.getPinByName(pinName));
                                    interruttoreService.toggleSwitchNew(relePin, interruttore, lit);                                                            // accendo
                                    interruttore.getGpio().unprovisionPin(relePin);
                                }



                        } catch (GpioPinExistsException ge) {
                            logger.error("Gpio già esistente");
                        } catch (PessimisticLockingFailureException pe){
                            logger.error("Pessimistic Lock conflict", pe);
                            return new ResponseEntity<Sensoristica>(sensoristica, HttpStatus.CONFLICT);
                        }

toggleSwitchNew is as follows

@Override
@Transactional(isolation=Isolation.REPEATABLE_READ)
public void toggleSwitchNew(GpioPinDigitalOutput relePin, Interruttore interruttore, boolean on) {
    Date date = new Date();
    interruttore.setDateTime(new Timestamp(date.getTime()));
    interruttore.setStato(on);

    String log = getLogStatus(on) + interruttore.getNomeInterruttore();
    logger.debug(log);
    relePin.high();
    try {
        Thread.sleep(200);
    } catch (InterruptedException e) {
        logger.error("Errore sleep ", e);
    }
    relePin.low();
    updateInterruttore(interruttore);
    illuminazioneService.createIlluminazione(interruttore, on);


}

Then we log the request status code in our clients and they alway get 200 even if they are concurrent

Answer 1

This is a classical locking problem. You can either use pessimistic locking: by allowing only one client at the time to operate on the data (mutual exclusion) or by optimistic locking: by allowing multiple concurrent clients to operate on the data but allowing only the first committer to succeed.

There are many different ways to do that depending on the technology you are using. For example, an alternative way to solve it would be by using the right database isolation level. In your case it seems you need at least "repeatable read" isolation level.

Repeatable read will ensure that if two concurrent transactions read and change the same record more or less concurrently, only one of them will succeed.

In your case you could mark your Spring transaction with the right isolation level.

@Transacational(isolation=REPEATABLE_READ)
public void toggleSwitch() {
    String status = readSwithStatus();
    if(status.equals("on") {
         updateStatus("off");
    } else {
         updateStatus("on");
    }
}

If two concurrent clients try to update the switch status, the first to commit will win, and the second one will always fail. You just have to be prepared to tell the second client its transaction did not succeed due to concurrent failure. This second transaction is automatically rolled back. You or your client may decide to retry it or not.

@Autowire
LightService lightService;

@GET
public ResponseEntity<String> toggleLight(){
   try {
       lightService.toggleSwitch();
       //send a 200 OK
   }catch(OptimisticLockingFailureException e) {
      //send a Http status 409 Conflict!
   }
}

But as I was saying, depending on what you're using (e.g. JPA, Hibernate, plain JDBC), there are multiple ways to do this with either pessimistic or optimistic locking strategies.

Why Not Just Thread Synchronization?

Other answers suggested so far are about pessimistic locking by using Java's mutual exclusion at the thread level using synchronized blocks which might work if you have a single JVM running your code. This strategy might prove to be ineffective if you have more than one JVM running your code or if you eventually scale horizontally and add more JVM nodes behind a load balancer, in whose case thread locking would not solve your problem anymore.

But you could still implement pessimistic locking at the database level, by forcing the process to lock the database record before changing it and by this creating a mutual exclusion zone at the database level.

So, what matters here is understanding the locking principles and then finding a strategy that works for your particular scenario and tech stack. Most likely, in your case, it will involve some form of locking at the database level at some point.

Answer 2

Use synchronized - but if your user clicks quick enough then you will still have issues in that one command will execute immediately after another.

Synchronized will make sure only one thread executes the block in

synchronized(this) { ... } 

at a time.

You may also want to introduce delays and reject commands in quick succession.

try {
    synchronized(this) {
        String pinName = interruttore.getPinName();                     
            if (!interruttore.isStato()) { // switch is off
            GpioPinDigitalOutput relePin = interruttore.getGpio()
                .provisionDigitalOutputPin(RaspiPin.getPinByName(pinName));
            interruttoreService.toggleSwitchNew(relePin, interruttore, lit); // turn it on
            interruttore.getGpio().unprovisionPin(relePin);
       }
   }
} catch (GpioPinExistsException ge) {
    logger.error("Gpio già esistente");
}

Answer 3

Other people's answers seem overly complex to me... Keep it simple.

Instead of toggling make the request have the new value. Inside controller put a synchronized block. Perform action inside the synchronized block only if the new value differs current value.

Object lock = new Object();
Boolean currentValue = Boolean.FALSE;
void ligthsChange(Boolean newValue) {
  synchronized(lock) {
    if (!currentValue.equals(newValue)) {
      doTheSwitch();
      currentValue = newValue;
    }
  }
}