쿠폰 고도화 / 비관적 락 -> Redis Lua - #91
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request transitions the coupon issuance logic from a database pessimistic lock approach to a high-performance Redis Lua script-based mechanism. It also introduces Resilience4j circuit breaking and time limiting to protect against Redis outages. The code reviewer identified several critical issues with the current implementation: 1) executing Redis network I/O and asynchronous joins inside a database transaction, which can quickly exhaust the HikariCP connection pool; 2) an inefficient double-blocking thread pool architecture for simple Redis calls; and 3) a memory leak bug where empty Redis Sets are not assigned a TTL because EXPIREAT is called on non-existent keys. Detailed suggestions were provided to move the Redis calls outside the @Transactional block using TransactionTemplate, convert the Redis client call to a synchronous execution, and dynamically apply the TTL inside the Lua script.
| private final CouponRepository couponRepository; | ||
| private final CouponTemplateRepository couponTemplateRepository; | ||
| private final UserRepository userRepository; | ||
| private final RedisCouponIssuer redisCouponIssuer; |
Contributor
There was a problem hiding this comment.
💡 TransactionTemplate 주입
Redis 호출을 데이터베이스 트랜잭션 외부로 분리하기 위해 TransactionTemplate을 주입받아 사용하도록 필드를 추가합니다.
Suggested change
| private final RedisCouponIssuer redisCouponIssuer; | |
| private final RedisCouponIssuer redisCouponIssuer; | |
| private final TransactionTemplate transactionTemplate; |
Comment on lines
+62
to
+94
| @Transactional | ||
| public CouponIssueResponse issueCoupon(Long templateId, Long userId) { | ||
| User user = userRepository.getById(userId); | ||
|
|
||
| CouponTemplate template = couponTemplateRepository.findByIdWithLock(templateId) | ||
| .orElseThrow(() -> new CustomException(ErrorCode.COUPON_TEMPLATE_NOT_FOUND)); | ||
|
|
||
| if (couponRepository.existsByUserIdAndCouponTemplateId(userId, templateId)) { | ||
| throw new CustomException(ErrorCode.DUPLICATE_COUPON); | ||
| IssueResult result = unwrapIssueResult(templateId, userId); | ||
| switch (result) { | ||
| case DUPLICATE -> throw new CustomException(ErrorCode.DUPLICATE_COUPON); | ||
| case EXHAUSTED -> throw new CustomException(ErrorCode.COUPON_EXHAUSTED); | ||
| case NOT_AVAILABLE -> throw new CustomException(ErrorCode.COUPON_TEMPLATE_NOT_FOUND); | ||
| case UNAVAILABLE -> throw new CustomException(ErrorCode.COUPON_ISSUE_TEMPORARILY_UNAVAILABLE); | ||
| case SUCCESS -> { /* fall through */ } | ||
| } | ||
|
|
||
| template.decreaseRemain(); | ||
|
|
||
| Coupon coupon = Coupon.builder() | ||
| .user(user) | ||
| .couponTemplate(template) | ||
| .build(); | ||
| try { | ||
| User user = userRepository.getById(userId); | ||
| CouponTemplate template = couponTemplateRepository.findById(templateId) | ||
| .orElseThrow(() -> new CustomException(ErrorCode.COUPON_TEMPLATE_NOT_FOUND)); | ||
|
|
||
| Coupon saved = couponRepository.save( | ||
| Coupon.builder() | ||
| .user(user) | ||
| .couponTemplate(template) | ||
| .build() | ||
| ); | ||
| return CouponIssueResponse.from(saved); | ||
| } catch (RuntimeException e) { | ||
| // Redis 는 발급 확정 상태인데 DB INSERT 실패 → 보상. | ||
| try { | ||
| redisCouponIssuer.compensate(templateId, userId); | ||
| } catch (Exception compEx) { | ||
| log.error("쿠폰 발급 Redis 보상 실패. templateId={}, userId={}", templateId, userId, compEx); | ||
| } | ||
| throw e; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
⚠️ 트랜잭션 내 외부 네트워크 I/O (Redis 호출) 포함 문제
현재 issueCoupon 메서드 전체에 @Transactional이 적용되어 있어, Redis Lua 스크립트를 호출하는 unwrapIssueResult가 데이터베이스 트랜잭션 내부에서 실행됩니다.
문제점
- 커넥션 점유 시간 증가: Redis 호출(네트워크 I/O) 및 Resilience4j의 비동기 대기(
.join()) 시간 동안 DB 커넥션(HikariCP)을 계속 붙잡고 있게 됩니다. - 커넥션 풀 고갈: 고동시성(High Concurrency) 환경에서 Redis 지연이 조금이라도 발생하면, DB 커넥션 풀이 순식간에 고갈되어 전체 시스템 장애(
SQLTransientConnectionException)로 이어질 수 있습니다.
개선 방안
Redis 호출을 통한 선착순 검증(EVAL)은 트랜잭션 외부에서 수행하고, 발급이 확정된 건에 대해서만 DB 트랜잭션을 시작하도록 분리해야 합니다. 이를 위해 TransactionTemplate을 사용하여 DB 저장 로직만 트랜잭션으로 감싸는 방식을 권장합니다.
public CouponIssueResponse issueCoupon(Long templateId, Long userId) {
IssueResult result = unwrapIssueResult(templateId, userId);
switch (result) {
case DUPLICATE -> throw new CustomException(ErrorCode.DUPLICATE_COUPON);
case EXHAUSTED -> throw new CustomException(ErrorCode.COUPON_EXHAUSTED);
case NOT_AVAILABLE -> throw new CustomException(ErrorCode.COUPON_TEMPLATE_NOT_FOUND);
case UNAVAILABLE -> throw new CustomException(ErrorCode.COUPON_ISSUE_TEMPORARILY_UNAVAILABLE);
case SUCCESS -> { /* fall through */ }
}
try {
return transactionTemplate.execute(status -> {
User user = userRepository.getById(userId);
CouponTemplate template = couponTemplateRepository.findById(templateId)
.orElseThrow(() -> new CustomException(ErrorCode.COUPON_TEMPLATE_NOT_FOUND));
Coupon saved = couponRepository.save(
Coupon.builder()
.user(user)
.couponTemplate(template)
.build()
);
return CouponIssueResponse.from(saved);
});
} catch (RuntimeException e) {
// Redis 는 발급 확정 상태인데 DB INSERT 실패 → 보상.
try {
redisCouponIssuer.compensate(templateId, userId);
} catch (Exception compEx) {
log.error("쿠폰 발급 Redis 보상 실패. templateId={}, userId={}", templateId, userId, compEx);
}
throw e;
}
}
Comment on lines
101
to
110
| private IssueResult unwrapIssueResult(Long templateId, Long userId) { | ||
| try { | ||
| return redisCouponIssuer.tryIssue(templateId, userId).join(); | ||
| } catch (CompletionException e) { | ||
| Throwable cause = e.getCause(); | ||
| if (cause instanceof CustomException ce) throw ce; | ||
| if (cause instanceof RuntimeException re) throw re; | ||
| throw new CustomException(ErrorCode.COUPON_ISSUE_TEMPORARILY_UNAVAILABLE); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
💡 unwrapIssueResult 동기식 전환
tryIssue가 동기식으로 변경됨에 따라, 불필요한 비동기 대기(.join()) 및 예외 처리 로직을 제거하고 동기식으로 단순화합니다.
private IssueResult unwrapIssueResult(Long templateId, Long userId) {
return redisCouponIssuer.tryIssue(templateId, userId);
}
Comment on lines
+68
to
+94
| @TimeLimiter(name = RESILIENCE_INSTANCE) | ||
| @CircuitBreaker(name = RESILIENCE_INSTANCE, fallbackMethod = "tryIssueFallback") | ||
| public CompletableFuture<IssueResult> tryIssue(Long templateId, Long userId) { | ||
| return CompletableFuture.supplyAsync(() -> { | ||
| Long code = redissonClient | ||
| .getScript(StringCodec.INSTANCE) | ||
| .eval( | ||
| RScript.Mode.READ_WRITE, | ||
| issueLuaScript, | ||
| RScript.ReturnType.INTEGER, | ||
| List.of(stockKey(templateId), issuedKey(templateId)), | ||
| String.valueOf(userId) | ||
| ); | ||
| return IssueResult.of(code); | ||
| }, couponIssueExecutor); | ||
| } | ||
|
|
||
| /** | ||
| * Circuit Breaker / TimeLimiter 폴백. | ||
| * 비즈니스 예외는 ignore-exceptions 로 걸러져서 여기엔 인프라 장애만 들어온다. | ||
| */ | ||
| @SuppressWarnings("unused") // Resilience4j 가 리플렉션으로 호출 | ||
| private CompletableFuture<IssueResult> tryIssueFallback(Long templateId, Long userId, Throwable t) { | ||
| log.warn("쿠폰 발급 Redis 호출 실패. templateId={}, userId={}, cause={}", | ||
| templateId, userId, t.toString()); | ||
| return CompletableFuture.completedFuture(IssueResult.UNAVAILABLE); | ||
| } |
Contributor
There was a problem hiding this comment.
⚠️ 비효율적인 비동기 스레드 풀 사용 및 이중 블로킹 문제
현재 @TimeLimiter 적용을 위해 tryIssue를 CompletableFuture.supplyAsync로 구현하고 별도의 스레드 풀(couponIssueExecutor)을 사용하고 있습니다. 하지만 CouponService에서 즉시 .join()을 호출하여 결과를 동기적으로 기다리고 있습니다.
문제점
- 이중 스레드 블로킹 (Double Blocking): 요청이 들어오면 톰캣 워커 스레드가 블로킹된 상태로 대기하고, 동시에
couponIssueExecutor스레드도 Redis 호출을 위해 블로킹됩니다. 즉, 하나의 요청 처리를 위해 두 개의 스레드가 점유됩니다. - 컨텍스트 스위칭 오버헤드: 1~2ms 내외로 끝나는 가벼운 Redis Lua 호출을 위해 스레드 풀 간 컨텍스트 스위칭이 발생하여 불필요한 CPU 오버헤드가 발생합니다.
- 병목 지점 형성: 톰캣 스레드가 아무리 많아도
couponIssueExecutor풀 크기(100)에 의해 동시 처리량이 제한됩니다.
개선 방안
- 동기식 전환:
tryIssue메서드를 동기식으로 전환하고CompletableFuture를 제거합니다. - 타임아웃 설정 일원화:
@TimeLimiter대신 Redisson 자체의 Command Timeout(예: 500ms) 설정을 활용합니다. Redisson은 Netty 기반의 비동기 I/O를 사용하므로 동기 호출 시에도 매우 효율적으로 타임아웃을 처리합니다. - 동기식 CircuitBreaker 적용:
@CircuitBreaker는 동기 메서드에도 완벽히 동작하므로, Redis 장애 시 회로 차단 및 폴백이 정상적으로 작동합니다. - 스레드 풀 제거: 이 변경을 통해
CouponIssueExecutorConfig클래스 전체를 안전하게 제거할 수 있습니다.
@CircuitBreaker(name = RESILIENCE_INSTANCE, fallbackMethod = "tryIssueFallback")
public IssueResult tryIssue(Long templateId, Long userId) {
Long code = redissonClient
.getScript(StringCodec.INSTANCE)
.eval(
RScript.Mode.READ_WRITE,
issueLuaScript,
RScript.ReturnType.INTEGER,
List.of(stockKey(templateId), issuedKey(templateId)),
String.valueOf(userId)
);
return IssueResult.of(code);
}
/**
* Circuit Breaker 폴백.
* 비즈니스 예외는 ignore-exceptions 로 걸러져서 여기엔 인프라 장애만 들어온다.
*/
@SuppressWarnings("unused") // Resilience4j 가 리플렉션으로 호출
private IssueResult tryIssueFallback(Long templateId, Long userId, Throwable t) {
log.warn("쿠폰 발급 Redis 호출 실패. templateId={}, userId={}, cause={}",
templateId, userId, t.toString());
return IssueResult.UNAVAILABLE;
}
Comment on lines
+25
to
+27
| redis.call('DECR', KEYS[1]) | ||
| redis.call('SADD', KEYS[2], ARGV[1]) | ||
| return 1 |
Contributor
There was a problem hiding this comment.
⚠️ Redis 빈 Set에 대한 EXPIRE 실패로 인한 메모리 누수 버그
현재 RedisCouponIssuer의 warmUp 및 warmUpIfAbsent 메서드에서 아직 생성되지 않은 빈 Set(coupon:issued:{templateId})에 대해 expireAt을 호출하고 있습니다.
문제점
- Redis EXPIRE 동작 방식: Redis에서는 존재하지 않는 키에 대해
EXPIRE또는EXPIREAT명령을 실행하면 아무런 동작도 하지 않고0을 반환합니다. - TTL 유실: 템플릿 생성 시점에는 발급 완료 Set이 비어 있어 존재하지 않으므로,
expireAt호출이 무시됩니다. - 메모리 누수: 이후 사용자가 쿠폰을 발급받을 때 Lua 스크립트의
SADD명령에 의해 Set이 처음 생성되지만, 이때는 **TTL이 설정되지 않은 상태(영구 저장)**로 생성됩니다. 이로 인해 발급 완료된 사용자 목록이 Redis에 영구히 남아 메모리 누수가 발생합니다.
개선 방안
Lua 스크립트 내부에서 재고 키(KEYS[1])의 남은 TTL을 조회하여, 발급 완료 Set(KEYS[2])이 처음 생성되거나 업데이트될 때 동일한 TTL을 동적으로 설정하도록 개선해야 합니다.
redis.call('DECR', KEYS[1])
redis.call('SADD', KEYS[2], ARGV[1])
local ttl = redis.call('TTL', KEYS[1])
if ttl > 0 then
redis.call('EXPIRE', KEYS[2], ttl)
end
return 1
Comment on lines
+135
to
+136
| given(redisCouponIssuer.tryIssue(1L, 1L)) | ||
| .willReturn(CompletableFuture.completedFuture(IssueResult.SUCCESS)); |
Contributor
There was a problem hiding this comment.
💡 테스트 코드 Mocking 수정 (tryIssue 동기화)
tryIssue 메서드가 동기식으로 변경됨에 따라, 테스트 코드의 Mocking 정의도 CompletableFuture 래핑을 제거하고 동기식 반환값으로 수정합니다.
Suggested change
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(CompletableFuture.completedFuture(IssueResult.SUCCESS)); | |
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(IssueResult.SUCCESS); |
Comment on lines
+155
to
+156
| given(redisCouponIssuer.tryIssue(1L, 1L)) | ||
| .willReturn(CompletableFuture.completedFuture(IssueResult.UNAVAILABLE)); |
Contributor
There was a problem hiding this comment.
💡 테스트 코드 Mocking 수정 (tryIssue 동기화 - UNAVAILABLE)
Suggested change
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(CompletableFuture.completedFuture(IssueResult.UNAVAILABLE)); | |
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(IssueResult.UNAVAILABLE); |
Comment on lines
+167
to
+168
| given(redisCouponIssuer.tryIssue(1L, 1L)) | ||
| .willReturn(CompletableFuture.completedFuture(IssueResult.DUPLICATE)); |
Contributor
There was a problem hiding this comment.
💡 테스트 코드 Mocking 수정 (tryIssue 동기화 - DUPLICATE)
Suggested change
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(CompletableFuture.completedFuture(IssueResult.DUPLICATE)); | |
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(IssueResult.DUPLICATE); |
Comment on lines
+179
to
+180
| given(redisCouponIssuer.tryIssue(1L, 1L)) | ||
| .willReturn(CompletableFuture.completedFuture(IssueResult.EXHAUSTED)); |
Contributor
There was a problem hiding this comment.
💡 테스트 코드 Mocking 수정 (tryIssue 동기화 - EXHAUSTED)
Suggested change
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(CompletableFuture.completedFuture(IssueResult.EXHAUSTED)); | |
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(IssueResult.EXHAUSTED); |
Comment on lines
+194
to
+195
| given(redisCouponIssuer.tryIssue(1L, 1L)) | ||
| .willReturn(CompletableFuture.completedFuture(IssueResult.SUCCESS)); |
Contributor
There was a problem hiding this comment.
💡 테스트 코드 Mocking 수정 (tryIssue 동기화 - DB 실패 시 보상 테스트)
Suggested change
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(CompletableFuture.completedFuture(IssueResult.SUCCESS)); | |
| given(redisCouponIssuer.tryIssue(1L, 1L)) | |
| .willReturn(IssueResult.SUCCESS); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📢 기능 설명
필요시 실행결과 스크린샷 첨부
연결된 issue
연결된 issue를 자동을 닫기 위해 아래 {이슈넘버}를 입력해주세요.
close #{이슈넘버}
✅ 체크리스트