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 구현하기] 몰리(김지민) 미션 제출합니다. #689

Merged
merged 22 commits into from
Sep 19, 2024

Conversation

jminkkk
Copy link

@jminkkk jminkkk commented Sep 13, 2024

안녕하세요 산초 😁
이제 추석인데 재미있는 명절 보내시길 바랍니다 ㅎㅎ

이번 pr에는 기존 apache 코드에 존재하던 애플리케이션단 로직을 제거하고자 했어요!

사용이 어색한 부분 있다면 말해주세요 :)
이번 리뷰도 잘 부탁드립니다!

Copy link

@nayonsoso nayonsoso left a comment

Choose a reason for hiding this comment

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

안녕하세요, 몰리! 4단계 코드도 잘 작성해주셨네요.
몰리의 코드를 읽으며, 인스턴스를 재사용하는 싱글톤 패턴이나 에러를 핸들링하는 부분등을 배워갈 수 있었습니다.
그리고 테스트 코드도 촘촘해서 읽기 편했던 것 같아요. 정말 많이 배웠습니다🙇🏻‍♀️
개인적으로 궁금증이 남는 부분들 & 조금 더 발전시킬만한 부분들을 코멘트로 남겨두었습니다.
그럼 추석 잘 보내시 바랍니다🌝

Comment on lines +8 to +17
public class ExceptionMapping implements ExceptionHandler {

private static final ExceptionMapping INSTANCE = new ExceptionMapping();

private ExceptionMapping() {
}

public static ExceptionMapping getInstance() {
return INSTANCE;
}

Choose a reason for hiding this comment

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

예외 핸들링을 이렇게도 할 수 있군요,.!
저는 try catch 만 사용했었는데 한수 배우고 갑니다☺️

Comment on lines 8 to 11
public class ControllerMapping implements HandlerMapping {
private static final ControllerMapping INSTANCE = new ControllerMapping();
private static final String PATH_DELIMITER = "/";

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.

기본적인 것인데 계속 놓치네요 😭
수정하겠습니다!

Comment on lines 6 to 13
public class Application {

public static void main(String[] args) {
final var tomcat = new Tomcat();
final Server server = new MyServer();
final var tomcat = new Tomcat(server);
tomcat.start();
}
}
Copy link

@nayonsoso nayonsoso Sep 16, 2024

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.

앗 현재는 Server를 ApplicationContext 로 변경했으니 ApplicationContext를 기준으로 설명해볼게요!

해당 커밋 이전에는 org.apache의 Http11Processor 내부에서 HandlerMapping의 인스턴스를 호출했습니다.
HandlerMapping에는 com.techcourse에 있어야 하는 로직들이 작성되어 있었구요.
때문에 org.apache는 현재 com.techcourse이라는 특정 애플리케이션만 처리가 가능했었습니다.

하지만 저는 제가 구현한 톰캣이 특정 애플리케이션에 종속되지 않고 다양한 애플리케이션에서 재사용 가능하도록 하고 싶었습니다.
따라서 애플리케이션에 따라 다른 것들은 외부에서 주입을 받고 톰캣은 어떤 애플리케이션이 실행되는지 모른 채로 요청을 처리하도록 변경했습니다!

Copy link
Author

Choose a reason for hiding this comment

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

+) 추가로 ApplicationContext과 Context가 같은 의미에서 사용되는 줄 알았는데, 다른 의미이더라구요ㅎ

ApplicationContext는 빈 관리와 DI(의존성 주입)의 목적인데, Context(== ServletContext)는 전체 웹 애플리케이션의 컨텍스트를 포함하는 것이라고 합니다.
덕분에 착각할뻔한 개념을 잡았어요 🙇🏻‍♀️

Choose a reason for hiding this comment

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

하지만 저는 제가 구현한 톰캣이 특정 애플리케이션에 종속되지 않고 다양한 애플리케이션에서 재사용 가능하도록 하고 싶었습니다. 따라서 애플리케이션에 따라 다른 것들은 외부에서 주입을 받고 톰캣은 어떤 애플리케이션이 실행되는지 모른 채로 요청을 처리하도록 변경했습니다!

이런 의도가 있었군요,
특정 구현체에 의존적이지 않도록 한다는게 객체지향의 원칙을 지키는 것 같아 좋은 시도 같습니다👍👍

Copy link

@nayonsoso nayonsoso Sep 19, 2024

Choose a reason for hiding this comment

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

그리고 ApplicationContext과 Context에 대해서는 몰리 덕분에 저도 차이점을 배워가네요 ㅎㅎ
정리해주셔서 감사합니다~

Comment on lines 66 to 67
Socket connection = serverSocket.accept();
executorService.submit(() -> process(connection));
Copy link

@nayonsoso nayonsoso Sep 16, 2024

Choose a reason for hiding this comment

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

추가로, 사실 이건 제게도 어려운 부분이긴 한데..!
쓰레드풀을 설정한 것이 잘 작동하는지 테스트할 수도 있다고 하더라고요.
4단계에서 쓰레드풀을 학습하는김에, Connector 에서 만들어둔 쓰레드풀대로 잘 작동하는지 테스트 코드를 작성해보는건 어떨까요?
몰리의 학습을 위해 제안드려봅니다!

e.g. 아모씨의 코드 : https://github.com/woowacourse/java-http/blob/3e65ff2537714b88f230711ab37e3a1037ce667f/tomcat/src/test/java/org/apache/catalina/connector/ConnectorTest.java

Copy link
Author

@jminkkk jminkkk Sep 18, 2024

Choose a reason for hiding this comment

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

이 부분을 아래의 경우로 테스트 해보고자 노력했으나,,,

  • 동시 요청이 최대 쓰레드 이하일 때 모든 요청 처리 성공
  • 동시 요청이 최대 쓰레드 이상이며, 최대 요청보다 많고 대기 큐보다 적을 때 일부 요청 대기
  • 동시 요청이 최대 쓰레드 이상이며, 대기 큐보다 클 때 일부 요청 거절

왜인지 모르게 전체 케이스에 대해 ThreadPoolExecutor가 모든 요청을 처리해버려 대기와 거절에 대한 테스트할 수 없더라구요 🥲
일단 동시에 처리가 가능한지에 대해서만 테스트를 추가해놓았는데, 혹시 산초가 보시기에 예측되는 이유가 있을까요...?
테스트 코드

Copy link

@nayonsoso nayonsoso Sep 19, 2024

Choose a reason for hiding this comment

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

먼저, 몰리의 코드를 전체적으로 다시 살펴봤는데, 제가 지난 리뷰에서 놓친 부분을 발견했어요.


지금의 몰리 코드에서는 '고정 크기의 쓰레드풀'이 제대로 설정되어있는 것 같지 않아요.
바로 아래 부분 때문인데요,

private void connect() {
    try {
        Socket connection = serverSocket.accept();
        threadPoolExecutor.submit(() -> process(connection)); // 1️⃣
    } catch (IOException e) {
        log.error(e.getMessage(), e);
    }
}

private void process(final Socket connection) {
    if (connection == null) {
        return;
    }
    var processor = new Http11Processor(connection, container);
    new Thread(processor).start(); // 2️⃣
}

1️⃣ 라인을 보면, 쓰레드풀의 쓰레드를 사용해서 process() 함수를 호출하고 있어요.
하지만 2️⃣ 에서 쓰레드 풀과 관계 없이 새로운 쓰레드를 생성하고 있어요.
즉, 반복적으로 process() 메서드를 처리하기 위한 쓰레드는 쓰레드 풀의 쓰레드를 사용하고 있지만
processor 를 처리하기 위한 쓰레드는 그와 관계 없는 쓰레드에요.


제가 몰리의 코드에 프린트문을 넣어서 요청을 1000개 보내봤어요 (Jmeter 이용)

private void process(final Socket connection) {
    if (connection == null) {
        return;
    }
    System.out.println("process를 호출하는 쓰레드의 이름 : " + Thread.currentThread().getName()); // ❗️ 이 라인 추가
    var processor = new Http11Processor(connection, container);
    new Thread(processor).start();
}

그랬더니 아래와 같은 출력이 나왔어요. 보다시피,
process() 메서드를 처리하기 위한 쓰레드는 재사용되고 있지만,
new Thread(processor).start(); 부분 때문에 계속해서 새로운 쓰레드가 생겨나고 있어요.

image


코드를 아래와 같이 바꿔야 요청을 처리하는 쓰레드를 쓰레드 풀의 쓰레드로 한정할 수 있어요.

private void connect() {
    try {
        Socket connection = serverSocket.accept();
        process(connection);
    } catch (IOException e) {
        log.error(e.getMessage(), e);
    }
}

private void process(final Socket connection) {
    if (connection == null) {
        return;
    }
    // System.out.println("process를 호출하는 쓰레드의 이름 : " + Thread.currentThread().getName());
    Http11Processor processor = new Http11Processor(connection, container);
    threadPoolExecutor.submit(processor); // ❗️processor 처리할 때, 쓰레드풀을 사용하도록 변경
}

아래 사진은 같은 요청을 보내봤을 때의 출력 내용이에요. 250개의 쓰레드가 재사용되는 것을 볼 수 있어요.

image

Choose a reason for hiding this comment

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

왜인지 모르게 전체 케이스에 대해 ThreadPoolExecutor가 모든 요청을 처리해버려 대기와 거절에 대한 테스트할 수 없더라구요

지금의 테스트 코드에서는 주어진 요청을 다 보낸 다음, 테스트코드의 assert문이 동작하게 되어있어요.
그런데 쓰레드 250개가 가득 차고, 50개가 대기하고 있는 상황을 연출하려면
assert 문에 도달할 때 "쓰레드가 요청을 처리하는 중"이어야 해요.
그렇지 않다면, 250개의 요청을 처리하고 대기하는 50개의 요청마저 다 처리해버리겠죠! 실제 서버가 그러하듯이요,

따라서 아래와 같이 CountDownLatch 를 사용해서 쓰레드의 작업을 지연시키는 방법을 쓴다면, 테스트코드가 통과할거에요.

@BeforeEach
void setUp() {
    testEndLatch = new CountDownLatch(1);
    context = new MyContext();
    connector = new Connector(context);

    // 요청 중 한 과정을 모킹해서 테스트 종료 시까지 대기하게 설정

    connector.start();
}

@AfterEach
void cleanUp() {
    testEndLatch.countDown();
}

하지만 제가 이런 테스트코드를 만드려고 세시간정도의 시간을 썼음에도...🫠
테스트에 실패했습니다.
static 함수인 getInstance() 들로 필드를 초기화하는게 많아서, 어느 하나를 모킹하기가 쉽지가 않았네요....
static 함수의 단점을 여기서 한번 더 느끼고 갑니다😓

그리고 모킹 대신 sleep() 을 넣어보기도 했는데, 그렇게 하니 '요청이 완료될 때 까지' 기다려서
역시나 '250'개의 쓰레드가 요청을 처리중인 상태를 연출하지 못했습니다.
제가 테스트 코드를 잘 작성하지 못 했던 것일수도 있고요..

Choose a reason for hiding this comment

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

무튼 몰리의 테스트를 통과하게 하고 싶어서 저도 이런저런 시도를 해보았으나 실패를 했습니다😭
그런데 그런 이번 단계에서의 핵심 요소는 아닌 것 같아서, 다시 RC 드리진 않겠습니다!
이 테스트에 대해서는 각자 공부하는 걸로 하고 (아톰의 코드를 여러번 봐야겠네요 ㅎㅎ) 이만 넘어가도 괜찮을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

Connector가 ThreadPoolExecutor의 쓰레드를 재사용하지 않고 있었군요..
꼼꼼히 찾아봐주셔서 감사합니다 왕감동이에요 🙇🏻‍♀️

무튼 몰리의 테스트를 통과하게 하고 싶어서 저도 이런저런 시도를 해보았으나 실패를 했습니다😭
그런데 그런 이번 단계에서의 핵심 요소는 아닌 것 같아서, 다시 RC 드리진 않겠습니다!
이 테스트에 대해서는 각자 공부하는 걸로 하고 (아톰의 코드를 여러번 봐야겠네요 ㅎㅎ) 이만 넘어가도 괜찮을 것 같습니다

좋아요!
추후에 개선된 코드를 고민해보고 공유드리겠습니다 열심히 봐주셔서 감사해요 😍

Comment on lines +3 to +10
public abstract class ApplicationConfig {

private static final int DEFAULT_PORT = 8080;
private static final int DEFAULT_ACCEPT_COUNT = 100;
private static final int DEFAULT_MAX_THREADS = 250;

private Integer port = DEFAULT_PORT;
private Integer acceptCount = DEFAULT_ACCEPT_COUNT;

Choose a reason for hiding this comment

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

이거 진짜 너무 좋은 것 같아요.. 순수 감탄..!
저도 이렇게 할 걸 그랬어요.. 🥹👏

final var socket = new StubSocket(httpRequest);
final Http11Processor processor = new Http11Processor(socket);
final var processor = new Http11Processor(socket, container);

Choose a reason for hiding this comment

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

변수 타입을 var 로 바꿔준 이유가 있으신가요? 😳

Copy link
Author

Choose a reason for hiding this comment

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

앗 아니요 🥲
Container를 추가하면서 무지성으로 제일 위 메서드를 복붙하다가 들어갔나봐요 ...
다시 변경하겠습니다!

Copy link

@nayonsoso nayonsoso left a comment

Choose a reason for hiding this comment

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

안녕하세요 몰리, 남겨주신 코멘트에 저도 답글 달았습니다😊
제가 제안드린 것들에 대해서 열심히 알아보셔서 감동을 받았습니다 ㅎㅎ❤️
이번 단계에서 학습할 것들은 충분히 학습하신 것 같아 이만 머지합니다.

@nayonsoso nayonsoso merged commit 58c94f3 into woowacourse:jminkkk Sep 19, 2024
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.

2 participants