[톰캣 구현하기 - 3, 4단계] 바론(이소민) 미션 제출합니다. #456

somsom13 · 2023-09-10T16:01:46Z

안녕하세요 홍고~!! 🍊 🧡 바론입니다 😸

3, 4단계 미션을 거쳐서,,, 어찌저찌 리팩터링 + 스레드 확장을 적용해보았습니다 ㅎ.ㅎ
그리고 Controller 가 분리된 만큼 각 Controller에 대한 테스트도 추가했습니다!

그런데 스레드 테스트는 어떻게 하는게 좋을지 감이 잡히지 않더라구요 🤔
혹시 홍고가 이에 대해서 고민해보신 내용이 있다면, 홍고의 생각을 알려주실 수 있나요?.?

리뷰 잘 부탁드립니다~~ 🙇

hgo641

안녕하세요, 바론~~~
코드 잘 봤습니다. FileReader클래스가 생기면서 코드가 더 깔끔해졌네요!
코드 잘 짜주셔서 리뷰할 게 별로 없네요ㅎㅎ 👍
궁금한 거 위주로 댓글 남겼습니다!
3, 4단계 구현하시느라 고생하셨습니다 ~~

hgo641 · 2023-09-12T15:58:18Z

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


 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;


250인 이유가 있나요? 궁금해요!

없습니다 ㅎㅎ LMS에 나온 생각해 볼 사항에 나온 값이라 그대로 설정했는데요...

스레드 수는 현재 서비스에 동시 사용자가 몇 명이나 접속할 수 있을지, 작업을 처리하는데 소모되는 시간 등을 고려해야 할 것 같아요.

해당 미션 코드에서는 애초에 사용자가 접속할 상황 자체가 없다고 생각해서 딱히 스레드를 측정하지 않고 LMS에 주어진 기본값으로 넣었습니다 ㅎㅎ

hgo641 · 2023-09-12T16:05:15Z

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

@@ -67,7 +70,7 @@ private void process(final Socket connection) {
            return;
        }
        var processor = new Http11Processor(connection);
-        new Thread(processor).start();
+        executorService.execute(processor);
    }

    public void stop() {


stop()에서 executorService를 shutdown하지 않은 이유가 있으신가요?

까먹었습니다... 스레드에 대한 지식이 부족한 탓(?) 입니다.

고치겠습니다 감사합니다!

hgo641 · 2023-09-12T16:06:27Z

tomcat/src/main/java/org/apache/coyote/FileReader.java

+import java.nio.file.Files;
+import org.apache.coyote.http11.exception.FileNotReadableException;
+
+public class FileReader {


파일을 읽는 로직을 클래스로 분리하셨군요! 깔끔합니다 👍

감사합니다 ^////^

hgo641 · 2023-09-12T16:14:42Z

tomcat/src/main/java/org/apache/coyote/FileReader.java

+
+    private final ClassLoader classLoader = getClass().getClassLoader();
+
+    public String readStaticFile(final String fileName) throws FileNotReadableException {


static메소드로 두지 않고, FileReader를 인스턴스화해서 사용하신 이유가 있으신가요?

사실 FileReader는 유틸성 클래스라 저도 static으로 관리하고 싶었지만.... ClassLoader 를 FileReader 내에서 관리하고 싶어서 FileReader를 인스턴스로 만드는 방법을 선택했습니다.

그리고 ClassLoader는 static 변수로는 관리할 수 없기 때문에 FileReader를 인스턴스로 만들고 내부에 인스턴스 변수로 classLoader를 가지게 했습니다!

ClassLoader를 외부에서 주입해줄 수도 있지만, 파일을 읽는데만 필요한 ClassLoader를 FileReader 밖에서 알아야 할 필요가 없을 것이라 생각했습니다.

@somsom13 ClassLoader를 static으로 할당하면 안되나요? ClassLoader.getSystemClassLoader();

이건 전혀 몰랐어요 ㅋㅋㅋㅋ 좋은 정보 감사합니다.. 전 당연히 파일을 읽을 클래스의 인스턴스가 생성되어야만 classLoader를 가져올 수 있다고 생각하고 있었어요 ㅎㅎ

감사합니다 따봉! 👍

hgo641 · 2023-09-12T16:19:35Z

tomcat/src/main/java/org/apache/coyote/UrlHandlerMapping.java

+    private static final Map<String, Controller> urlMatchedController;
+    private static final FileController fileController = new FileController();


얘네는 상수가 아닌건가요? 변수명이 소문자라 여쭤봅니다!

이런 간단한 컨벤션을 놓치다니 😢 😿 수정하겠습니다...!

왜 쳐다보세용..ㅠㅠ

정말 상수인지 궁금해서 여쭤봤습니다 😊

홍고와 이야기 했던 내용 추가)
FileController 는 유틸성 클래스이기 때문에 값이 변하지 않는 상수로 생각할 수 있을 것 같지만,
urlMatchedController는 불변이 보장되는 값인지 알 수 없다 -> 상수로 취급하는 것이 맞을까?

=> 지금 코드에서는 urlMatchedController 를 직접 개발자가 값을 설정해주기 때문에 불변이 맞다고 볼 수 있을 것 같아요! 하지만 만약 어노테이티드 controller나 다른 설정 방식을 사용한다면 컴파일 시에 직접 주입한 값들만 유지된다는 것을 확신하지는 못할 것 같아요.
그래서 이번 미션 코드에서는 상수로 취급해도 될 것 같은데, 혹시 어떻게 생각하시나요?

😄 좋아요. 코드에서 urlMatchedController의 값이 변경되지 않으니 ImmutableMap 강추합니다

hgo641 · 2023-09-12T16:35:54Z

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

@@ -16,6 +20,15 @@ public enum ContentType {

    public static final String ACCEPT_HEADER = "Accept";
    private static final ContentType DEFAULT = HTML;
+    private static final String FIELD_DELIMITER = ",";
+    private static final String WEIGHT_DELIMITER = ";";
+    private static final Map<String, ContentType> contentTypesByType;


Map객체 이름 짓는 규칙이 있으신가요? 궁금해요!
다른 코드에서는 keyWithValue 형식으로 Map객체 이름을 지으셨던 것 같아서요

없습니다 ㅎㅎ ㅋㅋㅋㅋㅋ 아마 이 부분은 ContentType을 강조하기 위해 ContentTypesByType 이라고 지었던 것 같은데요, 이건 다른 Map 들에도 통일해야겠네용

hgo641 · 2023-09-12T16:38:13Z

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

+
+    private static ContentType findContentTypeFromAcceptTypes(final List<String> types) {
+        return types.stream()
+            .map(contentTypesByType::get)


Map을 사용해서 더 빨리 찾을 수 있게 하셨군요 👍

hgo641 · 2023-09-12T16:43:35Z

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

+        if (id == null) {
+            return null;
+        }


findSession을 호출하는 메서드에서 null처리를 해주고 있긴 하지만, null대신 Optional을 반환하면 어떨까요? NPE무서워잉

NPE 무서웡... 좋습니다! 👍

HttpRequest의 getSession 도 Optional 로 처리할까 고민하다가.. 이 부분은 getSession(true) 를 하면 Optional 이 empty일 상황이 없고, 불필요한 isPresent 체크와 get이 호출될 것 같아서

말씀주신 SessionManager의 findSession만 Optional 적용했습니다!

hgo641 · 2023-09-12T16:44:49Z

tomcat/src/test/java/nextstep/org/apache/coyote/http11/Http11ProcessorTest.java


-        assertThat(socket.output()).contains(expectedHeader, expectedBody);
+        assertThat(socket.output()).isEqualTo(expected);


isEqualTo로 바뀐이유 궁금해요!!

Content-Type, Content-Length의 순서를 항상 유지하고 있기 때문인가요?

순서와는 별개로, 이전 단계에서 "모든 요청 상황에서" 반드시 헤더에 쿠키를 담게 해야 하는 요구사항을 구현할 때 "Cookie에 담기는 JSESSIONID의 랜덤값을 테스트 할 수 없었던 문제" 가 해결되었기 때문에 다시 수정했습니다 ㅎㅎ

이제는 로그인을 성공했을 때만 응답 헤더에 Set-Cookie 가 담기기 때문에 해당 테스트의 상황에서는 response에 JSESSIONID가 담길 상황이 사라져 isEqualTo 로 변경했습니다!

hgo641 · 2023-09-12T17:03:43Z

tomcat/src/main/java/org/apache/coyote/http11/controller/HelloController.java

+import org.apache.coyote.http11.message.request.HttpRequest;
+import org.apache.coyote.http11.message.response.HttpResponse;
+
+public class HelloController extends AbstractController {


저는 개인적으로 이번 미션에서 클래스들을 어떤 패키지에 넣을 것인지 고민을 많이 했던 것 같아요.
AbstractController의 구현체들 또한 마찬가지였는데요, HelloController포함 LoginController, RegisterController 구현체들은 apache의 기능이라기보단 jwp패키지의 model 을 CRUD 하는 컨트롤러라고 생각해서 jwp 패키지 하위에 두었어요.
인터페이스인 Controller는 apache에 둠으로써 의존성 역전(jwp -> apache)이 가능하다고 생각했어요.
고민을 많이 한 부분이라 바론은 요 친구들을 왜 apache/coyote 패키지에 두었는지 궁금하네요!

사실 처음에는 저도 새롭게 구현한 Controller 구현체들은 jwp 밑에 두었었는데요, 사실 이름만 Controller이지 내부 내용은 doPost, doGet으로 servlet의 작업에 더 가깝다고 생각해서 순수 비즈니스 로직이 아니라고 생각했습니다.

그런데 홍고의 리뷰를 보고 다시 생각해보니.. 저희가 지금까지 구현했던 Controller 도 비즈니스 로직 + ResponsenEntity 상태코드와 Location 헤더 설정하기 등등을 모두 수행했기 때문에 jwp 하에 위치하는 것이 맞는 것 같기도 하네요!

쳐다보시길래 추가 설명을 남겨보자면...

controller 구현체들 중 요청 URL에 따라 각각 다른 비즈니스 로직을 처리하도록 수행한 클래스들은 jwp 하위에 위치 시켰습니다.
다만, FileController (/index.html 등으로 요청이 오면 정적 파일을 반환하는 controller)는 비즈니스 로직이라고 보기 어려울 것 같고, 정적 파일에 대한 처리는 서블릿 차원에서 수행이 될 것이라 생각해 catalina 패키지 밑에 두었습니다!

그리고 FileReader 또한 FileController가 사용하는 부분이고, ViewResolver를 대체하는 역할이라 생각하여 catalina 하위에 두었습니다.
coyote 하위에는 정말 HTTP랑만 연관된 클래스들을 두었습니다 :)

ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ감사합니다...

sonarcloud · 2023-09-13T05:01:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 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

hgo641

안녕하세요, 바론!
댓글 잘 봤습니다ㅎㅎ 바론이랑 코드 얘기해서 재밌었어요
스레드는 다음에 또 얘기해보도록 해요 ㅎㅎㅎ
톰캣 미션 고생하셨습니다. MVC도 화이팅! 😄

somsom13 added 12 commits September 7, 2023 17:27

test: 캐시 학습 테스트

707dd1d

refactor: Controller 생성

895093c

docs: 3, 4 단계 기능 요구사항 작성

4b65958

feat: 동시성 확장

aca9710

test: 스레드 학습 테스트

c47cdcd

refactor: controller 패키지 분리

8e73ea5

refactor: Content Type 결정 시 Accept Type의 모든 타입 비교

d57277b

refactor: 500에러 핸들링

6cd857a

fix: LoginController 응답값 수정

b408ce0

test: Controller 테스트 추가

1ee2e74

test: 여러 Accept Type에 대한 Content Type 테스트 추가

ea44306

test: FileReader 테스트 추가

3b336b5

somsom13 changed the title ~~[톰캣 구현하기 - 3,4단계] 바론(이소민) 미션 제출합니다.~~ [톰캣 구현하기 - 3, 4단계] 바론(이소민) 미션 제출합니다. Sep 10, 2023

somsom13 requested a review from hgo641 September 10, 2023 16:04

somsom13 self-assigned this Sep 10, 2023

hgo641 requested changes Sep 12, 2023

View reviewed changes

somsom13 added 5 commits September 13, 2023 11:56

refactor: 상수명 컨벤션 적용, executorService shutdown

17ccd78

refactor: findSession Optional 포장

03b2e9d

refactor: 패키지 분리

62d4295

refactor: redirect 메서드 추가

c66012a

refactor: FileReader 유틸 클래스로 변경

b558ae2

hgo641 approved these changes Sep 13, 2023

View reviewed changes

hgo641 merged commit 2dce3f8 into woowacourse:somsom13 Sep 13, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 바론(이소민) 미션 제출합니다. #456

[톰캣 구현하기 - 3, 4단계] 바론(이소민) 미션 제출합니다. #456

somsom13 commented Sep 10, 2023 •

edited

Loading

hgo641 left a comment

hgo641 Sep 12, 2023

somsom13 Sep 13, 2023

hgo641 Sep 12, 2023 •

edited

Loading

somsom13 Sep 13, 2023

hgo641 Sep 12, 2023

somsom13 Sep 13, 2023

hgo641 Sep 12, 2023

somsom13 Sep 13, 2023

hgo641 Sep 13, 2023

somsom13 Sep 13, 2023

hgo641 Sep 12, 2023

somsom13 Sep 13, 2023

somsom13 Sep 13, 2023

hgo641 Sep 13, 2023

somsom13 Sep 13, 2023

hgo641 Sep 13, 2023

hgo641 Sep 12, 2023

somsom13 Sep 13, 2023

hgo641 Sep 12, 2023

hgo641 Sep 12, 2023

somsom13 Sep 13, 2023

somsom13 Sep 13, 2023

hgo641 Sep 12, 2023

somsom13 Sep 13, 2023 •

edited

Loading

hgo641 Sep 12, 2023

somsom13 Sep 13, 2023

somsom13 Sep 13, 2023

hgo641 Sep 13, 2023

sonarcloud bot commented Sep 13, 2023

hgo641 left a comment


		private final ClassLoader classLoader = getClass().getClassLoader();

		public String readStaticFile(final String fileName) throws FileNotReadableException {

		private static final Map<String, Controller> urlMatchedController;
		private static final FileController fileController = new FileController();


		assertThat(socket.output()).contains(expectedHeader, expectedBody);
		assertThat(socket.output()).isEqualTo(expected);

[톰캣 구현하기 - 3, 4단계] 바론(이소민) 미션 제출합니다. #456

[톰캣 구현하기 - 3, 4단계] 바론(이소민) 미션 제출합니다. #456

Conversation

somsom13 commented Sep 10, 2023 • edited Loading

hgo641 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hgo641 Sep 12, 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

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

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

somsom13 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

Choose a reason for hiding this comment

sonarcloud bot commented Sep 13, 2023

hgo641 left a comment

Choose a reason for hiding this comment

somsom13 commented Sep 10, 2023 •

edited

Loading

hgo641 Sep 12, 2023 •

edited

Loading

somsom13 Sep 13, 2023 •

edited

Loading