Merge/redis concurrency into develop - #85
Conversation
- DistributedLockExecutor: Redisson 기반 분산락 공통 실행기 추가 (global/lock/) - ErrorCode: LOCK_TIMEOUT 추가 (HTTP 409) - ReservationService: 좌석 차감 경로 3곳에 분산락 적용 - create / createReservationFromAi / updateReservation - 같은 좌석에 동시 요청을 직렬화하여 초과 예약 방지 - 충돌 시 명시 응답으로 전환 (REMAIN_EXHAUSTED / LOCK_TIMEOUT)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a distributed locking mechanism using Redisson to manage concurrency in the reservation system. A new DistributedLockExecutor component was implemented, and the ReservationService was refactored to wrap reservation creation and update logic within these locks using TransactionTemplate. Feedback indicates a potential race condition in the updateReservation method, which currently only locks the new reservation slot while the old slot remains unprotected during cancellation. It was also recommended to extract hardcoded lock duration parameters into constants to improve maintainability.
| public ReservationUpdateResponseDto updateReservation(Long reservationId, Long userId, ReservationUpdateRequestDto request) { | ||
| Reservation oldReservation = cancelReservationCore(reservationId, userId, ReservationStatus.REPLACED); | ||
| StoreRemain oldStoreRemain = oldReservation.getStoreRemain(); | ||
|
|
||
| // 기존 결제를 새 예약으로 이전 | ||
| Payment oldPayment = paymentRepository.findByReservation_Id(reservationId).orElse(null); | ||
|
|
||
| Reservation newReservation = createReservationCore(userId, request.newRemainId(), request.newMember(), request.couponId()); | ||
| newReservation.changeStatus(ReservationStatus.CONFIRMED); | ||
|
|
||
| if (oldPayment != null) { | ||
| oldPayment.transferToReservation(newReservation); | ||
| } | ||
|
|
||
| StoreRemain newStoreRemain = newReservation.getStoreRemain(); | ||
| eventPublisher.publishEvent(new ReservationChangedEvent( | ||
| newReservation.getId(), | ||
| userId, | ||
| newStoreRemain.getStore().getStoreName(), | ||
| oldStoreRemain.getRemainDate().toString(), | ||
| oldStoreRemain.getRemainTime().toString(), | ||
| newStoreRemain.getRemainDate().toString(), | ||
| newStoreRemain.getRemainTime().toString() | ||
| )); | ||
|
|
||
| return new ReservationUpdateResponseDto( | ||
| newReservation.getId(), | ||
| newReservation.getStoreRemain().getId(), | ||
| newReservation.getMember(), | ||
| newReservation.getStatus().name().toLowerCase(), | ||
| newReservation.getUpdatedAt() != null ? newReservation.getUpdatedAt() : java.time.LocalDateTime.now() | ||
| ); | ||
| String lockKey = "lock:reservation:remain:" + request.newRemainId(); | ||
|
|
||
| return lockExecutor.executeWithLock(lockKey, 3, 5, () -> { | ||
| TransactionTemplate tx = new TransactionTemplate(transactionManager); | ||
| return tx.execute(status -> { | ||
| Reservation oldReservation = cancelReservationCore(reservationId, userId, ReservationStatus.REPLACED); | ||
| StoreRemain oldStoreRemain = oldReservation.getStoreRemain(); | ||
|
|
||
| // 기존 결제를 새 예약으로 이전 | ||
| Payment oldPayment = paymentRepository.findByReservation_Id(reservationId).orElse(null); | ||
|
|
||
| Reservation newReservation = createReservationCore(userId, request.newRemainId(), request.newMember(), request.couponId()); | ||
| newReservation.changeStatus(ReservationStatus.CONFIRMED); | ||
|
|
||
| if (oldPayment != null) { | ||
| oldPayment.transferToReservation(newReservation); | ||
| } | ||
|
|
||
| StoreRemain newStoreRemain = newReservation.getStoreRemain(); | ||
| eventPublisher.publishEvent(new ReservationChangedEvent( | ||
| newReservation.getId(), | ||
| userId, | ||
| newStoreRemain.getStore().getStoreName(), | ||
| oldStoreRemain.getRemainDate().toString(), | ||
| oldStoreRemain.getRemainTime().toString(), | ||
| newStoreRemain.getRemainDate().toString(), | ||
| newStoreRemain.getRemainTime().toString() | ||
| )); | ||
|
|
||
| return new ReservationUpdateResponseDto( | ||
| newReservation.getId(), | ||
| newReservation.getStoreRemain().getId(), | ||
| newReservation.getMember(), | ||
| newReservation.getStatus().name().toLowerCase(), | ||
| newReservation.getUpdatedAt() != null ? newReservation.getUpdatedAt() : java.time.LocalDateTime.now() | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
현재 예약 변경 로직은 새로운 예약 시간대(request.newRemainId())에 대해서만 분산 락을 획득하고 있습니다. 하지만 예약 변경 과정에는 기존 예약을 취소하는 로직(cancelReservationCore 호출)이 포함되어 있으며, 이 과정에서 기존 StoreRemain의 좌석 수를 늘리는 작업이 수행됩니다.
이 기존 StoreRemain에 대한 수정 작업은 분산 락으로 보호되지 않아, 다른 트랜잭션(예: 해당 시간대에 대한 신규 예약 생성)과 경쟁 상태(race condition)에 빠질 수 있습니다. 이로 인해 OptimisticLockingFailureException이 발생하여 요청이 실패할 수 있으며, 이는 분산 락을 도입한 본래의 목적에 부합하지 않습니다.
이 문제를 해결하기 위해서는 기존 StoreRemain과 새로운 StoreRemain 모두에 대해 락을 획득해야 합니다. 교착 상태(deadlock)를 방지하기 위해 항상 일관된 순서(예: ID 오름차순)로 락을 획득하는 것이 중요합니다.
| Long remainId = availableRemain.get().getId(); | ||
| String lockKey = "lock:reservation:remain:" + remainId; | ||
|
|
||
| return lockExecutor.executeWithLock(lockKey, 3, 5, () -> { |
There was a problem hiding this comment.
분산 락의 대기 시간(3초)과 임대 시간(5초)이 하드코딩되어 있습니다. 이 값들은 create (218라인), updateReservation (277라인) 메서드에서도 반복적으로 사용됩니다.
코드의 유지보수성과 일관성을 높이기 위해 이 값들을 클래스 레벨의 private static final 상수로 추출하는 것을 권장합니다.
예시:
private static final long LOCK_WAIT_SECONDS = 3;
private static final long LOCK_LEASE_SECONDS = 5;이렇게 변경하면 향후 락 정책 변경 시 한 곳에서만 수정하면 되어 관리가 용이해집니다.
2 participants
📢 기능 설명
필요시 실행결과 스크린샷 첨부
연결된 issue
연결된 issue를 자동을 닫기 위해 아래 {이슈넘버}를 입력해주세요.
close #{이슈넘버}
✅ 체크리스트