[톰캣 구현하기 - 3, 4단계] 로이스(원태연) 미션 제출합니다. #429

TaeyeonRoyce · 2023-09-09T09:38:03Z

안녕하세요 마코!
3, 4단계 리뷰 요청 드립니다

이전에 이미 존재했던 코드들을 사용해서 구조가 크게 변경 되지 않았습니다.
전에 controller에서 ResponseEntity를 반환하여 적절한 response를 만들었는데, 불필요한 계층이라 판단하여 제거하였습니다!

aak2075

안녕하세요 로이스~

테스트도 꼼꼼히 작성해 주시고 미션도 깔끔하게 잘 구현해주셨네요 👍

이전 미션에서 구조를 잘 잡아 놓으셔서 편하셨을 것 같습니다 😄

application을 실행시키면 접속시 에러가 발생하는 부분이 있어서 예상되는 부분에 코멘트 남겼습니당.

aak2075 · 2023-09-10T08:11:59Z

study/src/main/java/cache/com/example/cachecontrol/CacheControlInterceptor.java

+    ) {
+        final String cacheControl = CacheControl
+                .noCache()
+                .cachePrivate()


no-cache와 no-store
private과 public 각각 어떤 차이가 있었나요?

모두 응답 헤더 Cache-control에 설정하여 캐시에 대한 정책에 대해 명시하는 것 입니다.
no-cache는 캐시를 저장은 해도 되지만, 매 요청마다 revalidate(재검증..?)가 필요하다는 정책이고 no-store는 캐시를 아예 저장하지 말라는 뜻입니다.

private과 public의 차이점은 캐싱의 범위입니다. CDN이나 다른 proxy 서버에 캐싱을 저장할 수 있는 public과 달리, private은 클라이언트에서만 캐싱할 수 있습니다.

이미지나 로고 같이 모든 사용자에게 보여지는 민감하지 않은 정보는 public, 사용자와 관련된 데이터는 private으로 설정하는 식으로 목적에 맞게 관리 할 수 있을 것 같습니다!

aak2075 · 2023-09-10T08:19:37Z

study/src/main/java/cache/com/example/etag/EtagFilterConfiguration.java

+    @Bean
+    public FilterRegistrationBean<ShallowEtagHeaderFilter> shallowEtagHeaderFilter() {
+        FilterRegistrationBean<ShallowEtagHeaderFilter> filterRegistrationBean =
+                new FilterRegistrationBean<>(new ShallowEtagHeaderFilter());


ShallowEtagHeaderFilter의 역할은 무엇인가요?

ETag를 생성한 뒤 응답에 포함시켜 반환합니다!

aak2075 · 2023-09-10T09:55:24Z

tomcat/src/main/java/org/apache/coyote/http11/controller/AbstractController.java

+    private final Map<HttpMethod, BiConsumer<HttpRequest, HttpResponse>> supportedMethods = Map.of(
+            HttpMethod.GET, this::doGetRequest,
+            HttpMethod.POST, this::doPostRequest
+    );


함수형 인터페이스 잘 쓰시네요👍
잘 배워갑니다

aak2075 · 2023-09-10T10:02:39Z

tomcat/src/main/java/org/apache/coyote/http11/controller/AbstractController.java

+        supportedMethods.get(httpMethod).accept(request, response);
+    }
+
+    protected void doGetRequest(final HttpRequest request, final HttpResponse response) {


요건 굳이 안바꿔주셔도 되지만
javax.servlet.http.ServletHttp 에서는 doGet, doPost와 같이 명명했습니다.
다 request를 처리하는 메서드이기도 하고, 중복 및 이름 길이 측면에서도 Request라는 접미사는 빠져도 괜찮을것 같습니다.

명시적이라 생각해서 붙혀두었는데 doGet도 충분한것 같네요!
반영해두었습니다

aak2075 · 2023-09-10T10:10:04Z

tomcat/src/main/java/org/apache/coyote/http11/common/SessionManager.java


 public class SessionManager {

-    private static final Map<String, Session> SESSIONS = new HashMap<>();
+    private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();


동시성 컬렉션 👍

aak2075 · 2023-09-10T12:37:20Z

tomcat/src/main/java/org/apache/coyote/http11/view/ResponseEntity.java

사용하지 않는 파일 같네용👀

죄송합니다..ㅠ 꼼꼼하게 체크하지 못했네요

aak2075 · 2023-09-10T12:42:53Z

tomcat/src/main/java/org/apache/coyote/http11/view/View.java

@@ -0,0 +1,8 @@
+package org.apache.coyote.http11.view;
+
+public interface View {


정적 자원의 뷰와 스트링으로 만든 뷰를 잘 추상화 하셨네요👍

aak2075 · 2023-09-10T12:47:29Z

tomcat/src/main/java/org/apache/coyote/http11/view/StaticResourceResolver.java

+
+    private static URL findResourceByViewName(final String viewName) {
+        final ClassLoader classLoader = StaticResourceResolver.class.getClassLoader();
+        return Optional.ofNullable(classLoader.getResource(RESOURCE_DIRECTORY + viewName))


~~Optional을 이렇게도 사용할 수 있군요👍~~
http://homoefficio.github.io/2019/10/03/Java-Optional-%EB%B0%94%EB%A5%B4%EA%B2%8C-%EC%93%B0%EA%B8%B0/
옵셔널은 비싸기 때문에 단순 값 비교가 목적이라면 null이 더 좋다는 이야기도 있네요

좋은 레퍼런스 감사합니다. 말씀대로 Java Optional을 의도대로 사용하고 있지 않았네요! 반환타입으로 사용되지도 않고, 단순 null 처리 용이다 보니 더 저렴하게 처리하도록 수정하겠습니다~!

aak2075 · 2023-09-10T13:47:09Z

tomcat/src/main/java/org/apache/coyote/http11/request/parser/HttpRequestMessageReader.java

@@ -33,7 +33,9 @@ private HttpRequestMessageReader() {
    }

    public static HttpRequest readHttpRequest(final InputStream inputStream) throws IOException {
-        try (final BufferedReader br = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8))) {
+        try (final InputStreamReader inputStreamReader = new InputStreamReader(inputStream, StandardCharsets.UTF_8);
+             final BufferedReader br = new BufferedReader(inputStreamReader)) {


요 부분 때문에 SoketException 이 발생하는 것 같습니다.
이 메서드가 종료되면서 br.close()가 호출되고, close() 내부에서는 inputStream을 close합니다.
그 이후에 Http11Processor 에서 다시 inputStream을 close 할 때 이미 닫혀있어서 Exception이 발생하는 것으로 보이는데요, 한번 확인해 주세요!

해당 오류 확인했습니다! BufferedReader를 close하면서 내부도 close를 다 하는 걸 확인했는데, try-resource를 통해 두번씩 닫히면서 오류가 발생하는군용.. 감사합니다!

aak2075 · 2023-09-10T13:49:39Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

        this.serverSocket = createServerSocket(port, acceptCount);
        this.stopped = false;
+        this.executor = Executors.newFixedThreadPool(maxThreadCount);


이렇게 설정하면 내부적으로 스레드가 어떻게 관리되나요?

설정한 개수(250) 만큼의 Thread pool의 크기를 가지는(corePoolSize, maxPoolSize) ExecutorService를 생성합니다.
만약 250개의로 설정하고 생성한다면, 250개의 Thread가 미리 생성되어 대기 하다가 work가 실행되면 생성된 Thread들이 해당 work를 수행합니다.
모든 thread가 활성화(active) 된 상태이면 내부적으로 설정된 LinkedBlockingQueue에 work가 적재되어 대기하다가 available(사용가능?)한 Thread가 작업을 실행하는 구조로 관리됩니다!

TaeyeonRoyce

마코, 꼼꼼한 리뷰 감사합니다! 추가적으로 던져주진 질문 덕분에 더 학습할 수 있는 기회가 되었습니다.
리뷰 반영 후 답변 남겼는데 확인해주시면 감사하겠습니다~!

TaeyeonRoyce · 2023-09-10T23:25:10Z

tomcat/src/main/java/org/apache/coyote/http11/view/StaticResourceResolver.java

+
+    private static URL findResourceByViewName(final String viewName) {
+        final ClassLoader classLoader = StaticResourceResolver.class.getClassLoader();
+        return Optional.ofNullable(classLoader.getResource(RESOURCE_DIRECTORY + viewName))


좋은 레퍼런스 감사합니다. 말씀대로 Java Optional을 의도대로 사용하고 있지 않았네요! 반환타입으로 사용되지도 않고, 단순 null 처리 용이다 보니 더 저렴하게 처리하도록 수정하겠습니다~!

TaeyeonRoyce · 2023-09-10T23:27:27Z

tomcat/src/main/java/org/apache/coyote/http11/view/ResponseEntity.java

죄송합니다..ㅠ 꼼꼼하게 체크하지 못했네요

TaeyeonRoyce · 2023-09-10T23:53:07Z

tomcat/src/main/java/org/apache/coyote/http11/request/parser/HttpRequestMessageReader.java

@@ -33,7 +33,9 @@ private HttpRequestMessageReader() {
    }

    public static HttpRequest readHttpRequest(final InputStream inputStream) throws IOException {
-        try (final BufferedReader br = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8))) {
+        try (final InputStreamReader inputStreamReader = new InputStreamReader(inputStream, StandardCharsets.UTF_8);
+             final BufferedReader br = new BufferedReader(inputStreamReader)) {


해당 오류 확인했습니다! BufferedReader를 close하면서 내부도 close를 다 하는 걸 확인했는데, try-resource를 통해 두번씩 닫히면서 오류가 발생하는군용.. 감사합니다!

TaeyeonRoyce · 2023-09-10T23:54:38Z

tomcat/src/main/java/org/apache/coyote/http11/controller/AbstractController.java

+        supportedMethods.get(httpMethod).accept(request, response);
+    }
+
+    protected void doGetRequest(final HttpRequest request, final HttpResponse response) {


명시적이라 생각해서 붙혀두었는데 doGet도 충분한것 같네요!
반영해두었습니다

TaeyeonRoyce · 2023-09-11T02:14:40Z

tomcat/src/main/java/org/apache/coyote/http11/common/Session.java


 public class Session {

    private final String id;
-    private final Map<String, Object> values = new HashMap<>();
+    private final Map<String, Object> values = new ConcurrentHashMap<>();


SessionManager에서 ConcurrentHashMap을 쓰다보니 get한 object(Session)에 대해서는 동시성이 보장되지 않을 것 같아서 둘 다 사용했습니다.
SessionManager에는 동시에 Session을 생성하는 경우는 존재하는데, 말씀하신대로 생성된 session에 대한 동시 수정에 대한 경우는 없긴 합니다...!
현재 상태에서 불필요한 동시성 관리보다 성능 이점이 있는 HashMap을 써도 좋아보이네요! 의견 감사합니다~!

TaeyeonRoyce · 2023-09-11T02:28:52Z

study/src/main/java/cache/com/example/etag/EtagFilterConfiguration.java

+    @Bean
+    public FilterRegistrationBean<ShallowEtagHeaderFilter> shallowEtagHeaderFilter() {
+        FilterRegistrationBean<ShallowEtagHeaderFilter> filterRegistrationBean =
+                new FilterRegistrationBean<>(new ShallowEtagHeaderFilter());


ETag를 생성한 뒤 응답에 포함시켜 반환합니다!

TaeyeonRoyce · 2023-09-11T02:39:09Z

study/src/main/java/cache/com/example/cachecontrol/CacheControlInterceptor.java

+    ) {
+        final String cacheControl = CacheControl
+                .noCache()
+                .cachePrivate()


모두 응답 헤더 Cache-control에 설정하여 캐시에 대한 정책에 대해 명시하는 것 입니다.
no-cache는 캐시를 저장은 해도 되지만, 매 요청마다 revalidate(재검증..?)가 필요하다는 정책이고 no-store는 캐시를 아예 저장하지 말라는 뜻입니다.

private과 public의 차이점은 캐싱의 범위입니다. CDN이나 다른 proxy 서버에 캐싱을 저장할 수 있는 public과 달리, private은 클라이언트에서만 캐싱할 수 있습니다.

이미지나 로고 같이 모든 사용자에게 보여지는 민감하지 않은 정보는 public, 사용자와 관련된 데이터는 private으로 설정하는 식으로 목적에 맞게 관리 할 수 있을 것 같습니다!

TaeyeonRoyce · 2023-09-11T02:52:53Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

        this.serverSocket = createServerSocket(port, acceptCount);
        this.stopped = false;
+        this.executor = Executors.newFixedThreadPool(maxThreadCount);


설정한 개수(250) 만큼의 Thread pool의 크기를 가지는(corePoolSize, maxPoolSize) ExecutorService를 생성합니다.
만약 250개의로 설정하고 생성한다면, 250개의 Thread가 미리 생성되어 대기 하다가 work가 실행되면 생성된 Thread들이 해당 work를 수행합니다.
모든 thread가 활성화(active) 된 상태이면 내부적으로 설정된 LinkedBlockingQueue에 work가 적재되어 대기하다가 available(사용가능?)한 Thread가 작업을 실행하는 구조로 관리됩니다!

sonarcloud · 2023-09-11T02:54:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

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

aak2075 · 2023-09-11T16:19:56Z

안녕하세요 로이스!

코드를 보면서, 피드백을 남기면서 저도 많이 배우게 됐습니다.

이번 미션 고생하셨고 다음 미션도 화이팅입니다~😄

TaeyeonRoyce added 8 commits September 6, 2023 22:58

test: cache 학습테스트 추가

75b2629

test: thread 학습테스트 추가

996e112

refactor: controller 메서드 이름 변경

a75db6e

refactor: ResponseEntity 제거

96d3b70

refactor: Resource 찾는 역할 분리

15c2edc

feat: Executors로 동시성 확장

4ac9370

feat: Session에 동시성 컬렉션 적용

59526f4

refactor: 패키지 이동

022251f

TaeyeonRoyce requested a review from aak2075 September 9, 2023 09:38

TaeyeonRoyce self-assigned this Sep 9, 2023

refactor: Static 클래스에 private 생성자 추가

bcbe17d

aak2075 requested changes Sep 10, 2023

View reviewed changes

TaeyeonRoyce added 5 commits September 11, 2023 08:21

refactor: 사용 되지 않는 ResponseEntity 제거

0370d81

refactor: 불필요한 Optional 제거

d13d207

fix: stream이 두번 닫히는 오류 수정

88e9075

refactor: servlet 메서드 이름 수정

3659b63

refactor: 동시성 컬렉션 수정

e182bb2

TaeyeonRoyce commented Sep 11, 2023

View reviewed changes

aak2075 merged commit fd6420b into woowacourse:taeyeonroyce Sep 11, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 로이스(원태연) 미션 제출합니다. #429

[톰캣 구현하기 - 3, 4단계] 로이스(원태연) 미션 제출합니다. #429

TaeyeonRoyce commented Sep 9, 2023

aak2075 left a comment

aak2075 Sep 10, 2023

TaeyeonRoyce Sep 11, 2023

aak2075 Sep 10, 2023

TaeyeonRoyce Sep 11, 2023

aak2075 Sep 10, 2023

aak2075 Sep 10, 2023

TaeyeonRoyce Sep 10, 2023

aak2075 Sep 10, 2023

aak2075 Sep 10, 2023

TaeyeonRoyce Sep 10, 2023

aak2075 Sep 10, 2023

aak2075 Sep 10, 2023 •

edited

Loading

TaeyeonRoyce Sep 10, 2023

aak2075 Sep 10, 2023

TaeyeonRoyce Sep 10, 2023

aak2075 Sep 10, 2023

TaeyeonRoyce Sep 11, 2023

TaeyeonRoyce left a comment

TaeyeonRoyce Sep 10, 2023

TaeyeonRoyce Sep 10, 2023

TaeyeonRoyce Sep 10, 2023

TaeyeonRoyce Sep 10, 2023

TaeyeonRoyce Sep 11, 2023

TaeyeonRoyce Sep 11, 2023

TaeyeonRoyce Sep 11, 2023

TaeyeonRoyce Sep 11, 2023

sonarcloud bot commented Sep 11, 2023

aak2075 commented Sep 11, 2023

		@@ -0,0 +1,8 @@
		package org.apache.coyote.http11.view;

		public interface View {

[톰캣 구현하기 - 3, 4단계] 로이스(원태연) 미션 제출합니다. #429

[톰캣 구현하기 - 3, 4단계] 로이스(원태연) 미션 제출합니다. #429

Conversation

TaeyeonRoyce commented Sep 9, 2023

aak2075 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aak2075 Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TaeyeonRoyce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 11, 2023

aak2075 commented Sep 11, 2023

aak2075 Sep 10, 2023 •

edited

Loading