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.

Sign up for GitHub

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

Jump to bottom

[톰캣 구현하기 - 3, 4단계] 땡칠(박성철) 미션 제출합니다. #471

Merged
merged 16 commits into from
Sep 14, 2023
Merged

[톰캣 구현하기 - 3, 4단계] 땡칠(박성철) 미션 제출합니다. #471

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a4b2bd7
refactor(Http11Processor): HTTP Version 관리를 Response로 위임
0chil Sep 7, 2023
1f6ab90
refactor(HttpRequest): Request Line을 객체로 분리
0chil Sep 7, 2023
fa959af
feat: GET, POST를 담당하는 핸들러
0chil Sep 7, 2023
04ee075
feat(Connector): Thread Pool 적용
0chil Sep 11, 2023
7dff12a
feat(SessionManager): 발생할 수 있는 동시성 문제를 해결한다
0chil Sep 11, 2023
9d3d82c
refactor: Handler 인터페이스를 request, response 로 수정한다
0chil Sep 11, 2023
a550e22
refactor: 쿠키, 세션 조회에 Optional 사용
0chil Sep 12, 2023
16cc50b
refactor(Headers): 메서드 변수명 수정
0chil Sep 12, 2023
ef2ac0d
refactor(HttpResponse): 바디 길이 0일때, Content-Length 헤더 제외
0chil Sep 12, 2023
93015f9
refactor: HttpResponse가 응답을 생성한다
0chil Sep 12, 2023
5fdd9e8
refactor: Handler를 Controller로 재명명
0chil Sep 12, 2023
5092d78
refactor: 컨트롤러 매핑 RequestMapping으로 분리, TargetPath 객체 분리
0chil Sep 12, 2023
1c46ac7
fix: 실패하는 테스트 보완
0chil Sep 13, 2023
5372bd0
refactor: RequestMapping 상수화
0chil Sep 14, 2023
4e85edc
refactor: 핸들러 목록의 자료형을 Set으로 변경
0chil Sep 14, 2023
d8f979d
typo(HttpResponse): 변수명 headers2 -> headers
0chil Sep 14, 2023
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
74 changes: 40 additions & 34 deletions tomcat/src/main/java/nextstep/jwp/handler/LoginHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,69 +1,75 @@
package nextstep.jwp.handler;

import java.util.Objects;
import java.util.Optional;
import nextstep.jwp.db.InMemoryUserRepository;
import nextstep.jwp.model.User;
import org.apache.coyote.http11.HttpRequest;
import org.apache.coyote.http11.HttpResponse;
import org.apache.coyote.http11.body.FormData;
import org.apache.coyote.http11.handler.FileHandler;
import org.apache.coyote.http11.handler.Handler;
import org.apache.coyote.http11.handler.GetAndPostHandler;
import org.apache.coyote.http11.header.Cookies;
import org.apache.coyote.http11.session.Session;
import org.apache.coyote.http11.session.SessionManager;

public class LoginHandler implements Handler {
public class LoginHandler extends GetAndPostHandler {

private static final String SESSION_KEY = "JSESSIONID";
private static final String UNAUTHORIZED_LOCATION = "/401";
private static final String MAIN_LOCATION = "/index";
private static final String SESSION_USER_KEY = "user";

private final FileHandler fileHandler = new FileHandler();

@Override
public HttpResponse handle(final HttpRequest httpRequest) {
if (httpRequest.getMethod().isPost()) {
FormData formData = FormData.of(httpRequest.getBody());
String account = formData.get("account");
String password = formData.get("password");
protected void doGet(final HttpRequest httpRequest, final HttpResponse httpResponse) {
Session session = getSessionFrom(httpRequest.getCookies());
if (session != null && hasUserOn(session)) {
httpResponse.redirectTo(MAIN_LOCATION);
}
fileHandler.handle(httpRequest, httpResponse);
}

Optional<User> found = InMemoryUserRepository.findByAccount(account);
if (found.isEmpty()) {
return HttpResponse.redirectTo(UNAUTHORIZED_LOCATION);
}
User user = found.get();
return signIn(httpRequest, user, password);
private Session getSessionFrom(final Cookies cookies) {
String sessionId = cookies.get(SESSION_KEY);
if (sessionId == null) {
return null;
}
if (httpRequest.getMethod().isGet()) {
String jsessionId = httpRequest.getCookies().get(SESSION_KEY);
if (jsessionId == null) {
return fileHandler.handle(httpRequest);
}
Session session = SessionManager.findSession(jsessionId);
if (session != null) {
return HttpResponse.redirectTo(MAIN_LOCATION);
}
return SessionManager.findSession(sessionId);
}

private boolean hasUserOn(Session session) {
return Objects.nonNull(session.getAttribute(SESSION_USER_KEY));
}

@Override
protected void doPost(final HttpRequest httpRequest, final HttpResponse httpResponse) {
FormData formData = FormData.of(httpRequest.getBody());
String account = formData.get("account");
String password = formData.get("password");

Optional<User> found = InMemoryUserRepository.findByAccount(account);
if (found.isPresent()) {
User user = found.get();
signIn(httpResponse, user, password);
return;
}
return fileHandler.handle(httpRequest);
httpResponse.redirectTo(UNAUTHORIZED_LOCATION);
}

private HttpResponse signIn(final HttpRequest httpRequest, final User user, final String password) {
private void signIn(final HttpResponse httpResponse, final User user, final String password) {
if (user.checkPassword(password)) {
Session session = new Session();
session.addAttribute("user", user);
session.addAttribute(SESSION_USER_KEY, user);
SessionManager.add(session);

Cookies cookies = httpRequest.getCookies();
Cookies cookies = new Cookies();
cookies.add(SESSION_KEY, session.getId());

return redirectToMainWith(cookies);
httpResponse.with(cookies).redirectTo(MAIN_LOCATION);
return;
}
return HttpResponse.redirectTo(UNAUTHORIZED_LOCATION);
}

private HttpResponse redirectToMainWith(Cookies cookies) {
HttpResponse httpResponse = HttpResponse.redirectTo(MAIN_LOCATION);
httpResponse.putHeader(cookies.toHeader("Set-Cookie"));
return httpResponse;
httpResponse.redirectTo(UNAUTHORIZED_LOCATION);
}
}
27 changes: 15 additions & 12 deletions tomcat/src/main/java/nextstep/jwp/handler/RegisterHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,27 @@
import org.apache.coyote.http11.HttpResponse;
import org.apache.coyote.http11.body.FormData;
import org.apache.coyote.http11.handler.FileHandler;
import org.apache.coyote.http11.handler.Handler;
import org.apache.coyote.http11.handler.GetAndPostHandler;

public class RegisterHandler implements Handler {
public class RegisterHandler extends GetAndPostHandler {

private static final String MAIN_LOCATION = "/index";
private final FileHandler fileHandler = new FileHandler();

@Override
public HttpResponse handle(final HttpRequest httpRequest) {
if (httpRequest.getMethod().isPost()) {
FormData formData = FormData.of(httpRequest.getBody());
protected void doGet(final HttpRequest httpRequest, final HttpResponse httpResponse) {
fileHandler.handle(httpRequest, httpResponse);
}

@Override
protected void doPost(final HttpRequest httpRequest, final HttpResponse httpResponse) {
FormData formData = FormData.of(httpRequest.getBody());

String account = formData.get("account");
String password = formData.get("password");
String email = formData.get("email");
String account = formData.get("account");
String password = formData.get("password");
String email = formData.get("email");

InMemoryUserRepository.save(new User(account, password, email));
return HttpResponse.redirectTo("/index");
}
return fileHandler.handle(httpRequest);
InMemoryUserRepository.save(new User(account, password, email));
httpResponse.redirectTo(MAIN_LOCATION);
}
}
26 changes: 17 additions & 9 deletions tomcat/src/main/java/org/apache/catalina/connector/Connector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,38 @@
package org.apache.catalina.connector;

import java.net.SocketException;
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.net.SocketException;
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_ACCEPT_COUNT = 100; // 모든 쓰레드가 사용중일 때 들어오는 연결에 대한 대기열의 길이
private static final int MAX_THREADS = 250; // 동시에 연결 후 처리 가능한 요청의 최대 개수
// 최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?
// acceptCount를 100으로 한다
// 처리중인 연결이 250개 이므로 나머지는 OS 큐에서 대기하게 된다.
private static final int SOCKET_TIMEOUT_SECONDS = 10;
Comment on lines +19 to 24
Copy link
Author

Choose a reason for hiding this comment

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

정확한지는 잘 모르겠는데 제가 이해한대로 우선 적어보았습니다.
히이로는 최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?
라는 요구사항을 어떻게 반영해야한다고 생각하시나요?

Copy link

Choose a reason for hiding this comment

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

네 저도 정확하게 땡칠과 같은 방식으로 이해하고 있습니다. 모든 쓰레드가 사용중일 떄 100개의 connection 요청들을 queueing하기 위해서 acceptCount로 ServerSocket을 생성할 때 OS에 저장되는 connection들에 대한 queue size를 100으로 설정해줍니다.

ThreadPool 생성 시 corePoolSize와 maximumPoolSize는 250으로 동일하게 설정했으니 시작부터 250개의 쓰레드가 생성되어 동작하면서 요청들을 처리하게 될 것 같습니다.

혹시 어떤 부분이 궁금하셨던건지 더 자세하게 따로 말씀해주시면 같이 고민해봐도 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

답변해주신 내용이 바로 궁금한 부분이었습니다.
Accept Count가 OS가 queueing 할 크기(백로그 크기)를 결정하는 부분이라고 이해했어요!


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

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

public Connector(final int port, final int acceptCount) {
public Connector(final int port, final int acceptCount, final int maxThreads) {
this.pool = Executors.newFixedThreadPool(maxThreads);
this.serverSocket = createServerSocket(port, acceptCount);
this.stopped = false;
}
Expand Down Expand Up @@ -60,6 +67,7 @@ private void connect() {
try {
process(serverSocket.accept());
} catch (IOException e) {
pool.shutdown();
log.error(e.getMessage(), e);
}
}
Expand All @@ -70,7 +78,7 @@ private void process(final Socket connection) throws SocketException {
}
connection.setSoTimeout(SOCKET_TIMEOUT_SECONDS * 1000);
var processor = new Http11Processor(connection);
new Thread(processor).start();
pool.execute(processor);
}

public void stop() {
Expand Down
17 changes: 10 additions & 7 deletions tomcat/src/main/java/org/apache/coyote/http11/Http11Processor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,19 @@ public class Http11Processor implements Runnable, Processor {

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

private static final String HTTP_VERSION = "HTTP/1.1 ";
private static final String CRLF = "\r\n";

private static final Handler DEFAULT_HANDLER = new FileHandler();
private static final LoginHandler LOGIN_HANDLER = new LoginHandler();
private static final RegisterHandler REGISTER_HANDLER = new RegisterHandler();
private static final Map<String, Handler> PREDEFINED_HANDLERS = Map.of(
"/", httpRequest -> new HttpResponse("Hello world!", "text/html"),
"/", (request, response) -> response.setBody("Hello world!"),
"/login", LOGIN_HANDLER,
"/login.html", LOGIN_HANDLER,
"/register", REGISTER_HANDLER,
"/register.html", REGISTER_HANDLER
);
private static final String WHITE_SPACE = " ";

private final Socket connection;

Expand All @@ -52,13 +52,14 @@ public void process(final Socket connection) {
final var outputStream = connection.getOutputStream()) {

HttpRequest httpRequest = HttpRequest.from(bufferedReader);
HttpResponse httpResponse = handle(httpRequest);
HttpResponse httpResponse = new HttpResponse();
handle(httpRequest, httpResponse);
Copy link

Choose a reason for hiding this comment

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

이 handle 로직을 hanlerMapper 객체로 분리해내주셨으면 더 책임분리가 깔끔하게 되지 않았을까 싶어요! 땡칠도 잘 아실거라 생각하지만 이 handlerMapper 객체가 서블릿 컨테이너와 책임이 동일한 객체인지라 coyote 패키지의 Http11Processor와는 분리가 되는 것이 맞다는 생각입니다.

이 생각의 흐름으로 보면 땡칠께서 남겨주신 의문점이 해결되지 않을까 싶습니다. "response가 어디서든 수정 가능하다는 장점"이 왜 필요한 것일까요? 저는 Http11Processor에서 처음 생성된 request, response가 handlerMapper(=servletContainer)에서 mapping된 handler(servlet)을 통해 비즈니스 로직을 수행하고 해당 결과가 response에 담겨 client에게 다시 전달되어야 하기 때문이라고 생각합니다.

또한 response에 담기는 헤더 값들이 어떤 한 지점에서 바로 완성되는 것이 아니라 계속 여러 단계를 거치면서 추가되는 것이기 때문에 response 객체가 전달되어야 한다고도 생각할 수 있을 것 같습니다.

이 생각들은 저도 개인적으로 학습하고 갖고있는 생각이라 충분히 틀릴 수 있으니 땡칠께서 보시고 이상하다 싶은 부분은 편하게 말씀해주시면 감사하겠습니다!

Copy link
Author

Choose a reason for hiding this comment

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

응답이 동적으로 생성되기까지 수많은 과정 속에서 언제든 응답코드, 헤더, 바디를 수정하기 위함이라는 말씀이시네요!
듣고 단번에 이해가 되었습니다.

이 말을 듣고보니까
함수 반환으로 Response를 사용하는 제 기존 디자인에서는
handler 호출 스택의 끝에서 response를 만들도록 제한되는 단점이 있네요.

예를 들어 서비스 객체가 있다면, 호출 스택의 끝인 서비스 객체에서 response를 만들어야할 것 같아요.
이런 디자인은 상황에 따라 부적절한 곳에서 응답을 생성하게 될 수도 있겠네요.

결론은 '어디서든 응답 수정을 쉽게 하려는 의도가 담긴 디자인이다'. 네요
좋은 커멘트 감사합니다~!!


String headerLines = httpResponse.getHeaders().stream()
.map(HttpHeader::toLine)
.collect(Collectors.joining(CRLF));
final var response = String.join(CRLF,
HTTP_VERSION + httpResponse.getStatus().toLine(),
HttpResponse.HTTP_VERSION + WHITE_SPACE + httpResponse.getStatus().toLine(),
headerLines,
"",
httpResponse.getBody()
Copy link

Choose a reason for hiding this comment

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

client에게 내려줄 최종 response 형태를 만드는 책임도 HttpResponse 객체에 부여해보시는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

오 좋은 생각입니다 역시 👍
반영해보겠습니다!

Expand All @@ -71,10 +72,12 @@ public void process(final Socket connection) {
}
}

private HttpResponse handle(final HttpRequest httpRequest) {
private void handle(final HttpRequest httpRequest, final HttpResponse httpResponse) {
if (PREDEFINED_HANDLERS.containsKey(httpRequest.getTarget().getPath())) {
return PREDEFINED_HANDLERS.get(httpRequest.getTarget().getPath()).handle(httpRequest);
PREDEFINED_HANDLERS.get(httpRequest.getTarget().getPath())
.handle(httpRequest, httpResponse);
return;
}
return DEFAULT_HANDLER.handle(httpRequest);
DEFAULT_HANDLER.handle(httpRequest, httpResponse);
}
}
27 changes: 8 additions & 19 deletions tomcat/src/main/java/org/apache/coyote/http11/HttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,28 +9,19 @@

public class HttpRequest {

private static final String WHITE_SPACE = " ";

private final HttpMethod method;
private final HttpTarget target;
private final String version;
private final RequestLine requestLine;
private final Headers headers;
private final String body;

private HttpRequest(final String method, final String target, final String version,
final Headers headers,
final String body) {
this.method = new HttpMethod(method);
this.target = new HttpTarget(target);
this.version = version;
public HttpRequest(final RequestLine requestLine, final Headers headers, final String body) {
this.requestLine = requestLine;
this.headers = headers;
this.body = body;
}

public static HttpRequest from(BufferedReader reader) {
try {
String[] requestLineElements = reader.readLine().split(WHITE_SPACE);

RequestLine requestLine = new RequestLine(reader.readLine());
Headers headers = new Headers();
String next;
while (reader.ready() && !(next = reader.readLine()).isEmpty()) {
Expand All @@ -45,9 +36,7 @@ public static HttpRequest from(BufferedReader reader) {
}
Copy link

Choose a reason for hiding this comment

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

headers에서 contentLength만 따로 get 로직을 작성해주셨네요! headers 내부에 EMPTY 객체를 잘 만들어두셔서 해당 객체를 활용한 일관적인 처리방식으로 수행되지 못하는 것 같아 약간 아쉬운 부분 같습니다.

여기 headers에서 contentLength 값을 가져와서 로직 처리하는 부분은 저번과 같은데 제가 그때는 미처 생각하지 못했나봅니다...😭 반드시 수정해야 할 부분은 아니지만 여유가 되신다면 수정해보시는 것도 좋을 것 같아서 리뷰드립니다!


Copy link

Choose a reason for hiding this comment

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

Suggested change
HttpHeader header = headers.get("Content-Length");
int contentLength = 0;
if (!header.isEmpty()) {
contentLength = Integer.parseInt(header.getValue());
}
char[] bodyBuffer = new char[contentLength];
while (contentLength > 0) {
int readCount = reader.read(bodyBuffer, 0, contentLength);
contentLength -= readCount;
}

요런 느낌은 어떠신지 조심히 제안드려봅니당 😁

Copy link
Author

Choose a reason for hiding this comment

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

위 커멘트와 이어지는 내용이네요.

EMPTY 객체를 만들어두고 잘 활용하지 못하는 부분이 아쉬우셨군요. 일관적이지 못하다는 관점 충분히 이해가 갑니다!
하지만 getContentLength()를 유지하고 싶어요.

image

이 메서드를 별도로 만든 의도는 null에 대한 대응 방법을 캡슐화하기 위해서였어요.
헤더값은 문자열이므로 Integer.parseInt(header.getValue())를 사용해주어야 하는데,
header.getValue()는 빈 문자열일수도 있기 때문에, 위 로직처럼 빈 헤더인지 체크 후에 사용해야하는 복잡함이 있습니다.

그래서 '자주 사용하는 일반적인 헤더니까 걱정없이 정수 형태의 컨텐츠 길이를 얻자' 라는 의도로 일종의 헬퍼 메서드를 만들었습니다.

사실 로직의 위치만 다를 뿐, 히이로가 제안해주신 내용과 같은 코드인 것 같아요.
그래서 당연하게도 히이로가 제안하신 코드도 당장 동작하는 상황입니다.

하지만 지금 구현한 헬퍼 메서드를 통해 null에 대한 처리도 함께 재활용할 수 있다는 장점이 있으므로 getContentLength()는 유지하고자 합니다.

Copy link

Choose a reason for hiding this comment

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

null 처리 로직의 캡슐화가 주 목적이셨군요! 이해했습니다 ㅎㅎ 👍

return new HttpRequest(
requestLineElements[0],
requestLineElements[1],
requestLineElements[2],
requestLine,
headers,
new String(bodyBuffer)
);
Expand All @@ -58,15 +47,15 @@ public static HttpRequest from(BufferedReader reader) {


public HttpMethod getMethod() {
return method;
return requestLine.getMethod();
}

public HttpTarget getTarget() {
return target;
return requestLine.getTarget();
}

public String getVersion() {
return version;
return requestLine.getHttpVersion();
}

public List<HttpHeader> getHeaders() {
Expand Down
Loading