Рефакторинг BookingService для корректного бронирования

9. Исправить BookingService для корректного конкурентного бронирования

Условие задачи:
Дан Spring-сервис бронирования гостиничных номеров.

У номера есть два состояния:

VACANT — свободен;
BOOKED — забронирован.

Метод bookRoom() должен забронировать номер за текущим клиентом только в том случае, если номер ещё свободен.

Необходимо провести code review и объяснить, почему реализация может успешно проходить тесты, но некорректно работать в production при нескольких одновременных запросах.

Код:

@Service
class BookingService {

    @Autowired
    private RoomRepository roomRepository;

    public boolean bookRoom(Integer roomId) {
        boolean roomBooked = false;

        Room room = roomRepository.findById(roomId);

        if (room.getStatus() == "VACANT") {
            room.setStatus("BOOKED");
            room.setClientId(SecurityContext.getClientId());

            roomRepository.save(room);
            roomBooked = true;
        }

        return roomBooked;
    }
}

@Entity
class Room {

    @Id
    private Integer id;

    private String status;
    private String clientId;
    private String roomNumber;
}

public interface RoomRepository
        extends JpaRepository<Room, Integer> {
}

Спойлеры к решению

Подсказки

💡 Главная проблема проявится, когда два запроса одновременно попытаются забронировать один номер.
💡 Простого @Transactional недостаточно: две транзакции всё равно могут одновременно прочитать VACANT.
💡 synchronized также не решает задачу в нескольких экземплярах приложения.
💡 Самый простой вариант — выполнить атомарный UPDATE ... WHERE status = VACANT и проверить количество изменённых строк.
💡 Статус лучше представить через enum, а не через String.

Решение

Основная проблема — race condition.

Представим два одновременных запроса:

Клиент A                     Клиент B
   │                            │
   ├─ читает Room              ├─ читает Room
   │  status = VACANT          │  status = VACANT
   │                            │
   ├─ status = BOOKED          ├─ status = BOOKED
   ├─ clientId = A             ├─ clientId = B
   │                            │
   └─ save()                   └─ save()

Оба запроса увидели свободный номер и оба считают, что успешно его забронировали.

В результате последний UPDATE перезапишет clientId первого клиента.

Именно поэтому проблема может почти не проявляться на тестовом стенде с небольшой нагрузкой, но регулярно возникать в production.

Оптимальный для данного случая вариант — выполнить проверку статуса и изменение записи одной атомарной SQL-операцией.

Сначала лучше заменить строковый статус на enum:

public enum RoomStatus {
    VACANT,
    BOOKED
}

Сущность:

@Entity
public class Room {

    @Id
    private Integer id;

    @Enumerated(EnumType.STRING)
    private RoomStatus status;

    private String clientId;
    private String roomNumber;
}

Репозиторий:

public interface RoomRepository
        extends JpaRepository<Room, Integer> {

    @Modifying
    @Query("""
            update Room r
               set r.status = :booked,
                   r.clientId = :clientId
             where r.id = :roomId
               and r.status = :vacant
            """)
    int bookIfVacant(
            @Param("roomId") Integer roomId,
            @Param("clientId") String clientId,
            @Param("vacant") RoomStatus vacant,
            @Param("booked") RoomStatus booked
    );
}

Сервис:

@Service
public class BookingService {

    private final RoomRepository roomRepository;

    public BookingService(RoomRepository roomRepository) {
        this.roomRepository = roomRepository;
    }

    @Transactional
    public boolean bookRoom(Integer roomId) {
        Objects.requireNonNull(roomId, "roomId");

        String clientId = SecurityContext.getClientId();

        if (clientId == null || clientId.isBlank()) {
            throw new IllegalStateException(
                    "Не удалось определить клиента"
            );
        }

        int updated = roomRepository.bookIfVacant(
                roomId,
                clientId,
                RoomStatus.VACANT,
                RoomStatus.BOOKED
        );

        return updated == 1;
    }
}

Ключевая операция фактически сводится к:

UPDATE room
SET status = 'BOOKED',
    client_id = :clientId
WHERE id = :roomId
  AND status = 'VACANT';

Если два запроса одновременно пытаются забронировать один номер, успешно изменить строку сможет только один из них.

Результат будет примерно таким:

Клиент A:
UPDATE ... WHERE status = VACANT
→ updated = 1
→ true

Клиент B:
UPDATE ... WHERE status = VACANT
→ updated = 0
→ false

То есть проверка:

свободен ли номер?

и действие:

забронировать номер

больше не являются двумя отдельными операциями. Они выполняются атомарно на стороне БД.

Это особенно важно, если приложение запущено в нескольких pod’ах:

pod-1 ─┐
pod-2 ─┤
pod-3 ─┼── PostgreSQL
...    │
pod-N ─┘

Использовать:

synchronized

для решения этой проблемы недостаточно. Он синхронизирует потоки только внутри одного JVM-процесса и никак не защищает от параллельного запроса из другого pod’а.

Есть и другие проблемы в исходном коде.

JpaRepository.findById() возвращает:

Optional<Room>

поэтому такой код некорректен:

Room room = roomRepository.findById(roomId);

При обычном чтении требовалось бы:

Room room = roomRepository.findById(roomId)
        .orElseThrow(() ->
                new EntityNotFoundException(
                        "Room not found: " + roomId
                )
        );

Но в предложенном варианте предварительное чтение вообще не требуется.

Также нельзя сравнивать строки так:

room.getStatus() == "VACANT"

== сравнивает ссылки на объекты, а не содержимое строк.

Можно было бы написать:

"VACANT".equals(room.getStatus())

но для конечного набора состояний лучше использовать enum.

Простое добавление:

@Transactional
public boolean bookRoom(...)

не решает гонку само по себе.

При стандартном уровне изоляции две транзакции могут выполнить:

T1: SELECT → VACANT
T2: SELECT → VACANT

и затем обе попытаться изменить запись.

Транзакция обеспечивает атомарность набора операций, но не превращает последовательность SELECT → проверка → UPDATE автоматически в взаимное исключение.

Альтернативный вариант — пессимистическая блокировка:

@Lock(LockModeType.PESSIMISTIC_WRITE)
@Query("select r from Room r where r.id = :id")
Optional<Room> findByIdForUpdate(
        @Param("id") Integer id
);

и затем:

@Transactional
public boolean bookRoom(Integer roomId) {
    Room room = roomRepository.findByIdForUpdate(roomId)
            .orElseThrow();

    if (room.getStatus() != RoomStatus.VACANT) {
        return false;
    }

    room.setStatus(RoomStatus.BOOKED);
    room.setClientId(SecurityContext.getClientId());

    return true;
}

Вторая транзакция будет ждать освобождения блокировки первой.

Такой подход корректен, но для простой операции «забронировать, только если свободно» условный UPDATE обычно проще и требует меньше работы:

SELECT + LOCK + UPDATE

против:

UPDATE ... WHERE status = VACANT

Ещё один вариант — optimistic locking через:

@Version
private Long version;

Тогда при конкурентном изменении одна из транзакций получит OptimisticLockException.

Это удобно для более сложной бизнес-логики, но для конкретной задачи атомарный conditional update является наиболее прямым решением.

Основные проблемы исходного кода:

возможна двойная бронь из-за race condition;
@Transactional без блокировки не устраняет гонку;
synchronized не работает между pod’ами;
findById() возвращает Optional;
строки сравниваются через ==;
статус лучше хранить как enum;
используется field injection;
выполняются лишние SELECT и save(), хотя задачу можно решить одним UPDATE.

Для данного сценария предпочтительная схема:

bookRoom()
    ↓
UPDATE room
WHERE id = ?
  AND status = VACANT
    ↓
updated == 1 → бронь успешна
updated == 0 → номер уже занят или отсутствует

Она корректно работает и при нескольких потоках, и при нескольких экземплярах приложения.