-
Notifications
You must be signed in to change notification settings - Fork 309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[톰캣 구현하기 - 3, 4단계] 에밀(김대희) 미션 제출합니다. #436
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에밀 안녕하세요 리뷰가 늦어서 미안합니다 ㅠㅠ 😭
구조가 깔끔해져서 훨씬 읽기 좋았어요! 리뷰할 부분이 많지 않아서 열심히 찾았는데도 얼마 안 되네요... 😅
에밀과 이야기 해보고 싶은 부분이 있어서 Request Change 드렸습니다 :)
확인해보시고 혹시 이해가 안 되는 부분 있으시면 디엠 주세요 !
@@ -41,7 +41,7 @@ private static final class SynchronizedMethods { | |||
|
|||
private int sum = 0; | |||
|
|||
public void calculate() { | |||
synchronized public void calculate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사소하긴 한데 public synchronized void
로 하는 건 어떨까요? 😊
@@ -32,7 +32,6 @@ void test() throws Exception { | |||
|
|||
for (final var thread : threads) { | |||
thread.start(); | |||
Thread.sleep(50); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순 궁금증인데 sleep() 빼신 이유가 뭔가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
왜 재우는지 이유를 몰라서 제거해봤는데요. 안 재워도 잘 동작하더라구요. 아마 그때 그 상태로 둔 것 같습니다
} | ||
|
||
public Connector(final int port, final int acceptCount, final int maxThreads, | ||
Function<Socket, RunnableProcessor> runnableProcessorGenerator) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RunnableProcessor 를 실행할 수 있는 Container
클래스를 따로 만들지 않고, Function 으로 받으신 이유가 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 질문을 잘 이해한 건지 모르겠지만 함수형 인터페이스로 받은 사고의 흐름은
- 스레드에서 실행하기 위해 Runnable 인터페이스의 구현체여야 했습니다. 그래서 run() 메서드의 파라미터가 없는 관계로 생성자로 매번 다른 데이터를 받아야 했습니다.
- 그래서 매번 객체를 새로 생성해야 해서 생성자 자체를 필드로 가지고 있으면 좋겠다는 생각이 들었습니다.
- 해당 방식을 구현하는 방식 중에서 함수형 인터페이스가 가장 간단해보여서 해당 방식으로 구현했습니다.
Container로 구현하는 방식 궁금한데요 한 번 구경하러 가겠습니다 ㅎㅎ
this.serverSocket = createServerSocket(port, acceptCount); | ||
this.executorService = Executors.newFixedThreadPool(maxThreads); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 제 미션 진행할 때 이 부분을 가장 많이 고민했었는데요. 아마 아시겠지만 newFixedThreadPool
을 사용하게 되면 작업 큐의 범위가 INTEGER 의 최댓값까지 설정됩니다. 즉, 작업 큐에 처리되기를 기다리는 작업이 계속해서 쌓일 수 있게 됩니다.
이런 문제는 어떻게 해결할 수 있을까요? 에밀의 의견이 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전 이걸 모르고 있었습니다 ㅋㅋ
그래서 acceptCount가 알아서 잘 해주겠지 하고 있었는데 포이가 알려주더라구요. (역시 공식 문서와 테스트가 중요한 거 같습니다..)
알고보니 전부 큐에 쌓여서 acceptCount가 전혀 동작을 하지 않고 있었습니다.
그래서 세마포를 사용하기로 했습니다.
플로우는 다음과 같습니다.
- 세마포의 값을 maxThread로 해둡니다.
- 소켓의 accept를 호출하기 위해서는 세마포를 획득해야 합니다.
- 세마포를 획득하면 accept를 호출하고 요청이 들어오면 새로운 스레드를 할당받습니다.
- 세마포 획득에 실패하면 accept 호출이 실패하고 요청은 큐에 쌓입니다. 이때 acceptCount 이상으로 쌓이면 해당 요청은 거절됩니다.
- 스레드 사용이 끝나면 semaphore를 release 합니다.
- 반복
이런 플로우인데요. 실제로 생각한대로 동작하는 것을(와이어샤크로) 확인했습니다만 테스트를 짜보면 실패합니다. 그 이유는 tcp 프로토콜의 재전송 메커니즘 때문인데요.
큐에 안 쌓고 거절해버리니 tcp 측에서 timeout 되어서 syn 패킷을 계속 재전송 하더라구요. 그리고 제가 os의 큐에 접근할 수 있는 것도 아니라서..
해당 문제를 해결할 마땅한 방법이 안 떠오르네요
아 그리고 또 하나 문제가 있는데요
스레드가 풀에 반환되는 것을 확인하고 세마포를 릴리즈 해야 하는데요
스레드가 작업을 끝내고 풀에 반환되는 것을 확인하기가 어렵더라구요.
이벤트 혹은 프록시 같은 것을 써서 스레드가 끝나면 세마포를 릴리즈 하는 방식도 생각해봤는데 일단 가장 간단하게 구현해봤습니다.
아무래도 프로세서가 세마포를 받는 것이 이상하긴 합니다ㅎㅎ;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저 같은 경우는 세마포어 릴리즈를 CompletableFuture
로 해결했습니다! CompletableFuture
의 runAsync
와 whenCompleteAsync
를 함께 사용하면 해당 작업이 끝난 다음에 수행할 작업을 지정해줄 수 있어요.
아래 링크 참고하시면 좋을 것 같습니다 :)
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html
@@ -80,7 +95,7 @@ public void stop() { | |||
} | |||
|
|||
private int checkPort(final int port) { | |||
final var MIN_PORT = 1; | |||
final var MIN_PORT = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0으로 변경하신 이유는 뭘까요? 🧐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 Random port를 허용하기 위해서입니다.
port 값으로 0을 설정하면 현재 가용 상태인 포트 중에서 랜덤으로 하나 지정해주게 됩니다. 해당 메커니즘으로 테스트 진행했습니다.
@@ -23,7 +22,7 @@ private Cookies(List<Cookie> cookies) { | |||
public static Cookies from(Map<String, List<String>> header) { | |||
List<String> cookieValues = header.get(HeaderKey.COOKIE.value); | |||
if (cookieValues == null) { | |||
return Cookies.EMPTY_COOKIE; | |||
return Cookies.emptyCookies(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 팩토리 메서드로 바꾼 것 좋네요!!!
@@ -10,10 +10,13 @@ public class SessionManager { | |||
private SessionManager() { | |||
} | |||
|
|||
private static final Map<String, Session> sessions = new HashMap<>(); | |||
private static final Map<String, Session> sessions = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashMap
과 ConcurrentHashMap
은 어떤 차이가 있을까요? 추가적으로 ConcurrentHashMap
으로 바꾸었을 때 어떤 이점이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concurrentHashMap은 동시성 제어를 보장해주는 것으로 알고 있습니다.
sessions가 아무래도 공유자원이다보니 동시에 값을 변경하다보면 문제가 발생할 수 있습니다.
이런 문제를 고수준으로 지원해주는 hashmap 자료구조가 바로 concurrenthashmap으로 알고 있어요.
단, 아무래도 충돌이 발생하는 것을 검사하고 조율해야하다보니 성능적 문제가 있는 걸로 알고 있습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다! 잘 알고 계시는군요 👍
@@ -10,10 +10,13 @@ public class SessionManager { | |||
private SessionManager() { | |||
} | |||
|
|||
private static final Map<String, Session> sessions = new HashMap<>(); | |||
private static final Map<String, Session> sessions = new ConcurrentHashMap<>(); | |||
|
|||
public static Session findSession(String sessionId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름은 findSession
인데 안에서 sessionId
가 null 인 경우 새로운 Session 을 생성해주고 있네요!
사용자가 이름으로부터 유추하기 힘든 메서드 내부 구현인 것 같은데, 에밀은 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 동의합니다ㅎㅎ
findOrCreate 정도의 이름 정도로 하는 것이 좋겠네요!
Kudos, SonarCloud Quality Gate passed! 0 Bugs 0.0% Coverage The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 반영하시느라 수고 많으셨습니다!
에밀이 언급해주셨던 테스트 관련한 부분은 저도 해결하지 못한 부분이라... 리뷰 남겨드리진 못했어요 죄송합니다 😂
리뷰는 간단하게 남겼는데, 시간 되실 때 천천히 반영해주시면 될 것 같습니다. 미션에 대해 좀 더 이야기 나눠보고 싶으시면 디엠으로 연락주셔도 됩니다!
톰캣 미션 수고 많으셨습니다~~
@@ -10,10 +10,13 @@ public class SessionManager { | |||
private SessionManager() { | |||
} | |||
|
|||
private static final Map<String, Session> sessions = new HashMap<>(); | |||
private static final Map<String, Session> sessions = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다! 잘 알고 계시는군요 👍
this.serverSocket = createServerSocket(port, acceptCount); | ||
this.executorService = Executors.newFixedThreadPool(maxThreads); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저 같은 경우는 세마포어 릴리즈를 CompletableFuture
로 해결했습니다! CompletableFuture
의 runAsync
와 whenCompleteAsync
를 함께 사용하면 해당 작업이 끝난 다음에 수행할 작업을 지정해줄 수 있어요.
아래 링크 참고하시면 좋을 것 같습니다 :)
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CompletableFuture.html
|
||
private final Socket connection; | ||
private final HttpControllers httpControllers; | ||
private final Semaphore semaphore; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompletableFuture
를 사용하게 되면 세마포어를 Processor 가 들고있지 않아도 될 것 같네요 !
import org.apache.coyote.http.HttpHeader; | ||
import org.apache.coyote.http.HttpHeaderConverter; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class HttpRequestDecoder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분 리뷰를 잠깐 놓쳤었는데, Decoder 는 유틸성이 높은 클래스처럼 보여요!
각 메서드들을 static 으로 두지 않고 인스턴스 메서드로 정의한 이유가 있을까요?
안녕하세요~ 베로
미션 리뷰 반영하고 3, 4단계 진행해보았습니다 ㅎㅎ
저번 리뷰 도움 많이 받았습니다
이번에도 잘 부탁드립니다^^