Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[톰캣 구현하기 - 3, 4단계] 두둠(최영훈) 미션 제출합니다. #447

Merged
merged 9 commits into from
Sep 14, 2023

Conversation

younghoondoodoom
Copy link

안녕하세요! 새로운 리뷰어이실텐데 반갑습니다🙌 아마 코치님이실거 같아서 좀 떨리네요..

작업 내용

3단계

  1. 1, 2단계에서 HttpRequest, HttpResponse을 미리 만들어놨어서 새로 구현한건 거의 없습니다!
  2. 요구사항에 따라서 Controller를 활용하도록 구현했습니다! 원래는 Handler로 사용했었는데 요구사항을 맞춰야할 것 같아서 이름을 수정 했습니다.

4단계

  1. ExecutorService를 이용해서 쓰레드 풀을 생성하고, execute를 통해 쓰레드 풀에서 쓰레드를 가져와 실행하도록 구현했습니다.
  2. 기존에도 SessionManager를 ConcurrentHashmap을 사용하고있어서 변경 사항은 없습니다.

좋은 리뷰 부탁드립니다🙇‍♂️

@younghoondoodoom younghoondoodoom self-assigned this Sep 10, 2023
Copy link
Contributor

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 두둠
몇가지 피드백 남겼어요. 확인해보시고 수정해서 리뷰 재요청주세요.
그리고 테스트 코드가 너무 빈약합니다. 내가 만든 톰캣이 의도한대로 동작하는지 검증해보시기 바랍니다. 만약 테스트 작성이 어려웠다면 어떤 점 때문일지 고민해보세요.

tomcat/src/main/java/nextstep/Application.java Outdated Show resolved Hide resolved
tomcat/src/main/java/nextstep/Application.java Outdated Show resolved Hide resolved
tomcat/src/main/java/nextstep/Application.java Outdated Show resolved Hide resolved
import java.util.Map;
import java.util.stream.Collectors;

public class ResponseGenerator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HttpResponse와 분리할 필요가 있을까요??

@younghoondoodoom
Copy link
Author

안녕하세요 리오! 오랜만에 뵙네요 ㅎㅎ
구구가 리뷰해준대로 프레임워크를 만드는 것보다 WAS를 만드는 것을 목표로 했습니다! 그래서 프레임워크에서 사용할만할 클래스는 모두 삭제 했습니다!

고민지점

저는 nextstep 패키지를 어플리케이션, catalina는 thread 및 연결을 하는 Tomcat 구현체, coyote는 Http 요청을 처리하는 부분으로 봤습니다.
그래서 의존관계가 nextstep -> coyote -> catalina가 되어야한다고 생각을 했습니다.
그러나 현재 Controller interfacce, HandlerMapping 클래스의 경우 application 영역인 nextstep에 있지만 HttpProcessor에서 상수로 가지고 사용하고 있습니다. coyote 영역에서 어떻게 application 영역의 시작 지점을 가져와서 잘 사용할 수 있을까요? 원래는 DispatcherServlet을 사용했지만, 구구께서 톰캣은 DispatcherServlet을 몰라야한다고 하셔서 질문 남깁니다!

@Jaeyoung22 Jaeyoung22 self-requested a review September 14, 2023 06:17
Copy link

@Jaeyoung22 Jaeyoung22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 두둠!
리뷰 요청이 조금 늦어져서 촉박하게 리뷰를 해드렸네요ㅠㅠ
궁금한 부분과 질문에 대한 답변이 있어서 RC 남깁니다!
확인하시고 리뷰 요청 부탁드려요!


@Override
protected void doPost(final HttpRequest request, final HttpResponse response) {
if (request.containJsessionId()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

쿠키의 밸류에 "JSESSIONID"만 포함되어 있는 경우와 저장되지 않은 session의 id를 들고 있는 경우면 에러가 발생하고 회원 가입 페이지를 볼 수 없네요!
의도하신 부분일까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSESSIONID만 포함되어있는 경우에도 에러가 발생하는 경우인가요? 해당 로직은 JSESSIONID를 가지고있고 해당 JSESSIONID가 유효하다면 바로 index.html로 리다이렉트하는 로직입니다! 저장되지 않은 session의 id를 들고있는 경우면 에러가 발생하는건 의도한 부분입니다. 하지만 JSESSIONID만 포함되어있는 경우는 아닌것 같습니다! 제가 질문의 의도를 잘못 파악한걸까요??

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 JESSIONID=afasefa21r2r 가 아니라,
Cookie: JSESSIONID; yammy_cookie=strawberry
의 예를 이야기한 부분입니다!

return;
}

final Controller controller = handlerMapping.findController(request.getPath());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coyote는 web server, catalina는 servlet container라고 이해하고 있습니다!
해당하는 컨트롤러를 찾아서 요청을 service하는 것이 coyote에서 이루어져야할까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 coyote가 controller에 의존하고 있는 형태가 말도 안된다고 생각합니다..! 구구께서 dispatcherServlet가 필요한가에 대해서 질문 하셨는데 제가 없애라는 뜻으로 오해해서 이런 상황이 발생했네요!
다만 Http11Processor에서 HttpServlet 인터페이스를 가지게 있게하고, 구현체인 DispatcherServlet을 주입 받아서 사용했는데, 구구의 요구사항이 Tomcat에 아무것도 주입 받지 말라는 것이었습니다. 그래서 일단 DispatcherServlet을 상수로 두고 사용하게 했습니다.
Tomcat에서 DispatcherServlet 같은 servlet 인스턴스를 주입 받을때 web.xml을 읽고 reflection을 이용한다고 합니다. 그 부분을 구현할 수는 없어서 일단 상수로 가지고 사용하고 있습니다!

@sonarcloud
Copy link

sonarcloud bot commented Sep 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning 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

Copy link

@Jaeyoung22 Jaeyoung22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 빠르게 반영해주셨네요!
테스트가 없어서 아쉽네요, 추후에 꼭 추가해주세요! 고생하셨습니다~

@Jaeyoung22 Jaeyoung22 merged commit 1ae5770 into woowacourse:younghoondoodoom Sep 14, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants