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

[4단계 - Tomcat 구현하기] 비토(오상훈) 미션 제출합니다. #687

Merged
merged 11 commits into from
Sep 14, 2024

Conversation

unifolio0
Copy link

@unifolio0 unifolio0 commented Sep 13, 2024

안녕하세요 이상!
4단계 구현 완료하고 리뷰 요청 보냅니다~

저번에 3,4단계 한번에 내고 다시 롤백하고 보냈을때 4단계 학습테스트 일부를 롤백을 깜빡해서 file changed에 없는 테스트도 있습니다!

Copy link

@kunsanglee kunsanglee left a comment

Choose a reason for hiding this comment

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

안녕하세요 비토!
저번 변경사항때 제대로 확인하지 못하고 넘어가서 죄송합니다🥲
수정이 필요한 부분과 학습 관점에서 질문 코멘트로 남겼어요.
확인 후 다시 리뷰요청 부탁드려요! 😇

@@ -3,8 +3,8 @@ handlebars:

server:
tomcat:
accept-count: 1
max-connections: 1
accept-count: 2

Choose a reason for hiding this comment

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

accept-count는 무엇이고 어떤 이유로 값을 2로 설정했나요?

Copy link
Author

Choose a reason for hiding this comment

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

accept-countmax-connections이상의 요청이 들어왔을 때 max-connections를 초과한 수만큼 큐에 담아두는데 이때 담는 큐의 사이즈를 의미합니다!

지금 다시 생각해보니 굳이 바꿀 이유가 없었네요...

accept-count: 1
max-connections: 1
accept-count: 2
max-connections: 2

Choose a reason for hiding this comment

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

max-connections는 무엇을 의미하고 이것 또한 어떤 이유로 값을 2로 설정했나요?

Copy link
Author

@unifolio0 unifolio0 Sep 14, 2024

Choose a reason for hiding this comment

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

max-connections는 서버가 동시에 처리할 수 있는 최대 연결 수를 의미합니다!

사실 위의 답글에 accept-count를 바꿀 필요가 없다고 했었지만 max-connections의 값과 accept-count의 합을 테스트에서 기대되는 값만큼으로 설정하고 TestHttpUtils에서 timeout의 값을 충분히 높게 설정해주면 테스트가 통과합니다!

하지만 가능한 적은 변화로 테스트가 통과되길 원해서 max-connections를 테스트에서 기대되는 값만큼으로 설정해서 2개를 제외한 나머지 스레드는 timeout이 발생해서 incrementIfOk에서 값을 증가시키지 못하도록 설정했습니다!

Comment on lines 9 to 10
min-spare: 2
max: 2

Choose a reason for hiding this comment

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

min-spare, max 또한 설명 부탁드려요!

Copy link
Author

Choose a reason for hiding this comment

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

  • min-spare는 스레드 풀에 대기 상태로 있을 수 있는 스레드 개수 입니다! 요청이 없어도 해당 개수만큼의 스레드를 유지하도록 하여 서버의 반응성을 높일 수 있습니다.
  • max는 동시에 실행할 수 있는 최대 스레드의 개수입니다!

}

public void stop() {
stopped = true;
executor.shutdown();

Choose a reason for hiding this comment

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

스레드 풀은 종료했는데, 소켓이 열려있어서 클라이언트가 연결을 시도하면 서버가 잘 종료 되나요?? 😣

Copy link
Author

Choose a reason for hiding this comment

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

검색해보니 이미 accept가 된 소켓이 있으면 close wait 상태에 빠질 수 있네요! finally구문을 통해 명시적으로 소켓을 닫도록하여 보완했습니다!

@@ -66,11 +71,12 @@ private void process(final Socket connection) {
return;
}
var processor = new Http11Processor(connection);
new Thread(processor).start();
executor.execute(processor);

Choose a reason for hiding this comment

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

지난 변경 사항이 아니라서 여기다 코멘트 남겨요!
Connector의 start 메서드도 ExecutorService를 사용하도록 변경해주세요~~

Copy link
Author

Choose a reason for hiding this comment

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

start 메소드도 스레드 풀을 사용하도록 수정했습니다!

Copy link

@kunsanglee kunsanglee left a comment

Choose a reason for hiding this comment

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

비토!
질문에 답도 잘 해주셨고 반영도 잘 해주셨네요 😇
고생 많으셨어요

머지 보류입니다.
Connector 클래스 테스트 부탁드려요 😅

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