[톰캣 구현하기 4단계] 메리(최승원) 미션 제출합니다. #476
Conversation
| } | ||
|
|
||
| public void stop() { | ||
| stopped = true; | ||
| try { | ||
| serverSocket.close(); | ||
| executorService.shutdown(); |
There was a problem hiding this comment.
저는 shutdown에 대해 몰랐는데, 찾아보니 현재 스레드풀에 있는 스레드는 그대로 놔두고, 더 이상 submit을 받지 않는다라고 해요.
shutdown의 경우에는 non-blocking이라 stop()이 호출되는 즉시 어플리케이션 자체가 종료될텐데,
이 경우 사용자의 요청이 처리되다가 끊길 수도 있으므로 데이터가 저장되다가 마는 등의.. 시나리오가 생길 수도 있을 것 같아요.
따라서 executorService.awaitTermiation 메소드를 통해 실행중인 스레드들은 작업을 모두 완료하고 종료할 수 있게 하면 어떨까 싶어요!
변경을 요청드리는 건 아니고, '이렇게 스레드 풀을 종료시킬 수도 있겠구나' 정도만 알고 넘어가시면 좋을 것 같아요.
관련해서 잘 정리되어 있는 아티클 첨부해드립니다!
| import org.apache.coyote.http11.http.message.HttpResponse; | ||
| import org.apache.coyote.http11.http.util.HttpMethod; | ||
|
|
||
| public abstract class AbstractController implements Controller { |
There was a problem hiding this comment.
추상클래스를 통해 Controller 중복 로직을 제거해주셨군요 👍
| throw new IllegalArgumentException("잘못된 메소드 형식입니다. " + method); | ||
| } | ||
|
|
||
| protected void doPost(HttpRequest request, HttpResponse response) throws Exception { |
There was a problem hiding this comment.
Override하는 용도이니, protected가 아닌 abstract method로 선언해도 좋을 것 같아요.
| public interface Controller { | ||
|
|
||
| HttpResponse run(final HttpRequest request) throws IOException; | ||
| void service(final HttpRequest request, final HttpResponse response) throws Exception; |
| this.messageHeaders = messageHeaders; | ||
| this.messageBody = messageBody; | ||
| public HttpResponse() { | ||
| this.messageHeaders = new HashMap<>(); |
There was a problem hiding this comment.
현재 HashMap을 사용해서 Header를 구성하고 있는데, 이 경우 순서 보장이 되지 않아서 테스트가 어쩔때는 통과하고, 어쩔 때에는 실패하는 것 같아요.
물론 LinkedHashMap을 사용해서 순서 보장을 하는 것도 방법이겠지만, 헤더는 순서가 상관이 없으므로 현재 로직 자체는 문제가 없다는 생각이 드네요.
테스트 코드에서 String 값을 통으로 비교하게 하는 게 아니라, assertThat(HTTP메세지).contains(헤더);
이런 식으로 검증하도록 변경하면 항상 성공하는 테스트가 될 것 같습니다!
There was a problem hiding this comment.
말씀해주신 부분은 고려를 못했네요ㅠ
그리고 헤더는 순서가 상관이 없다는 것에도 동의해 테스트 코드를 수정해두었습니다
감사합니다!
|
|
||
| public class SessionManager implements Manager { | ||
|
|
||
| private static final Map<String, HttpSession> SESSIONS = new HashMap<>(); | ||
| private static final ConcurrentMap<String, HttpSession> SESSIONS = new ConcurrentHashMap<>(); |
| } | ||
|
|
||
| private HttpResponse getLoginPage(final HttpRequest request) { | ||
| public void doGet(final HttpRequest request, final HttpResponse response) { | ||
| if (request.containsCookie()) { |
There was a problem hiding this comment.
이 부분은 제가 저번 리뷰 때 놓친 것 같은데, 쿠키가 있는지로만 로그인 여부를 판단하고 있네요.
JSESSIONID가 아닌 다른 쿠키가 있어도 login 페이지에 접근할 수 없는데, JSESSIONID로 판단하도록 변경해야 할 것 같아요!
| outputStream.flush(); | ||
| } catch (IOException | UncheckedServletException e) { | ||
| log.error(e.getMessage(), e); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
커밋 로그에 없어서 학습테스트를 하셨는지는 모르겠지만,
혹시 안하셨다면 추후 Thread 학습테스트도 꼭 진행해보셨으면 좋겠습니다~
개인적으로 얻어갈게 많다고 느껴졌었어요!
|
안녕하세요 메리! 수정할만한 부분은 거의 없었던 것 같고, 자잘한 부분에 대해서만 피드백 남겼습니다. 현재 구조도 되게 깔끔한 것 같아서, 제가 생각하기에는 큰 변화가 필요하지 않은 거 같아요. 고생하셨습니다~ 마지막 request change이니, 수정하시고 리뷰 요청하시면 머지하도록 하겠습니다! 🙂 |
|
안녕하세요 테오! 프로젝트와 관련해 급하게 해결해야할 일이 있어 리뷰 요청이 많이 늦어졌어요ㅠ.. 좋은 리뷰 많이 남겨주셔서 감사합니다!! |
|
Kudos, SonarCloud Quality Gate passed!
|
안녕하세요 테오!
3단계에서 요구사항과 다르게 구현되었던 Controller 수정 및 AbstractController를 추가하였고,
4단계 요구사항 적용 완료했습니다.
큰 변동사항은 작성한 두 가지 외에는 없는 것 같습니다.
지난번 코드에서 많이 리팩토링은 많이 부족한 것 같네요.....ㅠㅠ
리뷰해주셔서 감사합니다!!
이번 리뷰도 잘 부탁드리겠습니다