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

fix: [http] Consume unread body and trailers before reading next request #3990

Merged
merged 25 commits into from
Feb 25, 2020
Merged

fix: [http] Consume unread body and trailers before reading next request #3990

merged 25 commits into from
Feb 25, 2020

Conversation

keroxp
Copy link
Contributor

@keroxp keroxp commented Feb 13, 2020 •
edited
Loading

cc @kevinkassimo @bartlomieju

  • Added ServerRequest.finalize(): consuming all unread body stream and trailers.
    • This is cleanup method for reading next request from same keep-alive connection.
    • Needed when handler didn't consume all body and trailers even after responding.
  • refactor: ServerRequest._bodyStream(), ServerRequestBody are removed.
    • Now using bodyReader() and chunkedBodyReader() instead.
  • fix: Trailers should only be read transfer-encoding is chunked and trailer header is set and its value is valid.
  • fix: use Headers.append() on reading trailers.
  • fix: delete trailer field from headers after reading trailers.
  • reorg: Several functions related to IO are moved into http/io.ts
  • Adding more tests...

nayeemrmn
nayeemrmn requested changes Feb 13, 2020
View reviewed changes
Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

Can we avoid redundant file names?

std/http/http_io.ts -> std/http/io.ts

@keroxp keroxp changed the title WIP: [http] Consume unread body and trailers before reading next request fix: [http] Consume unread body and trailers before reading next request Feb 14, 2020
@keroxp
Copy link
Contributor Author

keroxp commented Feb 15, 2020

Now ready for review.

@bartlomieju
Copy link
Member

Sorry for delay @keroxp but I've been travelling. I'll review in the coming days

@keroxp
Copy link
Contributor Author

keroxp commented Feb 22, 2020

@ry Would you start reviewing? It's hard to keep syncing branch to the latest...

This patch contains an important adjustment to HTTP/1.1 protocol about handling request from keep-alive connection. Yesterday I added new integration test cases in racing_server_test.ts. Those cases won't be passed in master branch.

@bartlomieju
Copy link
Member

bartlomieju commented Feb 23, 2020 •
edited
Loading

@keroxp sorry for the delay. I merged master into your branch; I don't have any comments for code; looks thoroughly tested. If CI goes green I think we can land it

@keroxp
Copy link
Contributor Author

keroxp commented Feb 24, 2020

@bartlomieju Thanks!
@ry How about?

ry
ry approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - I'm sorry for the slow review. This is a complex patch. Generally looks good, but I expect we'll need to take a second pass at some point.

@ry ry merged commit 22f88b9 into denoland:master Feb 25, 2020
@keroxp
Copy link
Contributor Author

keroxp commented Feb 25, 2020

@ry Yeah thanks. I ported most of logic from my project (servestjs) but there may be bugs. I hope this patch improves deno's http stability.

dubiousjim added a commit to dubiousjim/deno that referenced this pull request Feb 26, 2020
@dubiousjim
Merge tag 'dm' into work2.2
b145285 
* tag 'dm': (58 commits)
  Port permission_prompt_tests to Rust (denoland#4129)
  upgrade: TypeScript 3.8 (denoland#4100)
  Remove _async from method names since _sync are gone (denoland#4128)
  ci: Use fetch-depth: 5 (denoland#4127)
  fix: Resolve makeTemp* paths from CWD (denoland#4104)
  Clean up how we use opIds (denoland#4118)
  Fix issues with JavaScript importing JavaScript. (denoland#4120)
  fix: [http] Consume unread body and trailers before reading next request (denoland#3990)
  Remove ansi_term dependency (denoland#4116)
  port tools/repl_test.py to Rust (denoland#4096)
  Revert "Remove ansi_term dependency"
  fix: createSecKey logic (denoland#4063)
  Add Deno.Err change missing from previous commit (denoland#4113)
  rename Deno.Err -> Deno.errors (denoland#4093)
  doc: Remove old link to deno types output (denoland#4111)
  feat: Add Deno.formatDiagnostics (denoland#4032)
  disable std/examples/chat tests for windows (denoland#4109)
  update build instructions for windows (denoland#4027)
  Remove ansi_term dependency (denoland#4106)
  fix: add mutex guard for permission_prompt tests (denoland#4105)
  ...
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
@keroxp
fix: [http] Consume unread body and trailers before reading next requ…
82ecb8d 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@keroxp
fix: [http] Consume unread body and trailers before reading next requ…
e41b214 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@keroxp @caspervonb
fix: [http] Consume unread body and trailers before reading next requ…
9890565 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@keroxp @caspervonb
fix: [http] Consume unread body and trailers before reading next requ…
37e4931 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@keroxp @caspervonb
fix: [http] Consume unread body and trailers before reading next requ…
0da877a 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@keroxp @caspervonb
fix: [http] Consume unread body and trailers before reading next requ…
6b2e7af 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@keroxp @caspervonb
fix: [http] Consume unread body and trailers before reading next requ…
8cc5e90 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@keroxp @denobot
fix: [http] Consume unread body and trailers before reading next requ…
ded719f 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
@keroxp @denobot
fix: [http] Consume unread body and trailers before reading next requ…
761c2c1 
…est (denoland/deno#3990)

- Added `ServerRequest.finalize()`:  consuming all unread body stream and trailers.
  - This is cleanup method for reading next request from same keep-alive connection.
  - Needed when handler didn't consume all body and trailers even after responding.
- refactor: `ServerRequest._bodyStream()`, `ServerRequestBody` are removed.
  - Now using `bodyReader()` and `chunkedBodyReader()` instead.
- fix: Trailers should only be read `transfer-encoding` is `chunked` and `trailer` header is set and its value is valid.
- fix: use `Headers.append()` on reading trailers.
- fix: delete `trailer` field from headers after reading trailers.
- reorg: Several functions related to IO are moved into `http/io.ts`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nayeemrmn nayeemrmn nayeemrmn requested changes

@ry ry ry approved these changes

@bartlomieju bartlomieju Awaiting requested review from bartlomieju

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@keroxp @bartlomieju @ry @nayeemrmn