Skip to content
35 changes: 35 additions & 0 deletions app/src/main/java/com/techcourse/service/AppUserService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.techcourse.service;

import com.techcourse.dao.UserDao;
import com.techcourse.dao.UserHistoryDao;
import com.techcourse.domain.User;
import com.techcourse.domain.UserHistory;

public class AppUserService implements UserService {

private final UserDao userDao;
private final UserHistoryDao userHistoryDao;

public AppUserService(final UserDao userDao, final UserHistoryDao userHistoryDao) {
this.userDao = userDao;
this.userHistoryDao = userHistoryDao;
}

@Override
public User findById(final long id) {
return userDao.findById(id);
}

@Override
public void insert(final User user) {
userDao.insert(user);
}

@Override
public void changePassword(final long id, final String newPassword, final String createdBy) {
final var user = findById(id);
user.changePassword(newPassword);
userDao.update(user);
userHistoryDao.log(new UserHistory(user, createdBy));
}
}
62 changes: 62 additions & 0 deletions app/src/main/java/com/techcourse/service/TxUserService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package com.techcourse.service;

import com.techcourse.domain.User;
import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.datasource.DataSourceUtils;
import com.interface21.transaction.support.TransactionSynchronizationManager;

import javax.sql.DataSource;
import java.sql.Connection;
import java.sql.SQLException;

public class TxUserService implements UserService {

private final UserService userService;
private final DataSource dataSource;

public TxUserService(final UserService userService, final DataSource dataSource) {
this.userService = userService;
this.dataSource = dataSource;
}

@Override
public User findById(final long id) {
return userService.findById(id);
}

@Override
public void insert(final User user) {
userService.insert(user);
}

@Override
public void changePassword(final long id, final String newPassword, final String createdBy) {
final Connection connection = DataSourceUtils.getConnection(dataSource);
TransactionSynchronizationManager.bindResource(dataSource, connection);
Comment on lines +33 to +35
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

커넥션 획득 단계에서 예외가 발생하면 어떻게 될까요?

DataSourceUtils.getConnection()에서 커넥션을 얻은 직후 bindResource()를 호출하는데, 만약 bindResource() 과정에서 예외가 발생한다면 이미 획득한 커넥션이 정리되지 않을 수 있습니다.

커넥션 획득과 바인딩 과정도 리소스 관리 측면에서 안전하게 처리할 방법을 고려해보시면 좋을 것 같아요.


트랜잭션 관리 및 커넥션 리소스 핸들링을 검증하기 위해 실제 코드를 살펴보겠습니다.


---

🏁 Script executed:

```shell
#!/bin/bash
# Retrieve the complete TxUserService.java file to verify transaction handling
fd "TxUserService.java" --type f

Length of output: 110


🏁 Script executed:

#!/bin/bash
# Examine the changePassword method and surrounding transaction management code
cat app/src/main/java/com/techcourse/service/TxUserService.java

Length of output: 2408


커넥션 획득 및 바인딩 단계에서 리소스 누수 위험 있음

현재 코드를 살펴보니 33-35번 라인의 우려가 실제로 존재합니다. DataSourceUtils.getConnection() 또는 TransactionSynchronizationManager.bindResource()에서 예외가 발생하면, 이미 획득한 커넥션이 37번 라인부터 시작하는 try-finally 블록에 도달하지 않아 정리되지 않습니다.

예를 들어 bindResource()에서 예외가 발생한 경우:

  • 커넥션은 이미 획득된 상태
  • try 블록에 진입하지 못함
  • finally 블록의 unbindResource()와 releaseConnection()이 실행되지 않음
  • 결과적으로 커넥션 누수 발생

개선 방향:
커넥션 획득과 바인딩 과정도 try-catch로 감싸거나, 이 로직을 기존 try 블록 내부로 이동시켜 반드시 finally 블록이 실행되도록 구조화하는 것을 고려해보세요.

🤖 Prompt for AI Agents
In app/src/main/java/com/techcourse/service/TxUserService.java around lines
33-35, obtaining the Connection and calling
TransactionSynchronizationManager.bindResource() are outside the try-finally and
can leak the Connection if an exception is thrown before the finally executes;
wrap the DataSourceUtils.getConnection(...) and bindResource(...) calls inside
the existing try block (or surround them with their own try/catch that closes
the Connection on failure) so that any exception during acquisition or binding
triggers the finally cleanup path (unbindResource and
DataSourceUtils.releaseConnection) and guarantees the Connection is always
released.


boolean originalAutoCommit = false;
Copy link

@2Jin1031 2Jin1031 Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originalAutoCommit 의 초기값을 false로 두신 이유가 있는 지 궁금해요!
Connection 객체 생성 시 autoCommit의 기본값은 true 이기 때문에, 변경 여부를 추적하기 위한 변수라면 true 초기화하는게 더 자연스러워 보여서요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 저도 그게 맞는 것 같아요!!! 칼리 말대로 true로 수정하였습니다!!

try {
originalAutoCommit = connection.getAutoCommit();
connection.setAutoCommit(false);

userService.changePassword(id, newPassword, createdBy);

connection.commit();
} catch (Exception e) {
try {
connection.rollback();
} catch (SQLException rollbackException) {
throw new DataAccessException("Failed to rollback transaction", rollbackException);
}
throw new DataAccessException(e);
} finally {
try {
connection.setAutoCommit(originalAutoCommit);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 여기서 SQLException이 발생 시, unbind/release가 실행되지 않아서 리소스 누수가 발생할 가능성이 존재할 것 같아요
리소스 정리가 누락되지 않을 방법을 고려해봐도 좋을 듯 합니다~

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finally 블록 안에서 각각의 리소스 해제 작업을 개별 try-catch로 감싸서, 하나의 예외 발생이 나머지 해제 작업을 방해하지 않도록 리팩토링하였습니다!!
수정 commit: 53a75d5

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다~
throw 하면 정리가 스킵될 수 있으니, 예외는 로깅으로 처리해서 흐름을 이어가는 방향도 있을 것 같네요
추후 시간 나시면 반영해보셔도 좋을 듯합니다

} catch (SQLException e) {
throw new DataAccessException("Failed to reset autoCommit", e);
}
TransactionSynchronizationManager.unbindResource(dataSource);
DataSourceUtils.releaseConnection(connection, dataSource);
}
}
}
71 changes: 4 additions & 67 deletions app/src/main/java/com/techcourse/service/UserService.java
Original file line number Diff line number Diff line change
@@ -1,75 +1,12 @@
package com.techcourse.service;

import com.techcourse.dao.UserDao;
import com.techcourse.dao.UserHistoryDao;
import com.techcourse.domain.User;
import com.techcourse.domain.UserHistory;
import com.interface21.dao.DataAccessException;
import com.interface21.jdbc.datasource.DataSourceUtils;
import com.interface21.transaction.support.TransactionSynchronizationManager;

import javax.sql.DataSource;
import java.sql.Connection;
import java.sql.SQLException;
public interface UserService {

public class UserService {
User findById(final long id);

private final UserDao userDao;
private final UserHistoryDao userHistoryDao;
private final DataSource dataSource;
void insert(final User user);

public UserService(final UserDao userDao, final UserHistoryDao userHistoryDao) {
this.userDao = userDao;
this.userHistoryDao = userHistoryDao;
this.dataSource = null;
}

public UserService(final UserDao userDao, final UserHistoryDao userHistoryDao, final DataSource dataSource) {
this.userDao = userDao;
this.userHistoryDao = userHistoryDao;
this.dataSource = dataSource;
}

public User findById(final long id) {
return userDao.findById(id);
}

public void insert(final User user) {
userDao.insert(user);
}

public void changePassword(final long id, final String newPassword, final String createBy) {
if (dataSource == null) {
// 트랜잭션 없이 실행
executeChangePassword(id, newPassword, createBy);
return;
}

// 트랜잭션과 함께 실행
final Connection connection = DataSourceUtils.getConnection(dataSource);
try {
connection.setAutoCommit(false);

executeChangePassword(id, newPassword, createBy);

connection.commit();
} catch (Exception e) {
try {
connection.rollback();
} catch (SQLException rollbackException) {
throw new DataAccessException("Failed to rollback transaction", rollbackException);
}
throw new DataAccessException(e);
} finally {
TransactionSynchronizationManager.unbindResource(dataSource);
DataSourceUtils.releaseConnection(connection, dataSource);
}
}

private void executeChangePassword(final long id, final String newPassword, final String createBy) {
final var user = findById(id);
user.changePassword(newPassword);
userDao.update(user);
userHistoryDao.log(new UserHistory(user, createBy));
}
void changePassword(final long id, final String newPassword, final String createdBy);
}
14 changes: 10 additions & 4 deletions app/src/test/java/com/techcourse/service/UserServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ void setUp() {
@Test
void testChangePassword() {
final var userHistoryDao = new UserHistoryDao(jdbcTemplate);
final var userService = new UserService(userDao, userHistoryDao, DataSourceConfig.getInstance());
// 애플리케이션 서비스
final var appUserService = new AppUserService(userDao, userHistoryDao);
// 트랜잭션 서비스 추상화
final var userService = new TxUserService(appUserService, DataSourceConfig.getInstance());

final var newPassword = "qqqqq";
final var createBy = "gugu";
Expand All @@ -46,13 +49,16 @@ void testChangePassword() {
void testTransactionRollback() {
// 트랜잭션 롤백 테스트를 위해 mock으로 교체
final var userHistoryDao = new MockUserHistoryDao(jdbcTemplate);
final var userService = new UserService(userDao, userHistoryDao, DataSourceConfig.getInstance());
// 애플리케이션 서비스
final var appUserService = new AppUserService(userDao, userHistoryDao);
// 트랜잭션 서비스 추상화
final var userService = new TxUserService(appUserService, DataSourceConfig.getInstance());

final var newPassword = "newPassword";
final var createBy = "gugu";
final var createdBy = "gugu";
// 트랜잭션이 정상 동작하는지 확인하기 위해 의도적으로 MockUserHistoryDao에서 예외를 발생시킨다.
assertThrows(DataAccessException.class,
() -> userService.changePassword(1L, newPassword, createBy));
() -> userService.changePassword(1L, newPassword, createdBy));

final var actual = userService.findById(1L);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ public static Connection getConnection(DataSource dataSource) throws CannotGetJd
}

try {
connection = dataSource.getConnection();
TransactionSynchronizationManager.bindResource(dataSource, connection);
return connection;
return dataSource.getConnection();
} catch (SQLException ex) {
throw new CannotGetJdbcConnectionException("Failed to obtain JDBC Connection", ex);
}
Expand Down