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단계] 스플릿(박상현) 미션 제출합니다. #472

Merged
merged 9 commits into from
Sep 14, 2023

Conversation

splitCoding
Copy link

후추 저번 리뷰 너무 꼼꼼하고 좋았어요!!!👍

이번엔 저번보다 조금 더 많이 얘기 나눠보면 좋을 것 같아요!!❤️

@splitCoding splitCoding self-assigned this Sep 11, 2023
Copy link

@Combi153 Combi153 left a comment

Choose a reason for hiding this comment

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

안녕하세요 스플릿!

3,4 단계 구현하느라 고생하셨습니다!

요구사항을 명확히 지키고, 객체도 적절히 분리한 깔끔한 코드네요

이번에도 리뷰하면서 많이 배울 수 있었습니다 😃

시간이 되신다면 한 가지 제안드리고 싶은 것은 패키지를 적절히 분리하는 것입니다!

wikipedia에 보면 Tomcat의 Components는 Catalina, Coyote, Jasper 세 가지 입니다.
그 중 Catalina 는 servlet container, Coyote는 Connector 라고 소개되어 있네요.

스플릿의 현재 코드는 요구사항에 관한 대부분의 코드가 Coyote 패키지에 있어요. 만약 Catalina에게 servlet container로 책임을 부여한다면, 패키지 구조를 어떻게 바꾸면 좋을지 고민해보시면 좋을 것 같습니다!

물론 저도 Tomcat 구조를 잘 몰라서 고민하는 중입니다 ~ 스플릿은 패키지를 어떤 식으로 구성할지 궁금하네요 😀

Comment on lines +6 to +15
public enum HttpMethod {
GET,
HEAD,
POST,
PUT,
DELETE,
CONNECT,
OPTIONS,
TRACE,
PATCH;

Choose a reason for hiding this comment

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

HEAD, CONNECT 등은 있는 줄도 몰랐는데 정말 꼼꼼하시네요 👍

Comment on lines +25 to +47

- [x] HttpRequest
- [x] Method
- [x] Path
- [x] Protocol Version
- [x] Headers
- [x] Body

- [x] HttpResponse
- [x] Protocol Version
- [x] Status Code
- [x] Status Message
- [x] Headers
- [x] Body

- [x] Controller Interface
- Method : Service
- Parameters : HttpRequest, HttpResponse

- [x] AbstractController
- Implement : Controller
- Method : doGet, doPost
- Parameters : HttpRequest, HttpResponse

Choose a reason for hiding this comment

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

요구사항 정리 굿이요~ 👍

Comment on lines 18 to 21
public static void save(User user) {
user.setId(RESERVED_ID);
user.setId(++RESERVED_ID);
database.put(user.getAccount(), user);
}

Choose a reason for hiding this comment

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

이렇게 했을 때 멀티스레드 환경에서 Id 값이 중복되지 않을까 궁금증이 생겼고 실험을 해보았습니다!

실험 결과를 먼저 말씀드리자면, 멀티스레드에서 Id 값이 중복됩니다. 수정이 필요할 것 같아요!

실험 내용

    public static void save(User user) {
        user.setId(++RESERVED_ID);
        database.put(user.getAccount(), user);
        System.out.println(RESERVED_ID);
    }

우선 InMemoryUserRepository의 save 메서드를 살짝 변경했어요. 테스트를 위해 콘솔에 ID 값을 출력하게 설정했습니다.

class InMemoryUserRepositoryTest {

    @RepeatedTest(1000)
    void multi_threads_test() throws InterruptedException {
        // given
        int numThreads = 3;
        ExecutorService executorService = Executors.newFixedThreadPool(numThreads);

        // when
        executorService.submit(() -> InMemoryUserRepository.save(new User("huchu1", "huchu123", "huchu@naver.com")));
        executorService.submit(() -> InMemoryUserRepository.save(new User("huchu2", "huchu123", "huchu@naver.com")));
        executorService.submit(() -> InMemoryUserRepository.save(new User("huchu3", "huchu123", "huchu@naver.com")));

        executorService.shutdown();
        executorService.awaitTermination(10, TimeUnit.SECONDS);

        // then
        // console에 마지막 숫자가 3000인지 보기
    }
}

그리고 위와 같이 테스트를 작성해서 돌렸습니다.

  • 테스트는 @RepeatedTest로 1000번 실행됩니다.
  • 각 1번의 테스트는 세 개의 스레드가 save 메서드를 호출합니다.
  • 만약 멀티스레드 환경에서 동시성 이슈가 발생하지 않는다면, 콘솔에 1부터 3000까지 출력되어야 합니다.

실험 결과

3000까지 출력되지 않았어요.
스크린샷 2023-09-11 오후 2 57 48

제가 돌렸을 때는 2994까지 출력되었어요
스크린샷 2023-09-11 오후 2 57 27
중간중간 Id가 중복되는 경우가 있더라구요!

추가 실험

그렇다면 synchronized 키워드를 붙이면 3000까지 출력되나 확인해봤습니다.

    public synchronized static void save(User user) {
        user.setId(++RESERVED_ID);
        database.put(user.getAccount(), user);
        System.out.println(RESERVED_ID);
    }

스크린샷 2023-09-11 오후 3 02 38
3000까지 출력되네요!

질문

멀티스레드 환경에서 동시성 이슈를 해결하려면, synchronized 키워드를 붙여야 하지 않을까 생각됩니다!
한편, synchronized를 붙일거면 ConcurrentHashMap 을 사용할 필요가 있을까 생각도 되네요.

이에 대한 스플릿의 생각이 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

답변에 앞서 리뷰 진짜❤️

저도 synchronized 키워드를 쓰면 저장시 동기적으로 처리되는데 ConcurrentHashMap 이 의미가 없다는 생각에 완전 동의합니다!!

AtomicLong 을 사용하면 멀티쓰레드 환경에서 동기적이지 않게 처리가 가능하다 해서 AtomicLong을 사용하여 처리해 보았습니다!!

test 도 따로 만들어서 같이 커밋했습니다!!😊

Choose a reason for hiding this comment

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

AtomicLong 진짜 맛있네요 잘 먹겠습니다 👍

Comment on lines +7 to +23
public class TomcatThreadPool extends ThreadPoolExecutor {

private static final int DEFAULT_CORE_POOL_SIZE = 25;
private static final int DEFAULT_MAX_POOL_SIZE = 200;
private static final int DEFAULT_WORK_QUEUE_SIZE = Integer.MAX_VALUE;
private static final Long DEFAULT_KEEP_ALIVE_TIME = 0L;
private static final TimeUnit DEFAULT_KEEP_ALIVE_TIME_UNIT = TimeUnit.MILLISECONDS;

public TomcatThreadPool() {
super(
DEFAULT_CORE_POOL_SIZE,
DEFAULT_MAX_POOL_SIZE,
DEFAULT_KEEP_ALIVE_TIME,
DEFAULT_KEEP_ALIVE_TIME_UNIT,
new LinkedBlockingQueue<>(DEFAULT_WORK_QUEUE_SIZE)
);
}

Choose a reason for hiding this comment

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

이런 방법은 신박하네요! 👍
특별히 이렇게 ThreadPoolExecutor 를 사용하신 이유가 있을까요?
Executors가 제공하는 정적 팩터리 메서드를 사용하지 않고 직접 생성했을 때 어떤 장점과 단점을 느끼셨는지 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

Executors 에 corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue 를 모두 설정할 수 있는 정적 팩토리 메서드가 존재하지 않아서 직접 만들었습니다!! 😊

장점은 TomcatThreadPool 을 원하는 방식으로 생성할 수 있다는 것 이였습니다!!
느낀 단점은 딱히 없었던것 같아요!!

Comment on lines 73 to 94
if (!headers.containsHeader(HeaderName.CONTENT_LENGTH.getValue())) {
return HttpRequest.builder(start)
.headers(headers)
.build();
}

if (headers.containsHeader(HeaderName.CONTENT_LENGTH.toString())) {
final String contentLengthValue = headers.getHeaderValue(contentLengthHeader);
final int bodyLength = Integer.parseInt(contentLengthValue.strip());
final String contentLengthValue = headers.getHeaderValue("Content-Length");
final int bodyLength = Integer.parseInt(contentLengthValue.strip());

final StringBuilder bodyBuilder = new StringBuilder();
final StringBuilder bodyBuilder = new StringBuilder();

int totalRead = 0;
final char[] buffer = new char[DEFAULT_BUFFER_SIZE];
int totalRead = 0;
final char[] buffer = new char[DEFAULT_BUFFER_SIZE];

while (totalRead < bodyLength) {
final int readBytesCount =
bufferedReader.read(buffer, 0, Math.min(buffer.length, bodyLength - totalRead));
if (readBytesCount == -1) {
break;
}
bodyBuilder.append(buffer, 0, readBytesCount);
totalRead += readBytesCount;
while (totalRead < bodyLength) {
final int readBytesCount =
bufferedReader.read(buffer, 0, Math.min(buffer.length, bodyLength - totalRead));
if (readBytesCount == -1) {
break;
}

body = bodyBuilder.toString();
bodyBuilder.append(buffer, 0, readBytesCount);
totalRead += readBytesCount;

Choose a reason for hiding this comment

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

  1. 메서드가 더 길어서 적절히 분리해도 좋을 것 같아요!
  2. 지난 커멘트에 답변 잘 보았어요! 로직이 이해가 잘 되었습니다. 다만 한 가지 궁금증이 남았어요. buffer를 초기화할 때 DEFAULT_BUFFER_SIZE로 하시는 이유가 있으신가요? contentLengthValue로 초기화하면 메모리 상 이점을 얻지 않을까 생각이 되어서요!

Copy link
Author

Choose a reason for hiding this comment

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

  1. 저도 개인적으로 가독성을 위해서 메서드 분리에 대해서 많이 고민했었어요🧐 하지만 개인적으로 BufferedReader 를 한곳에서 절차적으로 사용하게 강제하고 싶어 분리하지 않고 한메서드로 사용하는 방법을 택했습니다. 😊
    ( private 메서드 마저도 내부적으로 잘못된 순서로 호출될 수 없게 하고 싶어서 분리하지 않는 것이 좋다고 판단했어요!! )

  2. contentLengthValue 가 훨씬 좋네요!!! 수정하겠습니다!!

Choose a reason for hiding this comment

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

오 그런 이유가 있었군요! 무엇인가를 읽는 작업은 반드시 정해진 절차를 지켜야 할텐데, 그럴 때는 스플릿처럼 절차를 강제하는 것도 좋은 것 같습니다. 저도 다음에 해봐야겠어요~

Comment on lines 12 to 18
try {
final HttpResponse newResponse =
StaticResourceResponseSupplier.getWithExtensionContentType(request.getPath());
response.update(newResponse);
} catch (IOException e) {
throw new ResourceLoadingException(request.getPath());
}

Choose a reason for hiding this comment

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

이런 식으로 각각 컨트롤러에서 예외처리를 해주니까 좋네요! 좋은 방법 배워갑니다! 😃

Comment on lines +3 to +8
public class InvalidHttp11ProtocolException extends RuntimeException {

public InvalidHttp11ProtocolException(final String httpVersion) {
super("HTTP/1.1 버전만 지원합니다. version : " + httpVersion);
}
}

Choose a reason for hiding this comment

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

꼼꼼한 예외처리 배워갑니다~

Comment on lines 3 to +8
public enum ContentType {
CSS(".css", "text/css"),
HTML(".html", "text/html"),
HTML_UTF8(".html", "text/html;charset=utf-8"),
JAVASCRIPT(".js", "text/javascript");

private final String extension;
private final String type;
CSS("text/css"),
HTML("text/html"),
HTML_UTF8("text/html;charset=utf-8"),
TEXT_PLAIN("text/plain"),
JAVASCRIPT("text/javascript");

Choose a reason for hiding this comment

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

ContentType에서 파일 확장자를 제거한 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

사람이 읽을 수 있는 모든 문자열 파일들은 text/plain 으로 처리할 수 있는데 이때 특정 확장자를 TEXT_PLAIN 에 지정하기에는 어려움이 있을 것 으로 판단했습니다!!

그래서 외부에서 정적 리소스 확장자에 정확히 맵핑된 Content-Type 을 매핑해주는 방법을 택했습니다!!😊

근데 지금보니 미션 요구사항을 넘어서서 ContentType에 대해 고민해본다면 관리하는 곳이 두 곳이 되어 관리의 어려움이 동반 될 것 같네요ㅠㅠ Content-Type을 정할 수 있는 다른 방식의 메서드를 제공하고 Enum 안에서 모두 관리하는 방법이 더 좋아보입니다!!👍

Choose a reason for hiding this comment

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

Content-Type을 정할 수 있는 다른 방식의 메서드를 제공하고 Enum 안에서 모두 관리하는 방법

스플릿이 말씀하신 걸 저도 구현해보고 싶었는데 쉽지 않더라구요! 나중에 여유가 있을 때 도전해보면 좋을 것 같아요 ~

Comment on lines +66 to +79
public void update(final HttpResponse other) {
if (this.statusLine != other.statusLine) {
this.statusLine = other.statusLine;
}
if (!this.headers.containsAll(other.headers)) {
this.headers = other.headers;
}
if (!this.cookie.getValues().equals(other.getCookie().getValues())) {
this.cookie = other.cookie;
}
if (!this.body.equals(other.body)) {
this.body = other.body;
}
}

Choose a reason for hiding this comment

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

setter 등의 메서드를 통해 Response를 완성시키는 건 어떠신가요?

예를 들어, 지금은 redirect 하는 response를 만들 때 HttpResponse.redirect()로 객체를 생성하고 update() 메서드로 필드를 갱신하고 있어요. 그런데 setRedirectResponse()와 같은 메서드를 만들면, 메서드를 한 번만 호출해도 괜찮지 않을까 생각이 들었어요.

처음에 update 메서드를 보고 이해하기 힘들었기에 제안드려봅니다!

제 생각과 스플릿의 생각이 다르다면, 취향차이~

Copy link
Author

Choose a reason for hiding this comment

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

후추의 방법도 좋다고 생각합니다!!!!👍

지금 코드가 정적 팩토리 메서드로 어느정도 특정 response에 대한 생성을 만들어 놓아서 똑같은 내용의 코드가 될 것 같아서 우선 이렇게 작성했었습니다!! 🫡

Comment on lines 47 to 53

final Optional<User> findUser = InMemoryUserRepository.findByAccount(account);
if (findUser.isPresent() && findUser.get().checkPassword(password)) {
loginSuccess(response, findUser.get());
return;
}
loginFail(request, response);

Choose a reason for hiding this comment

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

지금 만들어둔 controller에서 Login, Register 기능을 수행하고 있어요. 이러한 기능을 AuthService 같은 객체를 만들어 책임을 주면 어떨까요? Controller에서는 응답만 구성해도 될 것 같아요 😃

Copy link
Author

@splitCoding splitCoding Sep 14, 2023

Choose a reason for hiding this comment

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

객체 지향이 관점에서 AuthService 생성하는 것 매우 좋은 방법이라고 생각합니다!!👍

개인적으로는 이번 미션의 각각의 controller가 서블릿이고 요청에 따른 처리를 하는 서블릿의 생성에 집중해보는 것 같다고 생각해서 이렇게만 진행했던 것 같아요ㅎㅎ

Choose a reason for hiding this comment

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

맞습니다! 저도 미션에서 배울 수 있는 것에 집중하는 게 좋다고 생각해요. 특히나 바쁜 레벨4 미션에서는 더 그렇구요. 앞으로도 각 미션에서 뽑아먹을 수 있는 걸 다 뽑아먹자구요!!

@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 1 Code Smell

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

@Combi153 Combi153 left a comment

Choose a reason for hiding this comment

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

스플릿 안녕하세요! 후추입니다 😀

3,4 단계 미션 구현하시느라 고생하셨어요!

이번 미션에서 요구사항을 모두 지키셨고, 여러 방면에서 충분히 학습하셨다고 판단해요!

이만 머지하겠습니다 고생하셨어요~

남은 레벨4 화이팅 🚀

Comment on lines +43 to +49
if (!executorService.isTerminated()) {
logger.info("terminating threads");
executorService.awaitTermination(1, TimeUnit.SECONDS);
}

logger.info("terminating finished");
executorService.shutdown();

Choose a reason for hiding this comment

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

스플릿은 log를 자유자재로 다룰 줄 아시는 것 같아요 👍
저는 log에 대해 깊게 생각해본 적이 없는데 배워갑니다 😃

Comment on lines +21 to +27
void multi_threads_test() throws InterruptedException {
// given
int numThreads = 1000;
ExecutorService executorService = Executors.newFixedThreadPool(numThreads);
final CountDownLatch saveLatch = new CountDownLatch(1);

// when

Choose a reason for hiding this comment

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

콘솔에 ID를 찍어보는 제 테스트보다 훨씬 낫군요! 멋진 테스트에요 💯

import nextstep.jwp.model.User;

public class InMemoryUserRepository {

private static Long RESERVED_ID = 0L;
private static AtomicLong RESERVED_ID = new AtomicLong(0);

Choose a reason for hiding this comment

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

AtomicLong 란 게 있는 줄 몰랐는데 대단해요!
synchronized 키워드를 붙이는 것보다 훨씬 성능이 좋을 것 같아요.

Comment on lines 1 to 4
package org.apache.catalina;

import java.util.HashMap;
import java.util.Map;

Choose a reason for hiding this comment

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

ControllerMapper 가 catalina 패키지로 이동함으로써, Servlet container 역할을 적절히 수행하고 있네요 👍

Comment on lines 47 to 53

final Optional<User> findUser = InMemoryUserRepository.findByAccount(account);
if (findUser.isPresent() && findUser.get().checkPassword(password)) {
loginSuccess(response, findUser.get());
return;
}
loginFail(request, response);

Choose a reason for hiding this comment

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

맞습니다! 저도 미션에서 배울 수 있는 것에 집중하는 게 좋다고 생각해요. 특히나 바쁜 레벨4 미션에서는 더 그렇구요. 앞으로도 각 미션에서 뽑아먹을 수 있는 걸 다 뽑아먹자구요!!

Comment on lines 3 to +8
public enum ContentType {
CSS(".css", "text/css"),
HTML(".html", "text/html"),
HTML_UTF8(".html", "text/html;charset=utf-8"),
JAVASCRIPT(".js", "text/javascript");

private final String extension;
private final String type;
CSS("text/css"),
HTML("text/html"),
HTML_UTF8("text/html;charset=utf-8"),
TEXT_PLAIN("text/plain"),
JAVASCRIPT("text/javascript");

Choose a reason for hiding this comment

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

Content-Type을 정할 수 있는 다른 방식의 메서드를 제공하고 Enum 안에서 모두 관리하는 방법

스플릿이 말씀하신 걸 저도 구현해보고 싶었는데 쉽지 않더라구요! 나중에 여유가 있을 때 도전해보면 좋을 것 같아요 ~

Comment on lines 73 to 94
if (!headers.containsHeader(HeaderName.CONTENT_LENGTH.getValue())) {
return HttpRequest.builder(start)
.headers(headers)
.build();
}

if (headers.containsHeader(HeaderName.CONTENT_LENGTH.toString())) {
final String contentLengthValue = headers.getHeaderValue(contentLengthHeader);
final int bodyLength = Integer.parseInt(contentLengthValue.strip());
final String contentLengthValue = headers.getHeaderValue("Content-Length");
final int bodyLength = Integer.parseInt(contentLengthValue.strip());

final StringBuilder bodyBuilder = new StringBuilder();
final StringBuilder bodyBuilder = new StringBuilder();

int totalRead = 0;
final char[] buffer = new char[DEFAULT_BUFFER_SIZE];
int totalRead = 0;
final char[] buffer = new char[DEFAULT_BUFFER_SIZE];

while (totalRead < bodyLength) {
final int readBytesCount =
bufferedReader.read(buffer, 0, Math.min(buffer.length, bodyLength - totalRead));
if (readBytesCount == -1) {
break;
}
bodyBuilder.append(buffer, 0, readBytesCount);
totalRead += readBytesCount;
while (totalRead < bodyLength) {
final int readBytesCount =
bufferedReader.read(buffer, 0, Math.min(buffer.length, bodyLength - totalRead));
if (readBytesCount == -1) {
break;
}

body = bodyBuilder.toString();
bodyBuilder.append(buffer, 0, readBytesCount);
totalRead += readBytesCount;

Choose a reason for hiding this comment

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

오 그런 이유가 있었군요! 무엇인가를 읽는 작업은 반드시 정해진 절차를 지켜야 할텐데, 그럴 때는 스플릿처럼 절차를 강제하는 것도 좋은 것 같습니다. 저도 다음에 해봐야겠어요~

Comment on lines 18 to 21
public static void save(User user) {
user.setId(RESERVED_ID);
user.setId(++RESERVED_ID);
database.put(user.getAccount(), user);
}

Choose a reason for hiding this comment

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

AtomicLong 진짜 맛있네요 잘 먹겠습니다 👍

@Combi153 Combi153 merged commit f8d96c2 into woowacourse:splitcoding 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.

2 participants