-
Notifications
You must be signed in to change notification settings - Fork 309
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
[톰캣 구현하기 - 1, 2단계] 말랑(신동훈) 미션 제출합니다. #310
Conversation
- 톰캣에서 Handler를 모르도록 하기 위함
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그냥 tomcat을 만들어버렸네요 말랑
코드 리뷰를 해야 했는데 보고 배우거나 질문을 드린 부분도 많아서 오히려 좋았습니다
바로 merge해도 되지만 3단계까지 구현한 수준이라서 코멘트들 천천히 보시면서 적용해도 충분할 거 같습니다. 수고하셨습니다 말랑😊😊
tomcat/src/main/java/nextstep/jwp/handler/StaticResourceRequestHandler.java
Outdated
Show resolved
Hide resolved
|
||
public class InvalidateSessionException extends RuntimeException { | ||
|
||
public InvalidateSessionException() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만료된 세션에 대한 예외라면 ExpiredSession
은 어떤가요? 바꾼다면 Session 내부의 invalidate도 expired 같은 게 좋겠네요😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
힌트에 존재하는 메서드 이름이 Invalidat()여서 위와 같이 작성했습니다!
변경할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalidate() 메서드는 해당 세션을 못 쓰도록 만드는 기능이라 메서드명이 어울린다고 생각합니다. 동일한 형식을 가지고 가려면 Session 내부의 invalidate를 isInvalid와 같은 형용사로 바꾸는 건 어떨까요? 동사라서 좀 어색하게 느껴져요.
InvalidateSessionException
이라는 예외는 제가 느끼기에 무효화된 세션에 대해 작업을 하려고 할 때 호출된다기보다 어떤 세션을 무효화시킬 때 발생할 것같은 예외라고 생각되었어요. 말랑이 편한대로 해도 되겠네요!!
There was a problem hiding this 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/org/apache/catalina/servlet/util/RequestParamUtil.java
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/catalina/servlet/response/HttpStatus.java
Show resolved
Hide resolved
tomcat/src/test/java/nextstep/jwp/handler/StaticResourceRequestHandlerTest.java
Show resolved
Hide resolved
// given | ||
List<String> headers = List.of( | ||
"Host: localhost:8080", | ||
"User-Agent: Insomnia/2023.5.7", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
왜 불면증이죠?ㅋㅋㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 요거 답 안하고 왜 넘어갔죠 ㅋ꙼̈ㅋ̆̎ㅋ̊̈ㅋ̌̈ㅋ̄̈ㅋ̐̈ㅋ̑̈ㅋ̆̈
인섬니아라고 postman처럼 http 요청 쏴주는 툴 있는데 요즘 postman보다 이게 이쁘고 좋아서 사용하고 있습니다.
요청 보난거 복붙하다보니 이렇개 됐네용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의견 들려주셔서 감사합니다. merge하려고 했는데 테스트가 하나 깨지는 부분 있어서 수정 부탁드려요. 수고하셨습니다!!!
private RequestHeaders requestHeaders; | ||
private Body body; | ||
|
||
public HttpRequestBuilder startLine(StartLine startLine) { | ||
this.startLine = startLine; | ||
public HttpRequestBuilder startLine(RequestLine requestLine) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드명도 requestLine으로 바꿔야할 거 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니당
if (uri.path().endsWith(".js")) { | ||
return "test/javascript"; | ||
} | ||
if (uri.path().endsWith(".js")) { | ||
return "test/javascript"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동일한 if문이 중복되어 있고 "text/javascript;"
를 반환해줘야 테스트가 안 깨질 것 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇.. 피곤했나봐요...ㅠㅠ
Kudos, SonarCloud Quality Gate passed! 0 Bugs 0.0% Coverage 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨어요 말랑!!! 3, 4단계 진행해주시죠
안녕하세요 썬☀️샷🍻
1, 2 단계 리뷰 요청드립니다.
전체적인 구조는 다음과 같습니다.
org.apache
패키지 내부Tomcat
:Servlet
컨테이너Sevlet
: 사용자의 요청을 동적으로 처리하기 위한 인터페이스nextstep.jwp
패키지 내부DispatcherServlet
:Tomcat
에 등록되어프론트 컨트롤러
역할을 수행해줄Servlet
구현체입니다.RequestHandler
: 실제 사용자의 요청 처리를 담당하는 인터페이스만들고 보니까
RequestHandler
가 굳이 필요 없었고,Sevlet
을 구현하는 것만으로도 충분했었다는 생각이 드네요.. 🥲이러한 구조가 만들어진 이유는 다음과 같습니다.
톰캣을 구현하기 전에 먼저 톰캣에 대해 살짝 알아보았을 때,
톰캣
은서블릿 컨테이너
라는 것을 알게 되었고 이러한 개념을 적용하기 위해Servlet
인터페이스를 만들어Tomcat
에서 관리하도록 했습니다.(관리라 하기도 애매한게 라이프사이클을 딱히 관리하는게 아니라 단지 사용하는 것 뿐이라..)
그리고 이어서 스프링의
DispatcherServlet
을 참고하여 이와 비슷하게 만들어 보았습니다.RequestHandler
는Servlet
을 적용하기 전에 먼저 작성한 클래스라 나중에 가니까 딱히 쓸모가 없어졌네요..!구현을 하면서 제일 신경썼던 것은
org.apache
패키지가nextstep.jwp
패키지를 모르도록 만드는 것이었습니다!전체적인 흐름은 다음과 같습니다.
Tomcat
에DispatcherServlet
을 세팅한 뒤 실행합니다.Tomcat
은 실제 사용자와 연결하는 작업을Connector
로 위임하여 처리됩니다.Connector
는 사용자 요청이 들어오면Http11Processor
를 통해 처리합니다.Http11Processor
는 요청 정보를 통해HttpRequest
객체를 생성한 뒤DispatcherServlet
으로 사용자 요청 처리 동작를 위입합니다.DispatcherServlet
에서는HandlerMapping
을 통해 요청을 처리할 수 있는RequestHandler
를 찾고 실행합니다.RequestHandler
에서는 요청을 처리하며HttpResponse
객체를 세팅합니다.RequestHandler
의 처리가 끝나면 최종적으로Http11Processor
에서 세팅된HttpResponse
를 통해 응답 메세지를 만들어 사용자에게 응답합니다.