[톰캣 구현하기 3 & 4단계] 홍고(홍여진) 미션 제출합니다 #483

hgo641 · 2023-09-11T07:41:36Z

안녕하세요, 포이!
너......무......... 부족한 코드지만 잘 부탁드립니다ㅠㅠ
테스트도 이번에 완성해오겠다고 했는데 약속을 지키지 못해 죄송합니다

변경된 부분은 크게 두 가지 있습니다!

HandlerMapper내부의 메소드들을 Cotroller인터페이스의 구현체로 변경
파일을 읽는 StaticFile 클래스 생성

poi1649

홍고 안녕하세요! 전체적으로 패키지 분리를 열심히 해주신게 굉장히 눈에 띄네요! 특히 HandlerMapper 가 ServletContainer 처럼 활용되도록 설정하여 의존을 끊어주신 부분이 인상깊었습니다!

새로운 미션도 진행되고 할 일이 엄청 많으실 것 같아 머지를 할까 고민했는데요.. 특정 부분에서 의도하신대로 작동하지 않는 코드일 수 있을 것 같아 한 번은 리퀘스트 체인지 남깁니다! 혹시 제가 홍고의 의도를 잘못이해한거라면 수정하지 않으시고 커멘트로 내용만 공유해주시면 감사하겠습니다! 오늘도 좋은 하루 보내세요!

poi1649 · 2023-09-12T02:16:14Z

tomcat/src/main/java/nextstep/jwp/controller/LoginController.java

+ protected void doPost(final Request request, final Response response) {
+ final Map<String, String> requestForms = request.getRequestForms().getFormData();
+ final Optional<User> user = login(requestForms.get(ACCOUNT), requestForms.get(PASSWORD));
+ if (user.isPresent()) {


혹시 isPresentOrElse() 대신 if 분기문으로 처리해주신 이유가 있나용?

ifPresentOrElse() 가 있는지 몰랐네요!!! 적용했습니다 코드가 깔끔해졌네요! 👍

poi1649 · 2023-09-12T02:26:58Z

tomcat/src/main/java/nextstep/jwp/controller/LoginController.java

+
+ @Override
+ protected void doGet(final Request request, final Response response) throws Exception {
+ if (request.getSessionValue(USER) != Optional.empty()) {


request.getSessionValue(USER).isEmpty() 처럼 옵셔널 내부 메서드로 검사해주는게 더 옵셔널스럽게 사용하는 방법 같네요! 그러면 null 비교에 비해 이점을 확실하게 가져갈 수 있을 것 같아요😎

request.getSessionValue(USER).isEmpty() 너무 좋아요~~~ 다른 Optional들도 동일하게 수정했습니다!

poi1649 · 2023-09-12T04:30:06Z

tomcat/src/main/java/nextstep/jwp/controller/RootController.java

+
+ @Override
+ protected void doPost(final Request request, final Response response) throws Exception {
+ throw new UnsupportedRequestException();


커스텀 예외 멋지네요👍

poi1649 · 2023-09-12T04:31:32Z

tomcat/src/main/java/nextstep/jwp/controller/RootController.java

+ protected void doGet(final Request request, final Response response) throws Exception {
+ final Response createdResponse = Response.createByResponseBody(HttpStatus.OK,
+ new ResponseBody(ROOT_MESSAGE, ContentType.HTML));
+ response.setBy(createdResponse);


setBody, setStatus 대신 response 객체 자체를 새로 만들어주신 이유를 공유해주실 수 있나요?

기존에 유효한 정보(status, body)들을 받아 Response를 생성해주는 로직이 존재했는데, 3단계 요구사항에서 doGet(final Request request, final Response response) 메소드가 생기면서 Response를 무조건 디폴트값으로 초기화하게 되었습니다.
기존에 생성한 유효한 정보(status, body)들을 받아 Response를 생성해주는 로직을 그대로 재활용하기 위해 setBy()를 생성했습니다...ㅎㅎ

물론 사용하지 않을 복사용 Response 객체가 생기긴 하지만...ㅎㅎ 이 정도는 괜찮지 않을까 했어요...ㅎㅎ
혹시 마음에 걸리는 부분이 있으신가요?

저도 큰 차이는 없다고 생각하는데 혹시 제가 생각못한 장단점이 있을까해서 여쭤봤습니다! 🫡

일반적인 서블릿 프로그래밍과 약간의 차이가 있다면 공통 헤더를 처리해주는 filter들을 따로 두어 doFilter 메서드로 리스폰스에 필요한 것들을 계속해서 set해주는 과정으로 가져가는 것 정도 같네요!

poi1649 · 2023-09-12T04:42:39Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

 this.serverSocket = createServerSocket(port, acceptCount);
 this.stopped = false;
- this.handlerMapper = handlerMapper;
+ this.controllerMapper = controllerMapper;
+ this.executorService = Executors.newFixedThreadPool(maxThreads);
 }


Executors 클래스로 파일을 스레드풀을 생성하게되면 unbounded한 작업 큐가 생성되어 의도하신대로 100개의 작업만 대기시킬수는 없을 것 같네요! accept count 를 활용해 100개의 작업을 대기시키려면 다른 방법을 써야할 것 같아요!

헉 그렇네요! 스레드 고수 포이 감사합니다!!
new ThreadPoolExecutor를 사용해서 queue의 사이즈를 100으로 지정해주었습니다!

오 queue 를 사용하셨군요. 구구는 Executors 안의 메서드를 사용해서 만들어보라고 하셨지만 저도 이 방법이 제일 속편한 것 같아요ㅋㅋㅋㅋ.. ....

그런데 사실 현재 구조라면 socket의 accept-count 수 만큼 백로그에서 추가로 더 대기하게 됩니다. 스레드 작업큐 100개 + 백로그 100개 총 200개의 커넥션이 대기하게 되겠네요.
또, 두 가지 영역을 함께 사용하면 관리도 번거로워질 수 있어요!
백로그에 있는 커넥션은 타임아웃시 계속 재전송을 요청(tcp의 기능)하는 반면, 스레드에 있는 커넥션은 어플리케이션 로직에 따라 다르게 처리됩니다.

poi1649 · 2023-09-12T04:43:50Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

 }

 public void stop() {
 stopped = true;
 try {
 serverSocket.close();
- } catch (IOException e) {
+ executorService.shutdown();


오.. shutdown 꼼꼼하시군요! 혹시 awaitTermination 말고 shutdown만 쓰신 이유를 여쭤볼 수 있을까요?

awaitTermination에 대해 알지 못했습니다ㅠㅠ shutdown만 하면 task가 종료되기도 전에 shutdown을 호출한 메인 스레드가 먼저 종료될 수도 있겠군요. awaitTermination을 함께 사용해서 task가 종료되는 것을 60초동안 기다리게 변경했습니다!
+ InterruptedException처리도 해주는 게 좋다고 해서 추가했습니다!

👍👍 쓰레드는 신경쓸게 참 많은 것 같아요 ㅋㅋㅋㅋ

poi1649 · 2023-09-12T04:57:07Z

tomcat/src/main/java/org/apache/coyote/http11/message/StaticFile.java

+ final URL url = classLoader.getResource(STATIC_ROOT_PATH + path);
+ try {
+ return new String(Files.readAllBytes(new File(url.getFile()).toPath()));
+ } catch (final NullPointerException e) {


getFile() 을 사용하면 없는 경로의 경우 emptyString을 반환하고, new File 에서 빈 문자열이 들어오면 새로운 파일을 반환한다고 하네요. 이 경우 NPE가 발생하지 않을 것 같아요. files.exist() 라는 메서드가 있던 것 같은데 해당 메서드를 활용해봐야할 것 같기도 해요

classLoader.getResource()에서 리소스가 존재하지 않으면 url이 Null로 할당됩니다!
Null.getFile()이 되면서 NPE가 발생하더라고요.

return new String(Files.readAllBytes(new File(url.getFile()).toPath())); 에서 catch를 하고 있어 로직을 파악하기 힘들 것 같네요! url이 null이면 new File까지 가지 않고 바로 예외를 반환하게 변경하겠습니다. 감사합니다!!

앗 위 classLoader에서 먼저 null 을 반환하는군요 혼란을 드려 죄송합니다 ㅋㅋㅋㅋㅋ...🙇‍♂️

확실히 수정하신 로직이 더 좋아보이긴 하네요!

poi1649 · 2023-09-12T05:00:25Z

tomcat/src/main/java/org/apache/coyote/http11/SessionManager.java

+
+public class SessionManager {
+
+ private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();


https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names

저도 구구의 리뷰에서 발견한건데 구글 스타일 컨벤션에서 deeply Immutable 해야만 상수라고 설명하고 있네요. 조금 생각해보셔도 좋아보입니다!

갸아악 감사합니다

sonarcloud · 2023-09-13T10:47:25Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

0.0% Coverage
0.0% Duplication

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.
Read more here

poi1649

홍고 안녕하세요! 이것저것 바쁘실텐데 고생많으셨습니다. 🫡
이번 미션은 이만 머지해도 괜찮을 것 같아요!
정말 마지막으로 조금의 커멘트를 남겼는데 시간나실때 편하게 확인해주시면 감사하겠습니다!
다음 미션도 화이팅입니다!!

hgo641 added 11 commits September 10, 2023 21:49

refactor: 정적 파일을 읽는 로직을 클래스로 분리

7582b78

refactor: ResponseBody를 클래스로 분리

3719d7e

feat: Controller 생성

8da9f57

refactor: HandlerMapper를 ControllerMapper로 대체

3a9ee9e

test: thread 실습 테스트

b936de3

refactor: HttpMethod를 상수로 분리

d2258d0

refactor: 매직넘버 상수로 분리

06b723d

refactor: 커스텀 예외 생성

981f06a

refactor: 패키지 변경

f31be2d

refactor: final 추가

64a8acc

test: RootController 할당

d73bedc

hgo641 requested a review from poi1649 September 12, 2023 02:13

hgo641 self-assigned this Sep 12, 2023

poi1649 requested changes Sep 12, 2023

View reviewed changes

hgo641 added 6 commits September 13, 2023 12:22

style: 코드 스타일 적용

d87febe

refactor: URL이 null이면 바로 예외를 던지게 변경

078f8ed

test: StaticFile 테스트 추가

a3a1f28

refactor: null객체 Optional로 변경 및 Optional 메소드 활용

f1433c9

refactor: awaitTermination을 사용해 shutdown하게 변경

45136ef

refactor: 스레드풀의 큐 사이즈 지정

a8091b5

poi1649 approved these changes Sep 13, 2023

View reviewed changes

poi1649 merged commit fed4fdc into woowacourse:hgo641 Sep 13, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 3 & 4단계] 홍고(홍여진) 미션 제출합니다 #483

[톰캣 구현하기 3 & 4단계] 홍고(홍여진) 미션 제출합니다 #483

hgo641 commented Sep 11, 2023 •

edited

Loading

poi1649 left a comment

poi1649 Sep 12, 2023

hgo641 Sep 13, 2023

poi1649 Sep 12, 2023

hgo641 Sep 13, 2023

poi1649 Sep 12, 2023

poi1649 Sep 12, 2023

hgo641 Sep 13, 2023

poi1649 Sep 13, 2023

poi1649 Sep 12, 2023

hgo641 Sep 13, 2023

poi1649 Sep 13, 2023 •

edited

Loading

poi1649 Sep 12, 2023 •

edited

Loading

hgo641 Sep 13, 2023 •

edited

Loading

poi1649 Sep 13, 2023

poi1649 Sep 12, 2023

hgo641 Sep 13, 2023

poi1649 Sep 13, 2023 •

edited

Loading

poi1649 Sep 12, 2023

hgo641 Sep 13, 2023

sonarcloud bot commented Sep 13, 2023

poi1649 left a comment


		public class SessionManager {

		private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();

[톰캣 구현하기 3 & 4단계] 홍고(홍여진) 미션 제출합니다 #483

[톰캣 구현하기 3 & 4단계] 홍고(홍여진) 미션 제출합니다 #483

Conversation

hgo641 commented Sep 11, 2023 • edited Loading

poi1649 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poi1649 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

poi1649 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

hgo641 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poi1649 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 13, 2023

poi1649 left a comment

Choose a reason for hiding this comment

hgo641 commented Sep 11, 2023 •

edited

Loading

poi1649 Sep 13, 2023 •

edited

Loading

poi1649 Sep 12, 2023 •

edited

Loading

hgo641 Sep 13, 2023 •

edited

Loading

poi1649 Sep 13, 2023 •

edited

Loading