[톰캣 구현하기 3, 4단계] 로이 미션 제출합니다. #494
Conversation
| executorService.execute(processor); | ||
| new Thread(processor).start(); |
There was a problem hiding this comment.
이 부분은 중요한 부분이어서 말씀드려요!
newFixedThreadPool로 스레드 풀을 만들어서 사용하신 것은 적잘한데
대신 두 번째 줄의 new Thread(processor).start();는 지워주셔야 합니다
혹시 필요하실까봐 부가적으로 설명드리자면 현재 execute 메서드와 Thread의 생성자로 넘기는 파라미터인 processor는 Runnable 인터페이스의 구현체인데요, 지금 구조에선 스레드 풀에서 할당한 스레드와 이 process() 메서드에서 new Thread()로 만들어내는 스레드, 총 두 개의 스레드가 같은 Http11Processor의 run()을 실행시키게 됩니다!
이로 인해 이렇게 하나의 요청이 올 때마다 pool에서 할당한 thread([pool-1-thread-6])와 직접 생성한 thread([Thread-6])가 모두 같은 객체를 가지고 처리를 시도,
스레드 풀의 스레드가 먼저 처리하여 직접 만든 스레드는 `Http11Processor`의 process()에서 `IllegalArgumentException`이 뜨네요. (확실하진 않지만 OutputStream을 flush() 하면 socket이 닫혀 그런 게 아닐까 싶습니다...)
무튼 같은 요청을 두 스레드에서 처리하려고 하니 늦은 스레드는 예외가 발생한다고 보면 되겠네요~!
There was a problem hiding this comment.
사실 쓰레드 작동 원리에 관해서 제대로 이해하지 못한 채 구현하다 보니
이런 문제가 있었네요ㅠ
잘못된 부분을 알려주신 것 뿐만 아니라, 그 내용까지 상세히 알려주셔서 너무 감사합니다!
말씀하신대로 수정해봤구요! 이번 계기로 스레드에 대한 공부를 깊이 있게 좀 해봐야겠네요!!
| try { | ||
| executorService.shutdown(); | ||
| terminateWhenTimeLimitExceed(); | ||
| serverSocket.close(); | ||
| } catch (IOException e) { | ||
| log.error(e.getMessage(), e); | ||
| } catch (InterruptedException e) { | ||
| executorService.shutdownNow(); | ||
| log.error(e.getMessage(), e); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } |
There was a problem hiding this comment.
스레드 풀을 닫는 부분 고민해주신거 너무 좋네요~~ 저도 배웠습니다!
| @@ -16,12 +16,12 @@ private Cookie(Map<String, String> cookies) { | |||
|
|
|||
| public static Cookie from(String cookie) { | |||
| String[] params = cookie.split("; "); | |||
| HashMap<String, String> cookies = initCookies(params); | |||
| ConcurrentHashMap<String, String> cookies = initCookies(params); | |||
There was a problem hiding this comment.
동시성까지 고민해보신 것 같네요~
ConcurrentHashMap은 내부적으로 put()같이 쓰기 작업에서 synchronized블록으로 동기화를 진행하는데요, 필요하지 않은 곳에 이런 concurrent 자료구조를 사용하게 되면 미미하긴 하지만 synchronized 처리에 시간이 추가로 소요됩니다.
저의 경우에는 여러 스레드에서 공용으로 접근하는 InMemoryUserRepository객체는 concurrentHashMap의 사용이 필요하지만 Cookie같이 하나의 response 객체에 할당되는 객체 내부에서는 그러할 필요가 없지 않나 싶어요!
동시성 이슈는 다양한 스레드가 접근할 때 문제가 되는데 현재 요청을 처리하며 Response를 만들어내는 과정은 온전히 하나의 스레드에서 담당하지 않을까요? 이런 부분도 고민에 포함하시면 스레딩에 더욱 익숙해지지 않을까 싶어요😄
There was a problem hiding this comment.
말씀을 듣고보니 공용으로 사용되지 않는 Cookie에는 Concurrent 자료 구조를 적용할 필요가 없다는 생각이 드네요!!
좋은 고민거리를 주셔서 감사합니다!!
|
|
||
| public class SessionManager { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(SessionManager.class); | ||
|
|
||
| private static final Map<String, Session> SESSIONS = new HashMap<>(); | ||
| private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>(); |
|
|
||
| public class Connector implements Runnable { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(Connector.class); | ||
|
|
||
| private static final int DEFAULT_PORT = 8080; | ||
| private static final int DEFAULT_ACCEPT_COUNT = 100; | ||
| private static final int DEFAULT_MAX_THREADS = 250; |
There was a problem hiding this comment.
완전히 부가적인 부분인데요,
톰캣의 경우 기본 설정에서 IDLE 상태에선 10개의 thread, 최대 200개까지 유동적으로 바뀌게 됩니다!
현재 250이라는 숫자로 지정하면 사용하는 newFixedThreadPool은 IDLE에서도 250개, 최대에도 250개가 할당됩니다~
| @@ -56,7 +62,10 @@ private boolean checkInputForm(final HashMap<String, String> registerData) { | |||
| private HttpResponse generateBadRequestResponse() throws URISyntaxException, IOException { | |||
| Path path = PathFinder.findPath("/400.html"); | |||
There was a problem hiding this comment.
혹시 400.html은 별도로 존재했다가 삭제된 정적 파일일까요?
There was a problem hiding this comment.
샘플 코드로 주어진 것으로 잘못 알았네요ㅜ
400에러 페이지를 추가해봤습니다!
세심한 피드백 감사합니다👍
| @@ -33,6 +33,7 @@ protected HttpResponse doPost(HttpRequest request) throws Exception { | |||
| private HttpResponse defaultInternalServerErrorPage() throws URISyntaxException, IOException { | |||
| final Path path = PathFinder.findPath("/500.html"); | |||
| final String responseBody = new String(Files.readAllBytes(path)); | |||
| return new HttpResponse(HttpStatus.INTERNAL_SERVER_ERROR, responseBody, ContentType.HTML); | |||
|
|
|||
| return new HttpResponse.Builder(HttpStatus.INTERNAL_SERVER_ERROR, responseBody, ContentType.HTML).build(); | |||
There was a problem hiding this comment.
3단계 요구사항이 Controller의 service()메서드의 시그니처가 service(HttpRequest request, HttpResponse response)이런 식으로 강제되게 되었네요 ㅠㅠ 이런 방식으로 가게 되면 response를 외부에서 만들어 넣어주는 방식으로 구성해야 하고 Servlet과 HttpServlet의 정확한 표준을 따르게 되면 제안드렸던 builder패턴 사용으로 response 객체를 나중에 만드는 방식은 조오금 어려워질 것 같기도 합니다..😂
고치기에는 변경점이 많아서 가볍게 어떻게 바뀔지 고민만 해보시면 좋을 것 같아서요~
There was a problem hiding this comment.
일단 빌더 패턴을 한번 적용해보는 것 부터가 많은 공부가 되었습니다😀
말씀하신 것 처럼 service(HttpRequest request, HttpResponse response) 로 구성하는 게 올바른 리팩터링 방향이라고 생각합니다!
기한 내 적용은 못했지만.. 나중에 여유를 좀 가지고 천천히 구조 변경을 해보겠습니다!
|
Kudos, SonarCloud Quality Gate passed!
|
안녕하세요 모디!
중요한 내용 위주로 코멘트를 주셔서 정말 감사합니다!
코멘트 주신 내용(구조 변경) 중에 아직 반영하지 못한 내용이 있는데요.
해당 내용은 최대한 빨리 반영해보겠습니다!
감사합니다!