[톰캣 구현하기 1단계] 짱수(장혁수) 미션 제출합니다. #529
Conversation
- 지역 변수, 파라미터에 `final` 제거
There was a problem hiding this comment.
안녕하세요 짱수~! 리뷰어 도도입니다
1단계 'Http 서버 구현하기' 라는 미션 목표에 충실한 좋은 코드였습니다~!
이미 클래스분리도 잘되어 있어 Http11Processor가 길이가 짧은게 인상적이네요! 읽기도 좋았습니다!
미션만 하는 것이 아닌 프로젝트와 함께하는 4레벨이다보니, 어느 정도로 리뷰를 남겨야할 지, 짱수는 이 미션에 어느정도 진심인지, 고민을 하게 되더라구요.
이번에는 짱수가 스스로 생각해보고 함께 이야기 할 수 있는 느낌으로 리뷰를 남겼습니다!
당연하게도, 리뷰는 꼭 반영해야 하는 것은 아닙니다 함께 이야기해봐요!
혹시 의도를 잘 모르겠다던가, 조금 더 의도를 알려주면 좋겠다 싶으면 바로 DM주세요~!
1단계는 바로 approve하겠습니다! 2,3,4단계는 어떻게 나눠서 낼것인지도 알려주세요!
|
|
||
| private List<String> getPlainRequest(BufferedReader requestBufferedReader) throws IOException { | ||
| List<String> plainRequest = new ArrayList<>(); | ||
| while (requestBufferedReader.ready()) { |
There was a problem hiding this comment.
ready()로 request의 끝을 판단하고 있군요.
ready()를 사용해서 EOF를 판단하면, 어떤 상황에 문제가 생길 수 있을까요?
readLine()의 특성과 비교해서 생각해보면 좋을 것 같아요!
There was a problem hiding this comment.
도저히 모르겠어서 선배 기수의 PR 들을 조금 찾아봤는데요.
ready() 메서드를 확인하는 시점에서 아직 데이터가 도착하지 않으면 ready() 의 반환값이 false 가 될 수 있다고 하네요!
#17 (comment)
LMS 에 가이드 된 것 처럼 헤더의 Content-Length 값 만큼 body 를 읽도록 변경했습니다.
There was a problem hiding this comment.
@zangsu
제가 너무 광범위하게 질문한 것 같네요! 맞습니다 ready는 데이터가 buffer에 있는 지 없는 지만 고려합니다.
ready는 블락킹을 지원하지 않아서, 데이터가 모두 도착했는 지를 고려하지 않아서, EOF를 ready로 판단하기에 위험하다고 생각한 것 입니다.
예를들면, 데이터가 오는 도중에 네트워크가 지연됐다고 가정해볼까요
ready는 아직 올 데이터가 더 남아있는 지 유무와 상관없이 false를 반환하게 되어 모든 데이터가 아직 도착하지 않았음에도, 짱수의 코드는 request를 모두 받았다고 판단한 것이죠!
|
|
||
| private String getResponse(Request request) throws IOException { | ||
| for (RequestHandler requestHandler : requestHandlers) { | ||
| String response = requestHandler.handle(request); |
There was a problem hiding this comment.
requestHandler를 순회하며 request를 넣어보는군요.
request가 어떤 타입인지에 따라, 해당 handler를 부를 수는 없던 것일까요?
짱수의 생각이 궁금합니다.
There was a problem hiding this comment.
request 의 조건을 확인하는 canHandle(Request request) 메서드가 있어도 좋았겠군요!
현재는 요구사항을 다시 확인해 보니 api 응답이 필요 없을 것 같아 하나의 Handler 만 사용하도록 변경하였습니다.
| @Override | ||
| public String handle(Request request) throws IOException { | ||
| if (request.getMethod() != Method.GET) { | ||
| return null; |
There was a problem hiding this comment.
StaticResourceHandler에게 맞지 않는 request가 와서 null을 반환하고 있네요.
Http11Processor 클래스에서는 해당 null로 다음 Handler에게 넘기는 판단을 하고 있구요.
request를 처리해달라고 요청을 했는데, 이유도 모르고 null이 오면 당황스러울 것 같아요.
There was a problem hiding this comment.
null 반환은 확실히 당황스럽겠군요!
지금은 지워진 코드이지만, 나중에 다시 등장하게 될 것 같은데요.
그 때는 더 확실하게 예외를 던져보도록 할게요
| void serializeTest() throws JsonProcessingException { | ||
| User user = InMemoryUserRepository.findByAccount("gugu").get(); | ||
|
|
||
| System.out.println(objectMapper.writeValueAsString(user)); |
There was a problem hiding this comment.
확인이 된 테스트이니 계속 테스트가 동작할 필요는 없겠네요!
하지만, 그만큼 아직은 익숙하지 않은 기능이라는 뜻이니 @Disabled 를 사용해 실행되지는 않더라도 코드 자체는 남겨둘수 있도록 수정해 볼게요!
안녕하세요 도도!
미션 리뷰를 받게 된 짱수입니다.
잘 부탁드려요~