Conversation
develop <- feat/http/cookie/#41
- SingletonContainer 추상 클래스 정의 - 제네릭을 사용한 싱글톤 컨테이너 구축
codingbaraGo
added
the
refactor
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 코드 리뷰 완료
PR의 핵심 리뷰 포인트 **"멀티스레드 미고려"**를 중심으로 분석했습니다.
📋 주요 문제점
-
정적 맵의 상속 문제:
SingletonContainer의 정적 맵이 모든 상속 클래스에서 공유되어, 향후 확장 시 예상치 못한 동작 가능. -
타입 안전성 부족: 문자열 기반 key와 unchecked 캐스트로 인해 런타임에 타입 불일치 가능성.
-
컬렉션 싱글톤의 불변성 미보장: 반환된 List를 외부에서 수정하면 싱글톤이 오염될 위험.
-
멀티스레드 미처리: 현재 구조에서는 race condition 가능성이 있으므로, 설계 제약을 명시적으로 문서화 필요.
-
메서드 가시성 변경의 캡슐화 손상: 모든 메서드가 public이 되어 불필요한 인터페이스 노출.
✅ 개선 권장사항
각 인라인 코멘트에서 구체적인 수정 방안을 제시했습니다. 향후 지원 복잡도와 유지보수성을 높이기 위해 검토해주세요.
| import java.util.Map; | ||
| import java.util.function.Supplier; | ||
|
|
||
| public abstract class SingletonContainer { |
There was a problem hiding this comment.
정적 맵을 abstract 클래스에서 사용하면 상속 계층 구조 전체에서 공유된다는 점에 주의: 현재 설계에서는 단일 AppConfig만 있으므로 문제가 없지만, 향후 SingletonContainer를 상속받는 다른 설정 클래스가 생기면 모든 싱글톤이 같은 맵에 저장됩니다.
향후 확장성을 위해 다음 중 하나를 고려하세요:
- 인스턴스 필드로 변경: 각 설정 클래스가 독립적인 맵을 유지
- 생성자 매개변수화: 맵 이름이나 식별자를 받아 구분
- AppConfig 전용 구현: SingletonContainer를 상속받지 말고, AppConfig에 직접 구현하여 현재 요구사항에 맞춤
| public abstract class SingletonContainer { | ||
| private static final Map<String, Object> singletonMap = new HashMap<>(); | ||
|
|
||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
형변환 경고 억제(@SuppressWarnings)는 타입 안전성을 가릴 수 있습니다: 문자열 기반 key로 인해 런타임 타입 불일치가 발생할 수 있습니다.
예: getOrCreate("httpRequestResolver", SomeOtherClass::new)라고 잘못 호출하면 나중에 캐스트 예외가 발생합니다.
개선 방안: Map<String, Class<?>> 또는 더 타입-안전한 구조를 사용하거나, 최소한 저장된 타입과 요청된 타입이 일치하는지 검증하는 로직을 추가하세요.
| import web.renderer.HttpResponseRenderer; | ||
| import web.renderer.StaticViewRenderer; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| public class AppConfig { | ||
| //Http | ||
| public HttpRequestConverter httpRequestConverter(){ | ||
| public class AppConfig extends SingletonContainer { |
There was a problem hiding this comment.
기존 동작과 달라진 점: httpRequestConverter()와 httpResponseConverter()는 단순히 다른 메서드를 호출하기만 하므로, getOrCreate로 감싸지 않았습니다. 이로 인해 매번 같은 객체를 반환하지만 리팩토링 의도와 불일치합니다.
일관성을 위해 명확히 하세요:
- 옵션 1: 이 두 메서드도
getOrCreate로 감싸기 (싱글톤 명시적 보장) - 옵션 2: 이름 변경 또는 문서화: "이 메서드들은 캐시되지 않고 delegation만 수행" 표시
|
|
||
| private List<WebHandler> webHandlerList(){ | ||
| return List.of( | ||
| staticContentHandler(), | ||
| registerWithGet(), | ||
| registerWithPost() | ||
| /** | ||
| * ===== Web ===== | ||
| */ | ||
| public Dispatcher dispatcher() { | ||
| return getOrCreate( | ||
| "dispatcher", | ||
| () -> new Dispatcher( | ||
| webHandlerList(), | ||
| handlerAdapterList(), | ||
| webHandlerResponseHandlerList() |
There was a problem hiding this comment.
컬렉션 반환 메서드의 싱글톤 캐싱 주의: webHandlerList() 같은 컬렉션을 반환하는 메서드를 getOrCreate로 캐싱하면, 반환된 List가 변경되지 않는다고 가정합니다.
만약 런타임에 이 리스트를 수정하면 (예: add(), remove()) 싱글톤 인스턴스 자체가 변경됩니다. 이를 방지하려면:
- 불변 리스트 반환:
Collections.unmodifiableList(...) - 복사본 반환: 매번 새로운 컬렉션 인스턴스 생성
- 내부 리스트 접근 금지 (private 필드화)
| () -> List.of( | ||
| singleArgHandlerAdapter(), | ||
| defaultHandlerAdapter() | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| private SingleArgHandlerAdapter singleArgHandlerAdapter(){ | ||
| return new SingleArgHandlerAdapter( | ||
| argumentResolverList() | ||
| public SingleArgHandlerAdapter singleArgHandlerAdapter() { | ||
| return getOrCreate( | ||
| "singleArgHandlerAdapter", | ||
| () -> new SingleArgHandlerAdapter( | ||
| argumentResolverList() | ||
| ) | ||
| ); | ||
| } | ||
| private DefaultHandlerAdapter defaultHandlerAdapter(){ | ||
| return new DefaultHandlerAdapter(); | ||
|
|
||
| public DefaultHandlerAdapter defaultHandlerAdapter() { | ||
| return getOrCreate( |
There was a problem hiding this comment.
싱글톤 의존성 그래프 복잡성: 예를 들어, exceptionHandlerMapping()은 serviceExceptionHandler(), errorExceptionHandler(), unhandledErrorHandler()에 의존하는데, 이들이 모두 싱글톤으로 캐싱되므로 의존성 순서가 중요합니다.
현재는 서버 구동 시점에만 로드되므로 문제 없지만, 향후 런타임 재로드나 부분 초기화가 필요하면 의존성 추적이 어려워집니다. 필요시 의존성 명시를 위해 생성자 주입 패턴으로 마이그레이션을 고려하세요.
| "webHandlerList", | ||
| () -> List.of( | ||
| staticContentHandler(), | ||
| registerWithGet(), | ||
| registerWithPost() | ||
| ) | ||
| ); | ||
| } | ||
| private RegisterWithPost registerWithPost(){ | ||
| return new RegisterWithPost(); | ||
|
|
||
| public RegisterWithGet registerWithGet() { | ||
| return getOrCreate( | ||
| "registerWithGet", | ||
| RegisterWithGet::new | ||
| ); |
There was a problem hiding this comment.
메서드 가시성 변경 (private → public)의 영향: 기존 private 메서드들이 모두 public으로 변경되어, 외부에서 직접 호출할 수 있게 되었습니다.
예를 들어, registerWithGet()을 외부에서 두 번 호출하면:
AppConfig config = new AppConfig();
RegisterWithGet h1 = config.registerWithGet(); // 첫 호출: 캐시 저장
RegisterWithGet h2 = config.registerWithGet(); // 두 번째: 동일 인스턴스 반환이는 싱글톤 보장 측면에서는 긍정적이지만, 캡슐화를 깨뜨립니다. 필요한 진입점만 public으로 유지하고 나머지는 protected 또는 패키지 private으로 제한하는 것을 검토하세요.
1 participant
💻 작업 내용
✨ 리뷰 포인트
DI 로딩은 서버 구동 시점에 완료되는 구조라 멀티스레드를 고려하지 않았습니다.
🎯 관련 이슈
closed #44