-
Notifications
You must be signed in to change notification settings - Fork 388
[4단계 - Tomcat 구현하기] 커비(이현지) 미션 제출합니다. #701
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
Conversation
unifolio0
left a comment
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: | ||
| accept-count: 1 | ||
| max-connections: 1 | ||
| max-connections: 2 |
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.
질문입니다!
max-connections가 무엇을 의미하나요?
왜 해당 값만 변경해도 테스트가 통과하나요?
accept-count는 테스트를 통과하는데 영향이 없나요?
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.
max-connections는 동시에 처리할 수 있는 최대 연결수를 의미해요!
테스트에서 200을 응답하면 올라가는 count가 2어야지 통과되게 되어있기때문에
정상 처리되어야하는 연결수를 2로 바꾸어주었습니다.
accept-count는 쓰레드로 할당은 못되었지만 대기할 수 있는 대기열의 크기를 의미하므로 영향을 주지 않습니다~
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.
#687 (comment)
해당 테스트 코드를 보면 200을 반환하는 수가 2개면 통과하도록 되어 있어요. 그러면 대기하고 있다가 처리하면 통과하는 게 맞지 않을까요? accept-count도 테스트에 영향을 미칠 수 있어요!
레벨 4가 되서 많이 바쁜 건 알지만 좀 더 학습 테스트를 활용해 보는 건 어떨까요?
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.
accept-count 숫자만큼 기다리다가 타임아웃이 발생하여서 max-commections 숫자만큼만 200을 응답하는군요~ 덕분에 알아갑니다!
| import org.apache.catalina.response.HttpResponse; | ||
| import org.apache.coyote.processor.ControllerMapper; | ||
|
|
||
| public class ServletContainer { |
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.
ServletContainer라고 이름 지은 이유가 궁금해요~
ServletContainer가 어떤 건가요? 지금 이 클래스는 그 역할을 하고 있다고 볼 수 있나요?🤔
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.
서블릿 컨테이너는 서블릿의 실행에 관한 전반적인 영역을 다룹니다. 단순히 매핑되어 있는 걸 호출하기만 하는게 아닌 서블릿의 생명주기도 다루고 멀티 스레드를 통해 리소스도 관리해요. 대표적으로 톰켓이 서블릿 컨테이너의 예시라고 볼 수 있어요
지금 해당 클래스를 ServletContainer라고 부르는 게 맞는 지 다시 고민해보면 좋을 것 같아요
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.
어떤 서블릿과 연결해줄지 결정하고, 서블릿의 service 함수를 호출하는 역할을 하기에 충분히 서블릿 컨테이너라고 이름 붙일 수 있다고 생각해요~
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 클래스와 역할의 정도가 비슷하다고 볼 수 있나요?🤔
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.
서블릿 컨테이너 = 톰캣은 아니라고 생각해요!
톰캣은 connector 역할을 갖는 coyote 부분과 서블릿 엔진(서블릿 컨테이너) 역할을 갖는 catalina 부분으로 나눠져있고
그 중 서블릿 엔진 즉, 서블릿 컨테이너 역할을 하는 부분을 이 클래스로 정의한 것입니다~
https://jiminbyun.medium.com/apache-tomcat-1-core-components-and-their-interactions-939f1f476544

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.
Catalina (The Servlet Engine)
- Catalina is the powerhouse of Tomcat. It’s where the real magic happens.
- When a request arrives, Catalina decides whether it’s a servlet or a JSP (JavaServer Page). If it is a servlet, Catalina takes charge.
- Catalina manages the lifecycle of servlets, including loading, initialization, and handling requests.
커비가 어떤 말을 하는 지는 이해했지만 그게 정말 맞는진 모르겠어요... 단순히 매핑되어 있는 걸 호출하는게 서블릿의 생명 주기를 관리하는 걸로는 받아들일 수 없을 것 같아요!
| import org.apache.catalina.request.HttpRequest; | ||
| import org.apache.catalina.response.HttpResponse; | ||
|
|
||
| public abstract class AbstractController implements Controller { |
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.
질문입니다!
지금 아래 doPost나 doGet을 보면 abstract메소드가 아니기 때문에 이 클래스를 상속받아도 굳이 구현할 필요가 없습니다. 구현을 안 하면 여기에 있는 메소드를 사용하니까요.
그런데 여기에 있는 메소드에서는 아무런 로직이 없네요🤔
만약에 DELETE메소드를 지원하는 url이 생겨서 doDelete라는 메소드가 생겼습니다.
다만 /login은 DELETE를 지원하지 않는다고 할 때 /login을 DELETE로 호출하면 어떻게 되나요?
그 동작이 커비가 의도한 동작인가요?
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 (!isMember(user, password)) { | ||
| response.setStatusCode(StatusCode.UNAUTHORIZED); | ||
| response.setBody("/401.html"); | ||
| } | ||
| if (isMember(user, password)) { |
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 조건문은 상단의 if에서 return하는 방식으로 아래 if문을 없애보는 건 어떨까요?
| if (!isMember(user, password)) { | |
| response.setStatusCode(StatusCode.UNAUTHORIZED); | |
| response.setBody("/401.html"); | |
| } | |
| if (isMember(user, password)) { | |
| if (!isMember(user, password)) { | |
| response.setStatusCode(StatusCode.UNAUTHORIZED); | |
| response.setBody("/401.html"); | |
| return; | |
| } |
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.
이해했습니다!
다만 객체지향 원칙에서 if-else문을 왜 지양하라고 했는지 생각해보면 좋을 것 같았어요!
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-else문은 저도 가독성에 좋지 않다고 생각해요~ else가 있다면 '조건을 만족하지 못할때'라고 의도적으로 생각해야하니까요~ 이렇게 명시적으로 반대 조건문을 적어준다면 의미 파악이 한 눈에 들어온다고 생각합니다!
| @Override | ||
| protected void doGet(HttpRequest request, HttpResponse response) { | ||
| if (hasLogined(request)) { | ||
| response.setStatusCode(StatusCode.FOUND); // |
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.
아차차 수정했습니다ㅎㅎ
| @@ -0,0 +1,53 @@ | |||
| package org.apache.coyote.connector; | |||
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.
클라이언트의 입력을 httpRquest 형태로 변환하는 것은 catalina가 아니라 coyote쪽에서 하는 일이라고 생각해서 옮겨주었습니다~
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.
정확히는 connector 패키지에 위치시킨 이유가 궁금했어요!
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.
inputStream 같은 입력을 request 객체로 변환시키는 것이 connector의 역할 범위라고 알고있습니다~
| } | ||
|
|
||
| private static void setBasicHeader(HttpResponse httpResponse, HttpRequest httpRequest) { | ||
| httpResponse.addHeader(HeaderName.CONTENT_TYPE, httpRequest.getContentType()); |
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.
이모지만 있고 변경사항이 없으면 어떻게 생각한 건지 알 수 가 없어요😭😭
지금 무조건 Content-Type을 설정해주는데 만약 Response의 상태코드가 302여도 Content-Type이 필요한가요?
해당 헤더가 필요없는 상태코드들이 있는데 무조건 넣어주는 게 과연 좋을까요?
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.
확인하고 반영한다는게 깜빡..
맞네요~ body를 세팅해줄 때만 content type을 넣도록 수정해주었습니다~
| import org.apache.catalina.response.HttpResponse; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class AbstractControllerTest { |
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.
get, post 요청이 들어왔을 때 분기처리가 잘되는지 테스트입니다~
| assertAll( | ||
| () -> assertThat(response.getReponse()).contains("200 OK"), | ||
| () -> assertThat(response.getReponse()).contains("<title>대시보드</title>") | ||
| ); |
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.
homeController에서 세팅해주는 것은 저 두개이므로 나머지는 세팅하는 곳의 테스트에서 확인하는 것이 적절하다고 생각했습니다~
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.
homeController에서 Content-Length도 설정하지 않나요?🤔
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.
Content-Length는 homeController가 아닌 HttpResponse 객체의 메서드 책임이므로 해당 객체 테스트에 추가해주었습니다~
| assertAll( | ||
| () -> assertThat(response.getReponse()).contains("302 FOUND"), | ||
| () -> assertThat(response.getReponse()).contains("<title>대시보드</title>") | ||
| ); |
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.
위의 리뷰와 비슷해요! 다른 헤더 값이 잘 설정되어 있는 지 확인 안 해도 되나요?
예를 들어 302면 Location, Body가 있으면 Content-Length, Content-Type과 같은 필요한 값들을 말하는 겁니다!
다른 테스트도 확인해주세요!
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.
??? 그럼 더 이상하네요... 왜 302로 설정해주는데 Location이 없을까요?🤔
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.
컨트롤러 로직 오류를 수정하면서 locaition을 테스트하도록 바꿔주었습니다~
unifolio0
left a comment
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: | ||
| accept-count: 1 | ||
| max-connections: 1 | ||
| max-connections: 2 |
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.
#687 (comment)
해당 테스트 코드를 보면 200을 반환하는 수가 2개면 통과하도록 되어 있어요. 그러면 대기하고 있다가 처리하면 통과하는 게 맞지 않을까요? accept-count도 테스트에 영향을 미칠 수 있어요!
레벨 4가 되서 많이 바쁜 건 알지만 좀 더 학습 테스트를 활용해 보는 건 어떨까요?
| import org.apache.catalina.response.HttpResponse; | ||
| import org.apache.coyote.processor.ControllerMapper; | ||
|
|
||
| public class ServletContainer { |
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.
서블릿 컨테이너는 서블릿의 실행에 관한 전반적인 영역을 다룹니다. 단순히 매핑되어 있는 걸 호출하기만 하는게 아닌 서블릿의 생명주기도 다루고 멀티 스레드를 통해 리소스도 관리해요. 대표적으로 톰켓이 서블릿 컨테이너의 예시라고 볼 수 있어요
지금 해당 클래스를 ServletContainer라고 부르는 게 맞는 지 다시 고민해보면 좋을 것 같아요
| protected void doPost(HttpRequest request, HttpResponse response) throws NoSuchMethodException { | ||
| throw new NoSuchMethodException("HttpMethod not found"); | ||
| } |
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.
정말 질문입니다!
지금 NoSuchMethodException를 던져주고 있는데 해당 예외는 CheckedException이네요!
그러면 적절한 지점에서 예외 처리를 해줘야 합니다! 커비는 해당 예외를 계속 전파해 나가다가 Http11Processor에서 처리해주네요.
왜 CheckedException을 던졌고 해당 예외를 Http11Processor에서 처리했나요? 커비가 해당 예외를 처리하는 역할을 Http11Processor라고 판단한 기준이 궁금합니다!
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.
checkedException인 걸 고려하지 못했네요! 좋은 지적 감사🙏🙏
unCheckedException인 IllegalArgumentException로 변경하였습니다~
| response.setStatusCode(StatusCode.UNAUTHORIZED); | ||
| response.setBody("/401.html"); |
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.
로그인에 실패하면 401.html로 리다이렉트한다.
lms 요구사항과 다른 것 같아요!
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 (!isMember(user, password)) { | ||
| response.setStatusCode(StatusCode.UNAUTHORIZED); | ||
| response.setBody("/401.html"); | ||
| } | ||
| if (isMember(user, password)) { |
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-else문을 왜 지양하라고 했는지 생각해보면 좋을 것 같았어요!
| while ((rawLine = bufferedReader.readLine()) != null && !rawLine.isEmpty()) { | ||
| headerLines.add(rawLine); | ||
| if (rawLine.startsWith("Content-Length")) { | ||
| contentLengthHeader = rawLine.split(": ")[1]; | ||
| } | ||
| } |
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.
구조를 바꾸면 2depth를 줄일 수 있을 것 같긴한데... 이 부분은 굳이 반영 안해도 괜찮아요!
| public String get(String key) { | ||
| if (!body.containsKey(key)) { | ||
| throw new IllegalArgumentException("Body parameter not found"); | ||
| } | ||
| return body.get(key); | ||
| } |
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.
body에 해당 key값이 들어있는지 검증이 필요하지 않을까요?🤔
아마 이 부분 때문에 바꾼 것 같아 보여요.
지금 중점은 톰켓 구현이지만 그래도 서비스 관점에서 고민해봐요~
만약 커비가 로그인을 할려고 하는데 깜빡하고 비밀번호를 입력하지 않았을 때 지금 코드대로면 바로 예외 페이지가 뜨겠네요
그동안 커비가 사용해왔던 서비스는 해당 상황에서 어떻게 처리를 해줬나요? 지금의 예외 처리는 사용자에게 불편함을 주지 않을까요?
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.
맞네요~ 예외 발생하고 적당한 html 페이지가 없어서 우선 500 페이지를 내려주도록 하였습니다~
| assertAll( | ||
| () -> assertThat(response.getReponse()).contains("302 FOUND"), | ||
| () -> assertThat(response.getReponse()).contains("<title>대시보드</title>") | ||
| ); |
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.
??? 그럼 더 이상하네요... 왜 302로 설정해주는데 Location이 없을까요?🤔
| assertAll( | ||
| () -> assertThat(response.getReponse()).contains("200 OK"), | ||
| () -> assertThat(response.getReponse()).contains("<title>대시보드</title>") | ||
| ); |
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.
homeController에서 Content-Length도 설정하지 않나요?🤔
| } | ||
|
|
||
| private static void setBasicHeader(HttpResponse httpResponse, HttpRequest httpRequest) { | ||
| httpResponse.addHeader(HeaderName.CONTENT_TYPE, httpRequest.getContentType()); |
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.
이모지만 있고 변경사항이 없으면 어떻게 생각한 건지 알 수 가 없어요😭😭
지금 무조건 Content-Type을 설정해주는데 만약 Response의 상태코드가 302여도 Content-Type이 필요한가요?
해당 헤더가 필요없는 상태코드들이 있는데 무조건 넣어주는 게 과연 좋을까요?
unifolio0
left a comment
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.
레벨 4 첫 미션 수고하셨습니다!
많이 주고받지 못한 것 같아 아쉽네요! 이만 merge 하겠습니다~
| import org.apache.catalina.response.HttpResponse; | ||
| import org.apache.coyote.processor.ControllerMapper; | ||
|
|
||
| public class ServletContainer { |
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.
Catalina (The Servlet Engine)
- Catalina is the powerhouse of Tomcat. It’s where the real magic happens.
- When a request arrives, Catalina decides whether it’s a servlet or a JSP (JavaServer Page). If it is a servlet, Catalina takes charge.
- Catalina manages the lifecycle of servlets, including loading, initialization, and handling requests.
커비가 어떤 말을 하는 지는 이해했지만 그게 정말 맞는진 모르겠어요... 단순히 매핑되어 있는 걸 호출하는게 서블릿의 생명 주기를 관리하는 걸로는 받아들일 수 없을 것 같아요!
| public static SessionManager getInstance() { | ||
| if (instance == null) { | ||
| return new SessionManager(); | ||
| instance = new SessionManager(); | ||
| } | ||
| return instance; | ||
| } |
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.
저도 미션하면서 알게 된 건데 요즘에는 Bill Pugh Solution 방법으로 싱글톤을 구현한다고 하네요!
안녕하세요 비토!
동시성 관련 적용하여 미션 제출합니다!
즐거운 추석 보내세요🍊
(학습 테스트 중 AppTest는 App을 실행한 채로 돌려야 성공합니다~)