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단계] 땡칠(박성철) 미션 제출합니다. #471

Merged
merged 16 commits into from
Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from 5 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
70 changes: 37 additions & 33 deletions tomcat/src/main/java/nextstep/jwp/handler/LoginHandler.java
Original file line number Diff line number Diff line change
@@ -1,69 +1,73 @@
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 HttpResponse doGet(final HttpRequest httpRequest) {
Session session = getSessionFrom(httpRequest.getCookies());
if (session != null && hasUserOn(session)) {
return HttpResponse.redirectTo(MAIN_LOCATION);
}
return fileHandler.handle(httpRequest);
}

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 HttpResponse doPost(final HttpRequest httpRequest) {
FormData formData = FormData.of(httpRequest.getBody());
String account = formData.get("account");
String password = formData.get("password");

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

private HttpResponse signIn(final HttpRequest httpRequest, final User user, final String password) {
private HttpResponse signIn(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);
return HttpResponse.redirectTo(MAIN_LOCATION).with(cookies);
}
return HttpResponse.redirectTo(UNAUTHORIZED_LOCATION);
}

private HttpResponse redirectToMainWith(Cookies cookies) {
HttpResponse httpResponse = HttpResponse.redirectTo(MAIN_LOCATION);
httpResponse.putHeader(cookies.toHeader("Set-Cookie"));
return httpResponse;
}
}
27 changes: 15 additions & 12 deletions tomcat/src/main/java/nextstep/jwp/handler/RegisterHandler.java
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 HttpResponse doGet(final HttpRequest httpRequest) {
return fileHandler.handle(httpRequest);
}

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

InMemoryUserRepository.save(new User(account, password, email));
return HttpResponse.redirectTo("/index");
}
return fileHandler.handle(httpRequest);
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(MAIN_LOCATION);
}
}
26 changes: 17 additions & 9 deletions tomcat/src/main/java/org/apache/catalina/connector/Connector.java
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명까지 대기 상태로 만들려면 어떻게 할까?
라는 요구사항을 어떻게 반영해야한다고 생각하시나요?

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
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ 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();
Expand Down Expand Up @@ -58,7 +57,7 @@ public void process(final Socket connection) {
.map(HttpHeader::toLine)
.collect(Collectors.joining(CRLF));
final var response = String.join(CRLF,
HTTP_VERSION + httpResponse.getStatus().toLine(),
HttpResponse.HTTP_VERSION + httpResponse.getStatus().toLine(),
headerLines,
"",
httpResponse.getBody()
Expand Down
27 changes: 8 additions & 19 deletions tomcat/src/main/java/org/apache/coyote/http11/HttpRequest.java
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) {
}

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


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()는 유지하고자 합니다.

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
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
import java.util.List;
import java.util.Map;
import org.apache.coyote.http11.header.ContentType;
import org.apache.coyote.http11.header.Cookies;
import org.apache.coyote.http11.header.HttpHeader;

public class HttpResponse {

public static final String HTTP_VERSION = "HTTP/1.1";
private static final String EMPTY_BODY = "";

private final HttpStatus status;
Expand Down Expand Up @@ -58,4 +60,9 @@ public void putHeader(HttpHeader header) {
public List<HttpHeader> getHeaders() {
return new ArrayList<>(headers.values());
}

public HttpResponse with(Cookies cookies) {
putHeader(cookies.toHeader("Set-Cookie"));
return this;
}
}
37 changes: 37 additions & 0 deletions tomcat/src/main/java/org/apache/coyote/http11/RequestLine.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package org.apache.coyote.http11;

public class RequestLine {

private static final String SP = " ";
private static final int STANDARD_ELEMENT_COUNT = 3;

private final HttpMethod method;
private final HttpTarget target;
private final String httpVersion;

public RequestLine(String rawRequestLine) {
String[] elements = rawRequestLine.split(SP);
validateLengthOf(elements);
this.method = new HttpMethod(elements[0]);
this.target = new HttpTarget(elements[1]);
this.httpVersion = elements[2];
}

private void validateLengthOf(String[] elements) {
if (elements.length != STANDARD_ELEMENT_COUNT) {
throw new IllegalArgumentException("RequestLine의 요소 개수는 3개여야 합니다");
}
}

public HttpMethod getMethod() {
return method;
}

public HttpTarget getTarget() {
return target;
}

public String getHttpVersion() {
return httpVersion;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.apache.coyote.http11.handler;

import org.apache.coyote.http11.HttpRequest;
import org.apache.coyote.http11.HttpResponse;

public abstract class GetAndPostHandler implements Handler {

@Override
public HttpResponse handle(final HttpRequest httpRequest) {
if (httpRequest.getMethod().isGet()) {
return doGet(httpRequest);
}
return doPost(httpRequest);
}

protected abstract HttpResponse doGet(final HttpRequest httpRequest);

protected abstract HttpResponse doPost(final HttpRequest httpRequest);
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.apache.coyote.http11.header;

import java.util.Arrays;
import java.util.Collections;
import java.util.Map;
import java.util.stream.Collectors;

Expand All @@ -11,6 +12,10 @@ public class Cookies {

private final Map<String, String> cookies;

public Cookies() {
this(Collections.emptyMap());
}

private Cookies(Map<String, String> cookies) {
this.cookies = cookies;
}
Expand Down
Loading