[1단계 - Tomcat 구현하기] 낙낙(이낙헌) 미션 제출합니다. #561

nak-honest · 2024-09-06T08:39:27Z

안녕하세요 러쉬쓰~~~~

DM으로는 2단계까지 구현한다 했지만, 1단계까지밖에 구현하지 못했습니다..!
다만 구현하다보니 3단계의 일부분을 구현해서, 이후의 PR 양이 엄청 많아지지는 않을 것 같습니다.

보시면서 의문이 드시는 부분이나, 개선할만한 부분이 있다면 편하게 리뷰 주세요!!

xogns1514

안녕하세요 낙낙쓰~~~😀 러쉬입니다🏈 1단계임에도 불구하고, 클래스별 책임분리가 매우 잘 되어있군요ㅎㅎ 보고 배울점이 많은 코드였습니다. 아마 나머지 단계는 금방 구현하실 것 같아요.
간단한 질문사항만 남겨두습니다.
다음 코드까지 진행한 뒤 이야기할 것이 많아질 것 같아서 바로 Merge 드립니다.

xogns1514 · 2024-09-07T15:03:25Z

study/src/main/java/cache/com/example/cachecontrol/CacheWebConfig.java

    @Override
    public void addInterceptors(final InterceptorRegistry registry) {
+        WebContentInterceptor noCacheInterceptor = new WebContentInterceptor();


미리 구현된 인터셉터를 활용하셨군요👍🏿

xogns1514 · 2024-09-07T15:04:53Z

study/src/main/java/cache/com/example/version/VersionHandlebarsHelper.java

-@HandlebarsHelper
-public class VersionHandlebarsHelper {
+@Component
+public class VersionHandlebarsHelper implements Helper<Object> {


기존 Handlebars가 작동하지 않아, 어떻게 해결해야하는지 궁금했는데 배워갑니다ㅎㅎ

해당 부분은 #520 에서 참고했습니다!!
출처는 페드로에욥 ㅎㅎ

xogns1514 · 2024-09-07T15:10:57Z

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

@@ -67,7 +66,9 @@ private void process(final Socket connection) {
            return;
        }
        var processor = new Http11Processor(connection);
-        new Thread(processor).start();
+        Thread thread = new Thread(processor);
+        thread.setDaemon(true);


[질문]
데몬 스레드를 설정한 이유가 무엇인가요?

우선 확인해보니 스레드는 자신을 생성한 스레드의 데몬 상태를 상속한다고 합니다!
따라서 setDaemon(true) 를 안 해줘도, 자동으로 데몬 스레드가 되겠네요 😅
불필요한 코드였습니다!

제가 데몬 스레드로 설정하고자 했던 의도는 다음과 같습니다!

JVM은 일반 스레드가 하나라도 존재하면 종료되지 않습니다.
즉 메인 스레드가 종료되어도 다른 일반 스레드가 살아 있다면 JVM은 종료하지 않고 계속 실행됩니다.

따라서 Http11Processor 스레드가 일반 스레드였다면 메인 스레드가 죽어도 JVM이 죽지 않아 모든 로직을 다 실행할 것이고, 저는 이 상황이 정상 상황이 아니라고 생각하였습니다. 메인 스레드가 죽는 경우는 예외 상황이라고 생각했거든요!

저희 코드에서 테스트 해보니 메인 스레드를 inturrupt 로 갑자기 죽였을 때, ServerSocket을 close 하지 못해서 Http11Processor스레드가 계속 살아있더라구요.

따라서 데몬 스레드로 설정하려고 하였습니다!

xogns1514 · 2024-09-07T15:27:12Z

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

+    }
+
+    public static String probeContentType(Path path) throws IOException {
+        String mimeType = Files.probeContentType(path);


ContentType을 찾아주는 메서드를 활용하셨군요👍🏿

xogns1514 · 2024-09-07T15:53:35Z

tomcat/src/main/java/org/apache/coyote/http11/message/request/HttpRequestInfo.java

+public class HttpRequestInfo {
+
+    private final HttpMethod method;
+    private final String urlPath;


HttpUrl 이라는 클래스를 두셨던데, 여기에서 활용하지 않는 이유가 있는지 궁금합니다!

HttpRequestInfo는 HttpHandlerMapper 에서 key 로 활용하기 위해 만든 클래스입니다!
어떤 핸들러를 호출할지 결정하는 것은 요청의 url path + method 이기 때문에 위와 같이 하나의 객체로 묶었는데,
HttpUrl 에는 쿼리 파라미터도 포함되어 있기 때문에 여기서 활용하지 않았습니다..!

다만 러쉬가 말씀해 주신대로 Info 라는 이름은 명확하게 의도를 드러내지 못하는 것 같네요!!
리팩토링 하면서 더 좋은 이름을 지어 보겠습니닷!!

xogns1514 · 2024-09-07T15:54:07Z

tomcat/src/main/java/org/apache/coyote/http11/message/request/HttpRequest.java

+    private final HttpMethod method;
+    private final HttpUrl url;


이부분도 HttpRequestInfo라는 클래스를 활용하지 않은 이유가 있는지 궁금합니다ㅎㅎ

위에서 말씀 드린대로 HttpRequestInfo 는 HttpHandlerMapper의 key로 활용하기 위해서 였습니다..!

xogns1514 · 2024-09-07T16:04:51Z

tomcat/src/main/java/org/apache/coyote/http11/message/request/HttpRequestInfo.java

+
+import java.util.Objects;
+
+public class HttpRequestInfo {


Info 라는 네이밍은 흔히 의미가 불분명한 불용어라고 알려져있는데요.
Avoid words like Manager, Processor, Data, or Info in the name of a class. - (출처. 클린코드)

만약 해당 클래스를 HttpRequest에서도 활용하게 된다면, rfc 공식문서에서 사용하는 용어를 사용해보는건 어떨까 제안드려요~

xogns1514 · 2024-09-07T16:09:41Z

tomcat/src/main/java/org/apache/coyote/http11/message/request/HttpRequestReader.java

+        List<String> headerLines = new ArrayList<>();
+
+        String headerLine = bufferedReader.readLine();
+        while (!headerLine.isEmpty()) {


Suggested change

while (!headerLine.isEmpty()) {

while ((headerLine = bufferedReader.readLine()) != null && !headerLine.isEmpty()) {

null 검사도 추가하면, 예상치 못한 상황에 대해서도 안전하게 파싱할 수 있을 것 같습니다ㅎㅎ

훨씬 안전하겠네요!!
해당 부분 리팩토링 하겠습니다!

꼼꼼한 리뷰 감사드립니다 ㅎㅎ

xogns1514 · 2024-09-07T16:16:50Z

tomcat/src/main/java/org/apache/coyote/http11/message/request/HttpRequestReader.java

+import java.util.List;
+import org.apache.coyote.http11.message.HttpHeaders;
+
+// TODO: BufferedReader와 BufferedInputStream 같이 사용했을 때 문제가 발생하지 않는지 확인하기


BufferedInputStream를 같이 사용하신 이유가 있을까요?? BufferedReader만을 활용해서, body까지 파싱할 수 있을 것 같아서요!

BufferedReader는 문자 단위로 읽기 때문에, read 메소드에 전달되는 len이 바이트의 길이가 아닌 문자의 길이를 의미합니다.

그런데 UTF-8 인코딩에서 한글이나 특정 특수문자는 문자 하나당 2~4바이트의 크기를 가지는데요!
이 때문에 문자의 길이와 바이트의 길이가 같지 않는 문제가 발생하게 됩니다.
다른 인코딩 방식을 사용할 때에도 문제가 되구요!

따라서 원하는 크기의 바이트만 읽을 수 있는 BufferedInputStream 을 사용하게 되었습니다.

물론 BufferedReader를 사용해서 문자를 하나씩 읽고 해당 문자가 가지는 바이트 크기를 계산하면, Content-Length 만큼 읽는 것이 가능하긴 합니다. 하지만 계산 방식도 복잡하고 매번 문자를 하나씩만 읽어오는 것이 성능 상으로도 좋지 않을 것 같아 BufferedInputStream을 사용하기로 결정하였습니다.

또한 문자가 아닌 바이너리 데이터가 바디에 담기는 경우도 고려했구요!

하지만 확인해 보니 BufferedReader와 BufferedInputStream을 같이 사용하면 한 쪽 버퍼에 미리 담기게 되어 문제가 발생하더라구요!

따라서 전부 BufferedInputStream 을 사용하는 쪽으로 바꾸게 될 것 같습니다..!

오 해당 내용은 처음 알게된 내용이네요~ UTF-8 인코딩에서는 문제가 될 수 있겠군요ㅎㅎ

geoje and others added 16 commits September 5, 2024 11:11

fix: remove implementation logback-classic on gradle (woowacourse#501)

fed02f6

fix: add threads min-spare configuration on properties (woowacourse#502)

7e91356

feat: Http 요청/응답 객체 구현

a8707e9

feat: Http 요청을 read하고 응답을 wirte 하는 기능 구현

5063922

feat: Http 요청으로부터 응답을 내는 기능 구현

dcb726e

feat: 로그인을 하면 유저 정보를 찾는 기능 구현

cb8d8e4

feat: 클라이언트로부터 HTTP 요청을 받아 응답을 하는 기능 구현

9275770

refactor: 각 클라이언트의 요청을 처리하는 스레드를 데몬 스레드로 변경

1877ccd

feat: 학습 테스트 1 구현

94a6f54

feat: 학습 테스트 2 구현

c9dd251

merge 충돌 해결

b9a6d5a

refactor: query parameter 들을 일급 컬렉션으로 래핑

013d6a6

refactor: static class 의 생성자 접근제어자를 private 으로 변경

4cfcf08

style: 코드 스타일 통일성이 맞도록 수정

064a916

refactor: 변수명 변경

18211e1

refactor: 지역 변수로 받지 않고 바로 return하도록 변경

160869d

nak-honest requested a review from xogns1514 September 6, 2024 08:39

nak-honest self-assigned this Sep 6, 2024

nak-honest added 2 commits September 6, 2024 20:14

fix: 학습 테스트 2 4번에서 ETag 가 포함되지 않는 문제 해결

cdec028

fix: 브라우저에서 versoning이 적용되도록 수정

e37693b

xogns1514 approved these changes Sep 7, 2024

View reviewed changes

xogns1514 merged commit 5364a44 into woowacourse:nak-honest Sep 7, 2024

	while (!headerLine.isEmpty()) {
	while ((headerLine = bufferedReader.readLine()) != null && !headerLine.isEmpty()) {

[1단계 - Tomcat 구현하기] 낙낙(이낙헌) 미션 제출합니다. #561

[1단계 - Tomcat 구현하기] 낙낙(이낙헌) 미션 제출합니다. #561

Uh oh!

Conversation

nak-honest commented Sep 6, 2024

Uh oh!

xogns1514 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak-honest Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak-honest Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak-honest Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nak-honest Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nak-honest Sep 9, 2024 •

edited

Loading

nak-honest Sep 8, 2024 •

edited

Loading

nak-honest Sep 8, 2024 •

edited

Loading

nak-honest Sep 8, 2024 •

edited

Loading