diff --git a/contributions/65674.md b/contributions/65674.md new file mode 100644 index 00000000..881c5929 --- /dev/null +++ b/contributions/65674.md @@ -0,0 +1,232 @@ +--- +pr-url: https://github.com/nodejs/node/pull/65674 +--- + +# http: prevent reuse after incomplete request destruction + +## 문제 내용 + +HTTP server에서 `IncomingMessage`를 async iterator로 읽다가 request body를 끝까지 소비하지 않은 상태에서 iteration이 종료되면, request stream은 destroy되지만 underlying socket은 살아 있을 수 있었다. + +이 상태에서 keep-alive connection이 다음 request에 재사용되면, 아직 남아 있는 request body 데이터가 영향을 주어 이후 request가 stall될 수 있는 문제가 있었다. + +관련 Issue: #49429 + +## 원인 분석 + +문제를 재현하면서 request와 socket 상태를 확인했다. + +request는 destroy된 상태였지만 body는 완전히 소비되지 않았고, socket은 여전히 살아 있는 경우가 있었다. + +```text +req.destroyed = true +req.complete = false +req.readableEnded = false + +socket.destroyed = false +``` + +즉 다음과 같은 상태였다. + +```text +Request stream 종료 +→ Request body는 완전히 소비되지 않음 +→ Socket은 살아 있음 +→ Keep-Alive connection으로 재사용 가능 +``` + +이 때문에 불완전한 request가 남아 있는 connection이 다음 request에 다시 사용될 수 있었다. + +## 초기 해결 방향 + +처음에는 문제를 Stream destroy 경로에서 해결하려고 했다. + +request가 완전히 소비되지 않은 상태에서 destroy될 경우, 해당 socket을 다음 request에서 재사용하지 못하도록 처리하는 방향으로 수정했다. + +하지만 PR 리뷰에서 `ronag`에게 다음과 같은 피드백을 받았다. + +```text +The socket should not be re-used if destroyed without being fully consumed. +``` + +이 리뷰를 통해 단순히 Stream이 destroy되는 것보다, 불완전한 HTTP request가 끝난 뒤 connection 자체를 재사용해도 되는지가 핵심이라는 점을 다시 확인했다. + +## 해결 방향 수정 + +이후 수정 방향을 다음과 같이 정리했다. + +```text +Request body가 완전히 소비되지 않음 +→ Request destroy +→ 해당 HTTP connection은 재사용하지 않음 +``` + +이때 response header가 이미 전송되었는지 여부에 따라 두 가지 경우로 나누었다. + +### Response Header가 아직 전송되지 않은 경우 + +아직 header가 전송되지 않았다면 현재 response는 정상적으로 완료할 수 있다. + +대신 keep-alive를 비활성화한다. + +```js +response.shouldKeepAlive = false; +response._last = true; +``` + +이 경우 클라이언트에는 `Connection: close`가 전달되고, response가 끝난 뒤 socket은 재사용되지 않는다. + +### Response Header가 이미 전송된 경우 + +header가 이미 전송된 뒤에는 keep-alive 여부가 이미 결정된 상태일 수 있다. + +이 경우 response를 destroy하여 connection 자체가 재사용되지 않도록 한다. + +```js +response.destroy(); +``` + +## 리뷰를 통해 수정 위치 변경 + +이후 `mcollina`에게 다음과 같은 피드백을 받았다. + +```text +My understanding is that the fix for this needs to happen in http, not in streams. +``` + +처음에는 request가 Stream이기 때문에 Stream implementation에서 수정하려고 했다. + +하지만 실제 문제는 generic Stream 동작이 아니라 HTTP keep-alive connection의 재사용 정책과 관련된 문제였다. + +그래서 수정 위치를 Stream implementation에서 HTTP layer로 이동했다. + +최종적으로 `lib/_http_incoming.js`의 `IncomingMessage._destroy()`에서 처리하도록 변경했다. + +핵심 로직은 다음과 같다. + +```js +IncomingMessage.prototype._destroy = function _destroy(err, cb) { + if (this.socket === null) { + const response = this.client?._httpMessage; + + if (response?.req === this) { + if (response.headersSent) { + response.destroy(); + } else { + response.shouldKeepAlive = false; + response._last = true; + } + } + } + + // existing destroy handling +}; +``` + +## 테스트 및 검증 + +두 가지 상황을 각각 회귀 테스트로 추가했다. + +### Headers가 아직 전송되지 않은 경우 + +첫 번째 response가 정상적으로 끝나더라도 connection이 close되어야 한다. + +```js +assert.strictEqual(res.headers.connection, 'close'); +``` + +두 번째 request에서는 기존 socket을 재사용하지 않는지 확인했다. + +```js +assert.strictEqual(second.reusedSocket, false); +``` + +### Headers가 이미 전송된 경우 + +response 일부를 먼저 전송한 뒤 request body 처리를 중간에 종료하는 상황을 만들었다. + +```js +res.write('partial'); +``` + +이 경우 response/socket을 destroy하고, 클라이언트 측에서 `ECONNRESET`이 발생하는 것을 확인했다. + +```js +res.on('error', common.expectsError({ + code: 'ECONNRESET', + message: 'aborted', +})); +``` + +이후 두 번째 request가 새로운 socket을 사용하는 것도 확인했다. + +```js +assert.strictEqual(second.reusedSocket, false); +``` + +## 로컬 테스트 + +새로 추가한 회귀 테스트를 실행했다. + +```bash +python3 tools/test.py --mode=release \ + test/parallel/test-http-server-for-await-keepalive.js \ + test/parallel/test-http-server-for-await-keepalive-headers-sent.js +``` + +기존 관련 테스트도 함께 실행했다. + +```bash +python3 tools/test.py --mode=release \ + test/parallel/test-stream-destroy.js \ + test/parallel/test-http-server-incomingmessage-destroy.js \ + test/parallel/test-http-incoming-pipelined-socket-destroy.js +``` + +그리고 JavaScript lint도 확인했다. + +```bash +make lint-js +``` + +관련 테스트와 lint 모두 로컬에서 통과했다. + +## CI 및 Follow-up + +CI 과정에서 일부 실패가 있었지만, `x86_64-darwin` 환경의 `test-tick-processor-arguments.js` timeout으로 확인되었고 이번 HTTP 변경과 직접적인 관련은 없어 보였다. + +이후 수정 방향과 테스트 결과를 정리해 reviewer에게 다시 확인을 요청했다. + +또한 최신 `main`을 기준으로 rebase한 뒤 다시 리뷰를 요청했다. + +## 현재 상태 + +현재 PR #65674는 아직 Open 상태다. + +진행 과정은 다음과 같다. + +```text +Issue #49429 분석 +→ 문제 재현 +→ 불완전한 request body + keep-alive 재사용 문제 확인 +→ 초기 수정 +→ ronag Changes Requested +→ connection 재사용 방지 방향으로 수정 +→ headers 전송 전/후 회귀 테스트 추가 +→ mcollina 피드백 +→ 수정 위치를 Stream에서 HTTP로 이동 +→ IncomingMessage._destroy() 수정 +→ 관련 테스트 및 lint 통과 +→ 최신 main으로 rebase +→ 재리뷰 요청 +``` + +현재는 maintainer의 추가 리뷰를 기다리고 있다. + +## 배운 점 + +이번 기여를 통해 Stream lifecycle과 Socket lifecycle, HTTP connection lifecycle을 각각 구분해서 봐야 한다는 점을 배웠다. + +특히 request stream이 destroy됐다고 해서 underlying socket까지 자동으로 종료되는 것은 아니며, keep-alive connection의 재사용 여부는 HTTP layer에서 판단해야 한다는 점을 이해하게 됐다. + +또한 Core 코드에서는 단순히 문제가 발생한 위치를 수정하는 것이 아니라, 해당 동작을 어느 계층이 책임져야 하는지 판단하는 것이 중요하다는 점을 경험했다.