[4단계 - Transaction synchronization 적용하기] 모코(송선권) 미션 제출합니다. #1173
Conversation
Walkthrough이 변경 사항은 트랜잭션 관리 아키텍처를 리팩토링합니다. TransactionManager를 인터페이스로 변환하고 GeneralTransactionManager 구현체를 추가합니다. DAO 메서드들은 명시적 Connection 파라미터를 제거하고 TransactionManagerConfig.getCurrentConnection()을 사용하도록 변경됩니다. UserService는 인터페이스로 변환되며, AppUserService는 비즈니스 로직 구현, TxUserService는 트랜잭션 래핑을 담당합니다. 테스트와 관련 클래스들도 새로운 구조에 맞게 업데이트됩니다. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java (1)
28-39: executeInTransaction 메서드의 commit 실패 시 예외 은닉 문제 해결 필요리뷰 코멘트의 우려가 정확히 검증되었습니다. 현재 구현에서 실제 문제가 발생합니다:
문제 시나리오:
commit()실패 시, 내부 finally 블록이cleanupConnection()을 호출해connection.remove()실행 (라인 103)executeInTransaction의 catch 블록이rollback()호출rollback()에서connection.get()이 null 반환 →IllegalStateException발생- 원래의 commit 실패 원인이 새로운 예외로 대체됨
이 패턴은 두 메서드 모두 (lines 28-39, 41-51) 동일하게 영향받습니다.
개선 방향을 고려해보세요:
- commit 단계의 예외를 구분 처리하여, 이미 cleanup된 상태에서는 rollback 재시도를 피할 필요가 있습니다
- 예외 체이닝으로 원본 원인을 보존하는 방식도 검토해볼 만합니다
- 혹은 commit 실패 시 명시적인 정리(cleanup) 처리 분기를 두는 것도 고려 대상입니다
이 문제를 해결하지 않으면 디버깅이 어려워지고 실제 장애 원인을 파악하기 곤란합니다.
🧹 Nitpick comments (21)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java (1)
53-61: 콜백에 Connection 노출 최소화DAO가 내부적으로 현재 트랜잭션 Connection을 조회한다면, 콜백에 Connection을 넘길 필요가 줄어듭니다. 인프라 노출을 줄여 응집도를 높이기 위해 파라미터 제거(또는 비노출 타입으로 대체)를 검토해보시겠어요?
app/src/test/java/com/techcourse/service/UserServiceTest.java (3)
29-31: 트랜잭션 경계를 서비스 레이어로 올린 결정 GoodTxUserService로 감싸 테스트하는 방향이 의도에 부합합니다. 중첩 트랜잭션(내부 로직이 다시 트랜잭션을 시작하는 경우) 시나리오까지 케이스 추가해보면 어떨까요?
Also applies to: 36-37
57-57: 이름 일관성(createBy vs createdBy)테스트는 createdBy를 쓰고, 서비스 시그니처는 createBy입니다. “축약 금지” 원칙 관점에서도 createdBy로 통일을 고려해보셨나요?
58-61: 롤백 검증 강도 강화 제안비밀번호 원복만 확인하는데, UserHistory 미기록(또는 기록 롤백)까지 함께 검증하면 트랜잭션 동작을 더 확실히 보장할 수 있습니다. 어떤 지표로 확인할지 고민해보셨나요?
app/src/test/java/com/techcourse/service/MockUserHistoryDao.java (1)
15-16: 시그니처 정렬 OK, 예외 메시지 가시성테스트더라도 예외 메시지를 부여하면 실패 원인 파악이 쉬워집니다. 의도(롤백 유도)를 메시지로 드러내는 걸 고려해보세요.
app/src/main/java/com/techcourse/service/UserService.java (1)
5-11: 도메인 언어와 API 일관성
- 파라미터 이름 createBy → createdBy로 통일하면 의도가 명확해집니다(“축약 금지”).
- 원시값 포장 원칙: id, newPassword, createdBy를 VO로 포장하면 검증과 불변성을 가까이 둘 수 있습니다. 어떤 VO들이 적합할지 생각해보셨나요?
app/src/main/java/com/techcourse/service/AppUserService.java (2)
29-34: 행위 주도 설계(Tell, Don’t Ask)로 역사 기록 생성 책임 위치 재고UserHistory 생성을 서비스가 직접 하는 대신, User가 자신의 상태 변경에 대한 히스토리를 만들도록 위임하면 응집도가 높아집니다. 예: “사용자가 비밀번호 변경을 수행하면 히스토리도 함께 생성”. 어떤 객체가 이 책임을 가장 잘 가질까요?
8-16: 트랜잭션 경계 의존성 명확화AppUserService는 트랜잭션이 보장된 컨텍스트(TxUserService)에서만 사용되어야 합니다. 이 제약을 문서/가시적 설계로 드러낼 방법(예: 패키지 가시성, 생성 경로 제한)을 고려해보셨나요?
app/src/test/java/com/techcourse/dao/UserDaoTest.java (1)
32-34: 테스트에서 트랜잭션 래핑 적용 Good + 자잘한 정리 제안
- 모든 DAO 호출을 TransactionManagerConfig로 감싼 점 좋습니다. 읽기 케이스도 동일 경계에서 검증되어 일관성이 있습니다.
- 이제 개별 Connection 취득/필드가 불필요해졌습니다(예: setup의 connection, throws SQLException 등). 사용처를 정리하면 테스트가 더 간결해져요.
Also applies to: 39-42, 47-52, 56-61, 66-74, 79-89, 94-101
app/src/main/java/com/techcourse/service/TxUserService.java (5)
10-12: 의존 역전: TxUserService가 AppUserService(구현체)에 직접 결합되어 있습니다.트랜잭션 프록시는 보통 데코레이터로 동작하므로 인터페이스(UserService)에 의존하는 편이 결합도를 낮춥니다. 향후 다른 구현체가 생길 때 확장성에 차이가 납니다. 의도적으로 구현체를 고정하신 건가요? 인터페이스 의존으로 전환을 검토해보세요.
14-18: 읽기 전용 조회에도 트랜잭션을 여는 결정, 의도 확인 필요findById까지 항상 트랜잭션을 열면 DB 락/리소스 점유가 늘 수 있어요. 읽기 전용 트랜잭션 속성(또는 비트랜잭션 조회) 전략을 고려해보셨나요? 예: 서비스 레벨에서 읽기/쓰기 경계를 명시적으로 구분.
20-30: 중복되는 트랜잭션 보일러플레이트세 메서드 모두 동일한 트랜잭션 감싸기 패턴을 반복합니다. AOP/프록시(동적) 기반으로 공통화하면 서비스 메서드는 오롯이 도메인 로직만 담을 수 있어 응집도가 높아집니다. 언제까지 수동 래핑을 유지할지 결정 기준을 정해두면 좋습니다.
6-30: 어노테이션 누락UserService 구현 메서드에 @OverRide가 보이지 않습니다. Java Style Guide 준수를 위해 명시를 권장합니다.
26-30: 원시값·문자열 포장 검토id, newPassword, createBy는 도메인 의미가 강합니다. 값 객체나 커맨드 객체로 포장하면 검증과 의도가 응집됩니다. 왜 중요한가: 유효성(길이/형식/작성자 정책) 규칙을 객체로 밀어 넣어 서비스의 분기를 줄일 수 있습니다.
[Object Calisthenics 규칙 3 준수 권장]jdbc/src/main/java/com/interface21/jdbc/core/TransactionManager.java (1)
5-13: 트랜잭션 계약의 표현력 보강 제안
- setRollbackOnly()/isRollbackOnly()가 없어서 “내부 계층에서 롤백만 표시” 시나리오를 표현하기 어렵습니다. 현재는 예외 전파에만 의존합니다.
- getCurrentConnection()은 JDBC 커넥션을 노출해 상위 계층과 인프라 결합을 유발합니다. 커넥션 접근은 하위(DAO)에서만 필요하도록 설계 경계를 점검해보세요.
app/src/main/java/com/techcourse/dao/UserDao.java (3)
26-56: DAO가 트랜잭션 컨텍스트에 강하게 결합모든 메서드가 TransactionManagerConfig.getCurrentConnection()에 의존합니다. 트랜잭션 경계 밖에서 호출되면 즉시 예외가 발생합니다. 모든 상위 호출부가 프록시/래퍼를 통해 트랜잭션을 여는지 점검해 주세요. 또한 DAO는 가능한 한 “커넥션 공급자” 추상에 의존하도록 설계를 고려해 볼 수 있습니다.
54-56: 한 줄에 점 하나(디미터 법칙) + 게터 사용delete에서 simpleJpa.deleteById(..., user.getClass(), user.getId())처럼 한 줄에 여러 점을 사용하고, 게터로 내부 상태를 꺼내고 있습니다. 왜 중요한가: 결합도가 높아지고 객체 자율성이 약해집니다. 값 추출을 최소화하고, 필요한 데이터를 상위에서 명시적으로 전달하거나 도메인에 “행동”을 위임하는 방향을 검토해보세요.
[Object Calisthenics 규칙 4, 9]
38-39: 원시값·문자열 포장id, account는 도메인 의미가 분명합니다. 래퍼/값 객체를 도입하면 유효성(길이, 포맷, 존재성) 검증을 일관되게 적용하고 오용을 줄일 수 있습니다.
[Object Calisthenics 규칙 3]Also applies to: 46-48
jdbc/src/main/java/com/interface21/jdbc/core/GeneralTransactionManager.java (3)
21-30: 중첩 카운트 NPE 가능성 점검connection은 존재하나 transactionCount가 초기화되지 않은 비정상 상태에서 transactionCount.get() + 1이 NPE가 될 수 있습니다. 이 불변식(둘은 항상 함께 존재)을 어디에서 보장하는지 점검해 주세요. 필요하다면 단일 컨텍스트 객체로 묶어 원자성을 높이는 방법도 있습니다.
44-90: 들여쓰기 depth와 else 사용이 많습니다
- 규칙 1: 한 메서드에 들여쓰기 1단계 유지
- 규칙 2: else 예약어 금지
현재 commit/rollback에는 if-try-else 중첩이 깊습니다. 가드 클로즈, 조기 반환, 작은 메서드 추출로 복잡도를 낮추면 가독성과 테스트 용이성이 좋아집니다.
[Object Calisthenics 규칙 1, 2][As per coding guidelines]
9-13: 필드 수와 클래스 크기
- 규칙 6/7: 클래스는 작게, 인스턴스 변수 3개 이하
현재 ThreadLocal 3개 + DataSource로 4개이며, 클래스도 100+ 라인입니다. Connection/카운트/rollbackOnly를 보관하는 “트랜잭션 컨텍스트” 객체로 응집시키면 필드 수를 줄이고 정합성 보장이 쉬워집니다.Also applies to: 92-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java(2 hunks)app/src/main/java/com/techcourse/dao/UserDao.java(2 hunks)app/src/main/java/com/techcourse/dao/UserHistoryDao.java(2 hunks)app/src/main/java/com/techcourse/service/AppUserService.java(1 hunks)app/src/main/java/com/techcourse/service/TxUserService.java(1 hunks)app/src/main/java/com/techcourse/service/UserService.java(1 hunks)app/src/test/java/com/techcourse/dao/UserDaoTest.java(2 hunks)app/src/test/java/com/techcourse/service/MockUserHistoryDao.java(1 hunks)app/src/test/java/com/techcourse/service/UserServiceTest.java(2 hunks)jdbc/src/main/java/com/interface21/jdbc/core/GeneralTransactionManager.java(1 hunks)jdbc/src/main/java/com/interface21/jdbc/core/TransactionManager.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 🎯 코드 품질 중심 리뷰 가이드라인이 리뷰는 코드 품질과 객체지향 원칙에 집중합니다.
미션 달성 여부가 아닌, 코드 설계와 품질 개선에 대한 피드백을 한글로 제공해주세요.📚 학습 원칙
- 직접 코드를 제공하지 마세요 (학습자가 명시적으로 요청하는 경우 제외)
- 문제 해결 과정을 안내하되, 정답을 바로 알려주지 마세요
- 작은 단계로 문제를 분해하여 접근하도록 도와주세요
💡 피드백 방법
- 유도 질문 활용: "만약 ~라면 어떻게 될까요?", "~를 고려해보셨나요?"
- 힌트 제공: 방향은 제시하되, 구체적인 구현은 학습자가 하도록
- 다양한 접근법 제시: 한 가지 해결책이 아닌 여러 가능성을 제안
- 왜?에 집중: 단순히 무엇이 잘못되었는지보다 왜 그런지 이해하도록
⚡ 객체지향 생활 체조 원칙 검토
다음은 객체지향 생활 체조(Object Calisthenics) 9가지 원칙입니다.
위반 시 학습 효과를 위해 반드시 피드백을 제공하되, 왜 이 원칙이 중요한지 설명해주세요:규칙 1: 한 메서드에 오직 한 단계의 들여쓰기만
- 들여쓰기 depth 최대 1 (중첩 제어구조 금지)
- 📖 이유: 메서드 복잡도 감소, 단일 책임 원칙 강화
- 💡 힌트: "이 부분을 별도 메서드로 추출하면 어떨까요?"
규칙 2: else 예약어 금지
- else, switch/case 사용 금지
- 📖 이유: 복잡한 분기 제거, 명확한 코드 흐름
- 💡 힌트: "early return이나 가드 클로즈 패턴을 고려해보세요"
규칙 3: 모든 원시값과 문자열 포장
- 원시 타입과 String을 객체로 포장
- 📖 이유: 도메인 개념 명확화, 비즈니스 로직 응집
- 💡 예시:
int port→Port port,String name→Name name규칙 4: 한 줄에 점 하나만 (디미터 법칙)
- 메서드 체이닝 제한
- 📖 이유: 결합도 감소, 캡슐화 향상
- 💡 나쁜 예:
request.getUri().getPath().substring()- 💡 좋은 예:
request.extractPath()규칙 5: 축약 금지
- 명확한 이름 사용 (축약어 금지)
- 📖 이유: 코드 가독성, 의도 명확화
- 💡 예시:
req→request,calcAmt→calculateAmount규칙 6: 모든 엔티티를 작게 유지
- 클래스 50줄, 메서드 10줄 이하
- 📖 이유: 단일 책임, 이해와 테스트 용이성
- 💡 힌트: "이 클래스가 너무 많은 일을 하고 있지 않나요?"
규칙 7: 인스턴스 변수 3개 이하
- 클래스당 최대 3개의 인스턴스 변수
- 📖 이유: 높은 응집도, 단일 책임 유지
- 💡 힌트: "관련 필드들을 별도 객체로 묶을 수 있을까요?"
규칙 8: 일급 컬렉션 사용
- 컬렉션을 감싸는 클래스 사용
- 📖 이유: 컬렉션 로직 캡슐화, 불변성 보장
- 💡 예시:
List<HttpHeader>→HttpHeaders클래스규칙 9: 게터/세터/프로퍼티 금지
- Tell, Don't Ask 원칙 준수
- 📖 이유: 진정한 캡슐화, 객체 자율성
- 💡 나쁜 예:
if (user.getPassword().equals(input))- 💡 좋은 예:
if (user.authenticate(input))🎯 추가 프로그래밍 요구사항
코드 스타일
- Java Style Guide 준수
- 3항 연산자 사용 금지
테스트 및 구조
- **모든 기능 T...
Files:
app/src/main/java/com/techcourse/service/AppUserService.javaapp/src/test/java/com/techcourse/service/MockUserHistoryDao.javaapp/src/main/java/com/techcourse/service/UserService.javaapp/src/main/java/com/techcourse/config/TransactionManagerConfig.javaapp/src/main/java/com/techcourse/dao/UserHistoryDao.javaapp/src/main/java/com/techcourse/service/TxUserService.javajdbc/src/main/java/com/interface21/jdbc/core/GeneralTransactionManager.javaapp/src/main/java/com/techcourse/dao/UserDao.javaapp/src/test/java/com/techcourse/dao/UserDaoTest.javajdbc/src/main/java/com/interface21/jdbc/core/TransactionManager.javaapp/src/test/java/com/techcourse/service/UserServiceTest.java
🧬 Code graph analysis (8)
app/src/main/java/com/techcourse/service/AppUserService.java (3)
app/src/main/java/com/techcourse/dao/UserDao.java (1)
UserDao(14-57)app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
UserHistoryDao(12-27)study/src/main/java/aop/domain/UserHistory.java (1)
UserHistory(5-59)
app/src/main/java/com/techcourse/service/UserService.java (1)
study/src/test/java/aop/stage1/UserService.java (1)
UserService(9-36)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java (1)
jdbc/src/main/java/com/interface21/jdbc/core/GeneralTransactionManager.java (1)
GeneralTransactionManager(7-107)
app/src/main/java/com/techcourse/dao/UserHistoryDao.java (1)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java (1)
TransactionManagerConfig(12-64)
app/src/main/java/com/techcourse/service/TxUserService.java (1)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java (1)
TransactionManagerConfig(12-64)
app/src/main/java/com/techcourse/dao/UserDao.java (1)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java (1)
TransactionManagerConfig(12-64)
app/src/test/java/com/techcourse/dao/UserDaoTest.java (1)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java (1)
TransactionManagerConfig(12-64)
app/src/test/java/com/techcourse/service/UserServiceTest.java (1)
app/src/main/java/com/techcourse/config/DataSourceConfig.java (1)
DataSourceConfig(7-27)
🔇 Additional comments (2)
app/src/main/java/com/techcourse/config/TransactionManagerConfig.java (1)
19-19: ThreadLocal 누수 재점검 제안PR 설명에 “트랜잭션 종료 시 커넥션/ThreadLocal 누수 해결”이 포함되어 있습니다. GeneralTransactionManager의 cleanup에서 모든 ThreadLocal(특히 rollbackOnly)까지 제거되는지 확인되었나요? 누락 시 스레드 풀 환경에서 잔존 상태로 인한 오동작 가능성이 있습니다.
app/src/main/java/com/techcourse/dao/UserDao.java (1)
43-43: 일관성 유지: getFirst()와 get(0) 통일 필요프로젝트가 Java 21을 타깃하므로
getFirst()사용에 호환성 문제는 없습니다. 다만,findById()에서는getFirst(),findByAccount()에서는get(0)을 사용 중으로, 같은 패턴의 코드에서 메서드를 달리 사용하고 있습니다. 두 메서드를 통일하여 코드 일관성을 개선해 주세요.Likely an incorrect or invalid review comment.
| if (Objects.isNull(INSTANCE)) { | ||
| DataSource dataSource = DataSourceConfig.getInstance(); | ||
| INSTANCE = new TransactionManager(dataSource); | ||
| INSTANCE = new GeneralTransactionManager(dataSource); |
There was a problem hiding this comment.
싱글턴 초기화 동시성 안전성 보강 필요
멀티스레드 환경에서 getInstance 동시 호출 시 중복 초기화 가능성이 있습니다. 정적 홀더/enum/volatile DCL 등 확실한 초기화 전략을 고려해보셨나요?
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/config/TransactionManagerConfig.java around
line 19, the singleton INSTANCE is assigned directly (INSTANCE = new
GeneralTransactionManager(dataSource)) allowing race conditions on concurrent
getInstance calls; change to a thread-safe initialization strategy such as: make
INSTANCE volatile and implement double-checked locking in getInstance, or use
the static holder idiom (private static class Holder { static final
GeneralTransactionManager INSTANCE = new GeneralTransactionManager(...); }) or
convert to an enum singleton; update getInstance accordingly to ensure only one
instance is created safely under concurrency.
| return users.isEmpty() ? null : users.getFirst(); | ||
| } |
There was a problem hiding this comment.
3항 연산자 사용 금지 위반
두 곳의 반환부에서 삼항 연산자를 사용하고 있습니다. 코딩 가이드(추가 프로그래밍 요구사항) 위반이므로, 가드 클로즈나 명시적 분기로 풀어 가독성을 높여주세요. 왜 중요한가: 조건이 늘어날수록 가독성이 급격히 떨어집니다.
[As per coding guidelines]
Also applies to: 51-52
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/dao/UserDao.java around lines 43-44 (and
similarly at 51-52), replace the ternary return expressions with a guard-clause
or explicit branching: check if the users collection is empty and return null
early, otherwise return users.getFirst(); do the same for the other occurrence
so both return paths use clear if/else or early-return style instead of the ? :
operator to comply with the coding guideline.
| public void log(final UserHistory userHistory) { | ||
| simpleJpa.insert(TransactionManagerConfig.getCurrentConnection(), userHistory); | ||
| } |
There was a problem hiding this comment.
DAO가 정적 구성요소에 직접 의존 — 테스트 용이성/결합도 이슈
TransactionManagerConfig.getCurrentConnection()에 직접 의존하면 인프라 결합이 커집니다.
- ConnectionProvider/TransactionManager 주입(DIP)이나 포트-어댑터 구조를 고려해보셨나요?
- 트랜잭션 외부 호출 시 IllegalStateException이 발생하는데, 이 실패 모드를 명시적으로 관리할 방법(가드/검증)도 고민해보세요.
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/dao/UserHistoryDao.java around lines 24 to
26, the DAO directly calls TransactionManagerConfig.getCurrentConnection()
causing tight static coupling and testability issues; refactor to accept a
ConnectionProvider or TransactionManager (constructor- or setter-injected) and
use that injected dependency instead of the static call, and add a
guard/validation before using the connection that checks for presence (or throws
a clear IllegalStateException with a descriptive message) so callers and tests
can control/verify transaction context.
| @Override | ||
| public void changePassword(final long id, final String newPassword, final String createBy) { | ||
| final var user = userDao.findById(id); | ||
| user.changePassword(newPassword); | ||
| userDao.update(user); | ||
| userHistoryDao.log(new UserHistory(user, createBy)); | ||
| } |
There was a problem hiding this comment.
사용자 미존재 시 NPE 위험
userDao.findById가 null을 반환할 수 있다면 user.changePassword에서 NPE가 납니다.
- “찾을 수 없음”을 명시적 예외/Optional로 표현하고, 서비스에서 도메인 예외로 전환하는 방식을 고려해보셨나요?
🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/service/AppUserService.java around lines
28-34, the code calls userDao.findById(...) then directly
user.changePassword(...), which risks an NPE if the DAO returns null; change the
flow to explicitly handle missing users: have the DAO return Optional<User> (or
check for null), and in the service translate absence into a domain/service
exception (e.g., throw new UserNotFoundException(id)) before any mutating calls;
only call changePassword, userDao.update, and userHistoryDao.log after the
presence check to guarantee no NPE and produce a clear error path for callers.
| public void begin() { | ||
| try { | ||
| // propagation level: REQUIRED | ||
| if (connection.get() == null) { | ||
| Connection connection1 = dataSource.getConnection(); | ||
| connection1.setAutoCommit(false); | ||
| connection.set(connection1); | ||
| transactionCount.set(1); | ||
| rollbackOnly.set(false); | ||
| } else { | ||
| // 중첩 트랜잭션인 경우 카운트 증가 | ||
| transactionCount.set(transactionCount.get() + 1); | ||
| } | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to begin transaction", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
begin 예외 시 롤백 경로 안정성
setAutoCommit(false) 전에 ThreadLocal에 커넥션을 넣지 않아 실패하면, 상위(TransactionManagerConfig)가 rollback()을 호출해도 “No transaction started” 예외로 원인을 덮을 수 있습니다. 트랜잭션 시작 실패 시에도 롤백이 “무해(idempotent)”하게 동작하거나, 설정/등록 순서를 재검토해 일관된 상태를 보장할 필요가 있습니다.
🤖 Prompt for AI Agents
In jdbc/src/main/java/com/interface21/jdbc/core/GeneralTransactionManager.java
around lines 18 to 34, begin() currently obtains a Connection and calls
setAutoCommit(false) before consistently registering/initializing ThreadLocal
state, causing rollback() to see "no transaction started" if setAutoCommit
throws; fix by: after obtaining the Connection, immediately set it into the
ThreadLocal, then attempt setAutoCommit(false) inside a try block, and if
setAutoCommit (or any following init step) throws, close the Connection and
clear the ThreadLocal before rethrowing; only set transactionCount and
rollbackOnly after setAutoCommit succeeds so the ThreadLocal is always in a
consistent state for rollback handling.
| private void cleanupConnection(Connection conn) { | ||
| try { | ||
| try { | ||
| conn.setAutoCommit(true); | ||
| } catch (Exception ignored) { | ||
| } | ||
| try { | ||
| conn.close(); | ||
| } catch (Exception ignored) { | ||
| } | ||
| } finally { | ||
| connection.remove(); | ||
| transactionCount.remove(); | ||
| } | ||
| } |
There was a problem hiding this comment.
ThreadLocal 누수: rollbackOnly 제거 누락
cleanupConnection에서 connection, transactionCount만 remove하고 rollbackOnly는 남겨둡니다. 스레드 풀 환경에서는 메모리/상태 누수로 이어질 수 있습니다. 트랜잭션 종료 시 모든 ThreadLocal을 반드시 정리하세요.
🤖 Prompt for AI Agents
In jdbc/src/main/java/com/interface21/jdbc/core/GeneralTransactionManager around
lines 92 to 106, cleanupConnection currently removes only connection and
transactionCount ThreadLocals leaving rollbackOnly behind; add
rollbackOnly.remove() in the finally block so all ThreadLocals (connection,
transactionCount, rollbackOnly) are cleared on transaction end to avoid
ThreadLocal leaks in pooled threads.
1 participant
안녕하세요 벡터! 모코입니다 🌱
벌써 마지막 단계네요 🥲
이번 미션에서는 요구사항대로 트랜잭션매니저를 인터페이스 분리하고, 서비스 계층을 프록시로 감싸 비즈니스 로직에서 데이터 관련 코드를 제거했습니다.
추가로 지난 미션에서 말씀해주신 다음 부분들을 반영했습니다.
리뷰 잘부탁드립니다! 😄