[톰캣 구현하기 - 3, 4단계] 땡칠(박성철) 미션 제출합니다. #471

0chil · 2023-09-11T04:17:25Z

안녕하세요 히이로! 지난 리뷰에 이어 잘 부탁드립니다

특이사항은 Controller(Handler) 인터페이스를 요구사항에 맞춰 다음과 같은 형태로 변경하게 되었습니다.

public interface Handler {
    void handle(HttpRequest request, HttpResponse response) throws Exception;
}

이 형태가 response가 어디서든 수정 가능하다는 장점은 있지만, 이런 형태가 꼭 필요한 것인지 아직은 잘 모르겠습니다 ㅠㅠ
오히려 이렇게 바꾸다보니 어디서 수정되었는지 예측하기 어려워서 디버깅이 더 어려워진다는 단점이 있었어요.

히이로는 핸들러가 왜 이런 형태를 가져야 한다고 생각하시는지 궁금합니다.

0chil · 2023-09-11T05:04:19Z

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

+ private static final int DEFAULT_ACCEPT_COUNT = 100; // 모든 쓰레드가 사용중일 때 들어오는 연결에 대한 대기열의 길이
+ private static final int MAX_THREADS = 250; // 동시에 연결 후 처리 가능한 요청의 최대 개수
+ // 최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?
+ // acceptCount를 100으로 한다
+ // 처리중인 연결이 250개 이므로 나머지는 OS 큐에서 대기하게 된다.
 private static final int SOCKET_TIMEOUT_SECONDS = 10;


정확한지는 잘 모르겠는데 제가 이해한대로 우선 적어보았습니다.
히이로는 최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?
라는 요구사항을 어떻게 반영해야한다고 생각하시나요?

네 저도 정확하게 땡칠과 같은 방식으로 이해하고 있습니다. 모든 쓰레드가 사용중일 떄 100개의 connection 요청들을 queueing하기 위해서 acceptCount로 ServerSocket을 생성할 때 OS에 저장되는 connection들에 대한 queue size를 100으로 설정해줍니다.

ThreadPool 생성 시 corePoolSize와 maximumPoolSize는 250으로 동일하게 설정했으니 시작부터 250개의 쓰레드가 생성되어 동작하면서 요청들을 처리하게 될 것 같습니다.

혹시 어떤 부분이 궁금하셨던건지 더 자세하게 따로 말씀해주시면 같이 고민해봐도 좋을 것 같습니다!

답변해주신 내용이 바로 궁금한 부분이었습니다.
Accept Count가 OS가 queueing 할 크기(백로그 크기)를 결정하는 부분이라고 이해했어요!

MoonJeWoong

안녕하세요 땡칠, 히이로입니다! 🙇

전반적으로 사소하다고 느껴지는 부분들은 피하고 구조적인 리뷰를 드려보려고 노력했습니다. 땡칠께서 달아주신 질문들에도 답을 달아뒀는데 저도 학습자의 입장이다보니 아무래도 답변 내용이 부족할 수도 있을 것 같아요! 그런 부분은 따로 저에게 말씀해주시면 같이 더 고민해볼 수 있도록 해보겠습니다. 😃

다음에 다시 pr요청 주시면 merge 하도록 하겠습니다. 추가적으로 개선해보고 싶으신 부분들 선택적으로 반영해서 요청해주세요~

MoonJeWoong · 2023-09-11T14:38:34Z

tomcat/src/test/java/org/apache/coyote/http11/session/SessionManagerTest.java

+ if (SessionManager.findSession(session.getId()) == null) {
+ fail("동시성 문제 발생");
+ }
+ }


HashMap의 put 메서드 실행 시 내부 수행 로직으로 인한 동시성 문제가 get 메서드 수행 시 발생할 수 있네요! 깊게 고민해주시고 작성한 테스트라는게 느껴집니다. 동시성에 대한 새로운 시야 배워갑니다~ 👍

MoonJeWoong · 2023-09-11T15:08:35Z

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

@@ -45,9 +36,7 @@ public static HttpRequest from(BufferedReader reader) {
 }


headers에서 contentLength만 따로 get 로직을 작성해주셨네요! headers 내부에 EMPTY 객체를 잘 만들어두셔서 해당 객체를 활용한 일관적인 처리방식으로 수행되지 못하는 것 같아 약간 아쉬운 부분 같습니다.

여기 headers에서 contentLength 값을 가져와서 로직 처리하는 부분은 저번과 같은데 제가 그때는 미처 생각하지 못했나봅니다...😭 반드시 수정해야 할 부분은 아니지만 여유가 되신다면 수정해보시는 것도 좋을 것 같아서 리뷰드립니다!

MoonJeWoong · 2023-09-11T15:09:32Z

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

@@ -45,9 +36,7 @@ public static HttpRequest from(BufferedReader reader) {
 }



Suggested change

HttpHeader header = headers.get("Content-Length");

int contentLength = 0;

if (!header.isEmpty()) {

contentLength = Integer.parseInt(header.getValue());

}

char[] bodyBuffer = new char[contentLength];

while (contentLength > 0) {

int readCount = reader.read(bodyBuffer, 0, contentLength);

contentLength -= readCount;

}

요런 느낌은 어떠신지 조심히 제안드려봅니당 😁

위 커멘트와 이어지는 내용이네요.

EMPTY 객체를 만들어두고 잘 활용하지 못하는 부분이 아쉬우셨군요. 일관적이지 못하다는 관점 충분히 이해가 갑니다!
하지만 getContentLength()를 유지하고 싶어요.

이 메서드를 별도로 만든 의도는 null에 대한 대응 방법을 캡슐화하기 위해서였어요.
헤더값은 문자열이므로 Integer.parseInt(header.getValue())를 사용해주어야 하는데,
header.getValue()는 빈 문자열일수도 있기 때문에, 위 로직처럼 빈 헤더인지 체크 후에 사용해야하는 복잡함이 있습니다.

그래서 '자주 사용하는 일반적인 헤더니까 걱정없이 정수 형태의 컨텐츠 길이를 얻자' 라는 의도로 일종의 헬퍼 메서드를 만들었습니다.

사실 로직의 위치만 다를 뿐, 히이로가 제안해주신 내용과 같은 코드인 것 같아요.
그래서 당연하게도 히이로가 제안하신 코드도 당장 동작하는 상황입니다.

하지만 지금 구현한 헬퍼 메서드를 통해 null에 대한 처리도 함께 재활용할 수 있다는 장점이 있으므로 getContentLength()는 유지하고자 합니다.

null 처리 로직의 캡슐화가 주 목적이셨군요! 이해했습니다 ㅎㅎ 👍

MoonJeWoong · 2023-09-11T15:32:00Z

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

@@ -52,13 +52,14 @@ public void process(final Socket connection) {
 final var outputStream = connection.getOutputStream()) {

 HttpRequest httpRequest = HttpRequest.from(bufferedReader);
- HttpResponse httpResponse = handle(httpRequest);
+ HttpResponse httpResponse = new HttpResponse();
+ handle(httpRequest, httpResponse);


이 handle 로직을 hanlerMapper 객체로 분리해내주셨으면 더 책임분리가 깔끔하게 되지 않았을까 싶어요! 땡칠도 잘 아실거라 생각하지만 이 handlerMapper 객체가 서블릿 컨테이너와 책임이 동일한 객체인지라 coyote 패키지의 Http11Processor와는 분리가 되는 것이 맞다는 생각입니다.

이 생각의 흐름으로 보면 땡칠께서 남겨주신 의문점이 해결되지 않을까 싶습니다. "response가 어디서든 수정 가능하다는 장점"이 왜 필요한 것일까요? 저는 Http11Processor에서 처음 생성된 request, response가 handlerMapper(=servletContainer)에서 mapping된 handler(servlet)을 통해 비즈니스 로직을 수행하고 해당 결과가 response에 담겨 client에게 다시 전달되어야 하기 때문이라고 생각합니다.

또한 response에 담기는 헤더 값들이 어떤 한 지점에서 바로 완성되는 것이 아니라 계속 여러 단계를 거치면서 추가되는 것이기 때문에 response 객체가 전달되어야 한다고도 생각할 수 있을 것 같습니다.

이 생각들은 저도 개인적으로 학습하고 갖고있는 생각이라 충분히 틀릴 수 있으니 땡칠께서 보시고 이상하다 싶은 부분은 편하게 말씀해주시면 감사하겠습니다!

응답이 동적으로 생성되기까지 수많은 과정 속에서 언제든 응답코드, 헤더, 바디를 수정하기 위함이라는 말씀이시네요!
듣고 단번에 이해가 되었습니다.

이 말을 듣고보니까
함수 반환으로 Response를 사용하는 제 기존 디자인에서는
handler 호출 스택의 끝에서 response를 만들도록 제한되는 단점이 있네요.

예를 들어 서비스 객체가 있다면, 호출 스택의 끝인 서비스 객체에서 response를 만들어야할 것 같아요.
이런 디자인은 상황에 따라 부적절한 곳에서 응답을 생성하게 될 수도 있겠네요.

결론은 '어디서든 응답 수정을 쉽게 하려는 의도가 담긴 디자인이다'. 네요
좋은 커멘트 감사합니다~!!

MoonJeWoong · 2023-09-11T15:52:38Z

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

+ public void setBody(String body) {
+ this.body = body;
+ putHeader(new HttpHeader("Content-Length", String.valueOf(body.getBytes().length)));
+ }


반환하는 body값이 없을 때도 setBody를 해주시고 Content-Length: 0 이렇게 내려가는 것 같습니다. 원래 response 메세지의 body가 없을 떄도 Content-Length를 내려주는 것이 규약인가요? 요 부분은 저도 아리송해서 질문드립니다!

잠깐 찾아보니 body가 반환되는 일반적인 경우에서 body가 실제로 0임을 명시적으로 나타내기 위해 0을 나타내는 경우도 있는 것 같습니다. 그런데 302 Found 같은 경우는 Location 헤더만 명시해주면 될 것 같은데 body를 같이 내려주시는 이유가 따로 있으실까요? 혹시 명시된 document 부분이 있으면 알려주시면 감사하겠습니다!

바디가 없을 때에 대한 Content-Length 표준은 없습니다!
Content-Length를 별도로 설정하지 않고
body 길이에 따라 정하려는 의도였는데, body = "" 인 경우를 생각하지 못했네요.

이 부분도 반영해보겠습니다!

MoonJeWoong · 2023-09-11T16:10:08Z

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

 String headerLines = httpResponse.getHeaders().stream()
 .map(HttpHeader::toLine)
 .collect(Collectors.joining(CRLF));
 final var response = String.join(CRLF,
- HTTP_VERSION + httpResponse.getStatus().toLine(),
+ HttpResponse.HTTP_VERSION + WHITE_SPACE + httpResponse.getStatus().toLine(),
 headerLines,
 "",
 httpResponse.getBody()


client에게 내려줄 최종 response 형태를 만드는 책임도 HttpResponse 객체에 부여해보시는건 어떨까요?

오 좋은 생각입니다 역시 👍
반영해보겠습니다!

MoonJeWoong · 2023-09-11T16:30:05Z

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

+ private static final int DEFAULT_ACCEPT_COUNT = 100; // 모든 쓰레드가 사용중일 때 들어오는 연결에 대한 대기열의 길이
+ private static final int MAX_THREADS = 250; // 동시에 연결 후 처리 가능한 요청의 최대 개수
+ // 최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?
+ // acceptCount를 100으로 한다
+ // 처리중인 연결이 250개 이므로 나머지는 OS 큐에서 대기하게 된다.
 private static final int SOCKET_TIMEOUT_SECONDS = 10;


네 저도 정확하게 땡칠과 같은 방식으로 이해하고 있습니다. 모든 쓰레드가 사용중일 떄 100개의 connection 요청들을 queueing하기 위해서 acceptCount로 ServerSocket을 생성할 때 OS에 저장되는 connection들에 대한 queue size를 100으로 설정해줍니다.

ThreadPool 생성 시 corePoolSize와 maximumPoolSize는 250으로 동일하게 설정했으니 시작부터 250개의 쓰레드가 생성되어 동작하면서 요청들을 처리하게 될 것 같습니다.

혹시 어떤 부분이 궁금하셨던건지 더 자세하게 따로 말씀해주시면 같이 고민해봐도 좋을 것 같습니다!

0chil

안녕하세요 히이로 ✋🏻 좋은 아침입니다
커멘트 남겨주신 내용들 고민해보고 반영했습니다.
큰 변경이 하나 있고, 작은 변경들 몇 개가 있어서 둘을 나눠서 적어보았습니다:

RequestMapping

다음은 가장 큰 변경사항(RequestMapping) 커밋입니다!
5092d78

Controller 인터페이스에 supports 메서드 추가했습니다!
RequestMapping 클래스 만들었습니다. (리뷰 반영)
TargetPath 라는 클래스에 경로 관련 작업들을 위임했습니다.

그 외 변경사항

Response가 응답 생성 (리뷰 반영)
바디 길이 0일때, Content-Length 헤더 제외 (리뷰 반영)
Handler -> Controller로 재명명 (요구사항)

MoonJeWoong · 2023-09-13T13:51:37Z

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

+ private final RequestMapping requestMapping = new RequestMapping(List.of(
+ new HelloController(),
+ new LoginController(),
+ new FileController(),
+ new RegisterController()
+ ));


요 부분은 RequestMapping 객체가 초기화될 때 한 번만 수행되면 되는 로직이고, 동적으로 처리해야 되는 부분도 없는 것 같습니다. 이런 로직은 객체 내 static block으로 처리해보시는 건 어떤지 제안드려봅니당 ㅎㅎ

뒤쪽에서 RequestMapping 로직을 읽어봤는데요, List로 전달된 controller들을 순서대로 순회하면서 supports 메서드를 수행하는 방식으로 controller를 매핑시켜주는 방식이었던 것 같아요. 여기에서 List 자료구조를 선택하신 이유가 혹시 있으신지 개인적으로 궁금합니다!

좋은 생각이네요.! 동적으로 변할 필요가 있을 때 변경해도 괜찮을 것 같아요.

특별한 이유는 없었습니다. 순서 보장도 필요 없으니 Set이 더 적절하겠네요. 중복 방지도 가능하구요!

꼼꼼한 커멘트 감사합니다! 말씀주신 내용 반영하겠습니다

MoonJeWoong · 2023-09-13T13:52:03Z

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

+ HttpResponse httpResponse = new HttpResponse();
+ requestMapping.handle(httpRequest, httpResponse);

- outputStream.write(response.getBytes());
+ outputStream.write(httpResponse.toLine().getBytes());


마지막까지 깔끔하게 리팩토링 해주셨네요~ 👍

MoonJeWoong · 2023-09-13T13:53:27Z

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

- private final Map<String, HttpHeader> headers = new HashMap<>();
+ private HttpStatus status;
+ private String body;
+ private final Headers headers2 = new Headers();


리팩토링의 흔적이 남아있는 변수명이네요 ㅎㅎ 여기는 수정 부탁드립니다!

ㅎㅎ 꼼꼼한 리뷰 감사드립니다 반영했습니다!

MoonJeWoong · 2023-09-13T13:58:50Z

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

+import java.net.URL;
+import java.util.Objects;
+
+public class TargetPath {


책임분리 Cool~ 👍

sonarcloud · 2023-09-14T04:23:21Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
5.3% 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

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

MoonJeWoong

마지막 변경사항까지 반영 완료하셔서 merge 하도록 할게요!
미션 진행하시느라 고생 많으셨습니다 ㅎㅎ 다음 미션도 화이팅입니다~ 😄

0chil added 6 commits September 11, 2023 05:43

refactor(Http11Processor): HTTP Version 관리를 Response로 위임

a4b2bd7

refactor(HttpRequest): Request Line을 객체로 분리

1f6ab90

feat: GET, POST를 담당하는 핸들러

fa959af

feat(Connector): Thread Pool 적용

04ee075

feat(SessionManager): 발생할 수 있는 동시성 문제를 해결한다

7dff12a

refactor: Handler 인터페이스를 request, response 로 수정한다

9d3d82c

0chil commented Sep 11, 2023

View reviewed changes

MoonJeWoong self-requested a review September 11, 2023 14:15

MoonJeWoong requested changes Sep 11, 2023

View reviewed changes

0chil added 6 commits September 12, 2023 20:56

refactor: 쿠키, 세션 조회에 Optional 사용

a550e22

refactor(Headers): 메서드 변수명 수정

16cc50b

refactor(HttpResponse): 바디 길이 0일때, Content-Length 헤더 제외

ef2ac0d

refactor: HttpResponse가 응답을 생성한다

93015f9

refactor: Handler를 Controller로 재명명

5fdd9e8

refactor: 컨트롤러 매핑 RequestMapping으로 분리, TargetPath 객체 분리

5092d78

0chil commented Sep 13, 2023

View reviewed changes

fix: 실패하는 테스트 보완

1c46ac7

woowacourse deleted a comment from sonarcloud bot Sep 13, 2023

MoonJeWoong reviewed Sep 13, 2023

View reviewed changes

0chil added 3 commits September 14, 2023 13:17

refactor: RequestMapping 상수화

5372bd0

refactor: 핸들러 목록의 자료형을 Set으로 변경

4e85edc

typo(HttpResponse): 변수명 headers2 -> headers

d8f979d

MoonJeWoong approved these changes Sep 14, 2023

View reviewed changes

MoonJeWoong merged commit 00422ae into woowacourse:0chil Sep 14, 2023
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 땡칠(박성철) 미션 제출합니다. #471

[톰캣 구현하기 - 3, 4단계] 땡칠(박성철) 미션 제출합니다. #471

0chil commented Sep 11, 2023 •

edited

Loading

0chil Sep 11, 2023

MoonJeWoong Sep 11, 2023

0chil Sep 12, 2023

MoonJeWoong left a comment

MoonJeWoong Sep 11, 2023

MoonJeWoong Sep 11, 2023

MoonJeWoong Sep 11, 2023

0chil Sep 12, 2023

MoonJeWoong Sep 13, 2023

MoonJeWoong Sep 11, 2023

0chil Sep 12, 2023

MoonJeWoong Sep 11, 2023

0chil Sep 12, 2023

MoonJeWoong Sep 11, 2023

0chil Sep 12, 2023

MoonJeWoong Sep 11, 2023

0chil left a comment

MoonJeWoong Sep 13, 2023

MoonJeWoong Sep 13, 2023

0chil Sep 14, 2023 •

edited

Loading

MoonJeWoong Sep 13, 2023

MoonJeWoong Sep 13, 2023

0chil Sep 14, 2023

MoonJeWoong Sep 13, 2023

sonarcloud bot commented Sep 14, 2023

MoonJeWoong left a comment

		@@ -45,9 +36,7 @@ public static HttpRequest from(BufferedReader reader) {
		}

+HttpHeader header = headers.get("Content-Length");
+int contentLength = 0;
+if (!header.isEmpty()) {
+ contentLength = Integer.parseInt(header.getValue());
+}
+char[] bodyBuffer = new char[contentLength];
+while (contentLength > 0) {
+ int readCount = reader.read(bodyBuffer, 0, contentLength);
+ contentLength -= readCount;
+}

[톰캣 구현하기 - 3, 4단계] 땡칠(박성철) 미션 제출합니다. #471

[톰캣 구현하기 - 3, 4단계] 땡칠(박성철) 미션 제출합니다. #471

Conversation

0chil commented Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonJeWoong 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0chil left a comment

Choose a reason for hiding this comment

RequestMapping

그 외 변경사항

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0chil Sep 14, 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 14, 2023

MoonJeWoong left a comment

Choose a reason for hiding this comment

0chil commented Sep 11, 2023 •

edited

Loading

0chil Sep 14, 2023 •

edited

Loading