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단계] 아코(안석환) 미션 제출합니다. #466

Merged
merged 12 commits into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,11 @@
- [x] 로그인시 쿠키 생성해주기
- [x] Session 구현하기
- [x] 로그인시 세션저장하기
## 3단계
- [x] 전반적인 코드 리팩토링 하기
- [x] HttpRequest 및 HttpResponse 도입하기
- [x] controller 도입하기
- [x] 파일 읽어오는 기능 util 클래스로 분리
## 4단계
- [ ] Executors로 Thread Pool 적용
- [ ] 동시성 컬렉션 사용하기
18 changes: 11 additions & 7 deletions tomcat/src/main/java/org/apache/catalina/connector/Connector.java

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.

저도 계속 찾아보았는데 방식이 떠오르지 않아서 이 부분 추가하지 못했습니다 ㅜㅜ

Original file line number Diff line number Diff line change
@@ -1,31 +1,35 @@
package org.apache.catalina.connector;

import org.apache.coyote.http11.Http11Processor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import org.apache.coyote.http11.Http11Processor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class Connector implements Runnable {

private static final Logger log = LoggerFactory.getLogger(Connector.class);

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

private final ServerSocket serverSocket;
private boolean stopped;
private final ExecutorService executorService;

public Connector() {
this(DEFAULT_PORT, DEFAULT_ACCEPT_COUNT);
this(DEFAULT_PORT, DEFAULT_ACCEPT_COUNT, DEFAULT_THREAD_COUNT);
}

public Connector(final int port, final int acceptCount) {
public Connector(final int port, final int acceptCount, final int maxThreads) {
this.serverSocket = createServerSocket(port, acceptCount);
this.stopped = false;
this.executorService = Executors.newFixedThreadPool(maxThreads);
}

private ServerSocket createServerSocket(final int port, final int acceptCount) {
Expand Down Expand Up @@ -67,7 +71,7 @@ private void process(final Socket connection) {
return;
}
var processor = new Http11Processor(connection);
new Thread(processor).start();
executorService.execute(processor);
}

public void stop() {
Expand Down
162 changes: 0 additions & 162 deletions tomcat/src/main/java/org/apache/coyote/http11/Handler.java

This file was deleted.

41 changes: 0 additions & 41 deletions tomcat/src/main/java/org/apache/coyote/http11/HandlerMapping.java

This file was deleted.

19 changes: 5 additions & 14 deletions tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.Socket;
import nextstep.jwp.exception.UncheckedServletException;
import org.apache.coyote.Processor;
import org.apache.coyote.http11.controller.Controller;
import org.apache.coyote.http11.httpmessage.request.HttpRequest;
import org.apache.coyote.http11.httpmessage.response.HttpResponse;
import org.slf4j.Logger;
Expand Down Expand Up @@ -35,23 +35,14 @@ public void process(final Socket connection) {
final var reader = new BufferedReader(new InputStreamReader(inputStream))
) {
final String request = readRequest(reader);

final HttpRequest httpRequest = HttpRequest.from(request);
final HttpResponse httpResponse = new HttpResponse();
final Controller controller = RequestMapping.getController(httpRequest.getPath());

final HandlerMapping handlerMapping = HandlerMapping.find(
httpRequest.getPath(),
httpRequest.getHttpMethod());

final Handler handler = new Handler(
handlerMapping,
httpRequest.getQueryString(),
httpRequest.getRequestBody(),
httpRequest.getCookie());
final HttpResponse httpResponse = handler.makeResponse();

controller.service(httpRequest, httpResponse);
outputStream.write(httpResponse.makeToString().getBytes());
outputStream.flush();
} catch (IOException | UncheckedServletException e) {
} catch (Exception e) {
log.error(e.getMessage(), e);
}
}
Expand Down
28 changes: 28 additions & 0 deletions tomcat/src/main/java/org/apache/coyote/http11/RequestMapping.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package org.apache.coyote.http11;

import java.util.HashMap;
import java.util.Map;
import org.apache.coyote.http11.controller.Controller;
import org.apache.coyote.http11.controller.LoginController;
import org.apache.coyote.http11.controller.PageController;
import org.apache.coyote.http11.controller.RegisterController;
import org.apache.coyote.http11.controller.RootController;

public class RequestMapping {

private static final Map<String, Controller> REQUEST_MAPPER = new HashMap<>();

Comment on lines +13 to +14

Choose a reason for hiding this comment

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

Suggested change
private static final Map<String, Controller> REQUEST_MAPPER = new HashMap<>();
private static final Map<String, Controller> REQUEST_MAPPER = new HashMap<>();
private RequestMapping() {
}

인스턴스를 만들지 않는 유틸 클래스에는 private 기본 생성자를 추가해주세요..!

Utility classes, which are collections of static members, are not meant to be instantiated. Even abstract utility classes, which can be extended, should not have public constructors.

Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.

Copy link
Author

Choose a reason for hiding this comment

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

빼먹고 추가를 못했었네요 감사합니다!

static {
REQUEST_MAPPER.put("/", new RootController());
REQUEST_MAPPER.put("/login", new LoginController());
REQUEST_MAPPER.put("/register", new RegisterController());
}

public static Controller getController(final String path) {
if (REQUEST_MAPPER.containsKey(path)) {
return REQUEST_MAPPER.get(path);
}
return new PageController();

Choose a reason for hiding this comment

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

Suggested change
if (REQUEST_MAPPER.containsKey(path)) {
return REQUEST_MAPPER.get(path);
}
return new PageController();
return REQUEST_MAPPER.getOrDefault(path, new PageController());

이 부분은 취향 차이라고 생각하는데
getOrDefault를 사용해서 라인을 줄일 수 있어요!

Copy link
Author

Choose a reason for hiding this comment

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

엄청 깔끔하네요! 배워갑니다 저도 if문이 없는 부분이 더 가독성이 좋은 것 같습니다👍👍

}

}
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package org.apache.coyote.http11;

import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

public class SessionManger {
public class SessionManager {

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

Choose a reason for hiding this comment

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

Suggested change
private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();
private static final Map<String, Session> sessions = new ConcurrentHashMap<>();

이건 저도 몰랐는데 인스턴스는 상수가 아니라고 하네요..!

https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names

Copy link
Author

Choose a reason for hiding this comment

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

아 그렇네요 저도 이번에 처음 알았습니다 꼼꼼한 리뷰 감사합니다!


private SessionManger() {
private SessionManager() {

}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package org.apache.coyote.http11.controller;

import org.apache.coyote.http11.httpmessage.request.HttpMethod;
import org.apache.coyote.http11.httpmessage.request.HttpRequest;
import org.apache.coyote.http11.httpmessage.response.HttpResponse;

public abstract class AbstractController implements Controller {

@Override
public void service(final HttpRequest request, final HttpResponse response) throws Exception {
if (request.getHttpMethod() == HttpMethod.POST) {
doPost(request, response);
}
if (request.getHttpMethod() == HttpMethod.GET) {
doGet(request, response);
}

Choose a reason for hiding this comment

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

POST나 GET이 아닌 HttpMethod가 요청으로 들어오면 어떻게 될까요??

Copy link
Author

Choose a reason for hiding this comment

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

현재로서는 아무런 동작을 하지 않네요.. POST와 GET외에는 지원하지 메소드라는 예외처리를 진행하겠습니다.

}

protected void doPost(HttpRequest request, HttpResponse response) throws Exception {
}

protected void doGet(HttpRequest request, HttpResponse response) throws Exception {
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package org.apache.coyote.http11.controller;

import org.apache.coyote.http11.httpmessage.request.HttpRequest;
import org.apache.coyote.http11.httpmessage.response.HttpResponse;

public interface Controller {

void service(final HttpRequest request, final HttpResponse response) throws Exception;
}
Loading