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

Enforce HTTP/1.1 pipeline response order #331

Merged
merged 3 commits into from Apr 13, 2019

Conversation

3 participants
@kevinkassimo
Copy link
Contributor

commented Apr 9, 2019

  • Using a Map to track pipelined requests and resolve only when the previous request is responded.
  • Added test (racing_server_test.ts). This test have been confirmed to fail before this patch.

Slight performance impact, yet tail latency is still about the same:

Before:

Running 10s test @ http://localhost:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   382.03us  216.38us   6.15ms   94.04%
    Req/Sec    13.63k     1.64k   15.48k    86.14%
  274043 requests in 10.10s, 13.07MB read
Requests/sec:  27133.13
Transfer/sec:      1.29MB

After:

Running 10s test @ http://localhost:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   388.00us  176.04us   2.82ms   93.29%
    Req/Sec    13.18k   832.27    14.60k    67.33%
  264768 requests in 10.10s, 12.63MB read
Requests/sec:  26214.26
Transfer/sec:      1.25MB

@kevinkassimo kevinkassimo force-pushed the kevinkassimo:http/pipeline branch from 6a42062 to 8f3d2bb Apr 9, 2019

@kevinkassimo kevinkassimo force-pushed the kevinkassimo:http/pipeline branch from 8f3d2bb to 3f62bda Apr 9, 2019

@zekth

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Use Invoke-WebRequest -Uri "URL" in Powershell for windows.

@kevinkassimo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@zekth Guess I have to blindly try here (I don't have a windows machine)

Does it open up a raw TCP connection?

let server;
async function startServer(): Promise<void> {
server = run({
args: ["deno", "--A", "http/racing_server.ts"],

This comment has been minimized.

Copy link
@ry

ry Apr 9, 2019

Contributor

You don't need to do this as a separate process, you can just launch the server in-process here

Use Deno.dial or fetch to access

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Apr 9, 2019

Author Contributor

Totally forgot about Deno.dial 😂 Updating.

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Apr 9, 2019

Author Contributor

About using the same process, we do not have the ability to close the HTTP server so the test would not terminate...

This comment has been minimized.

Copy link
@ry

ry Apr 10, 2019

Contributor

why can't we terminate an http server? does Deno.close(server.rid) not work?

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Apr 10, 2019

Author Contributor

Since server returned by serve() is an async iterator of ServerRequests and does not have the rid field.
(I could still add it to each request or to the async iterator itself but feels like a hack...)

This comment has been minimized.

Copy link
@ry

ry Apr 10, 2019

Contributor

Having ServerRequest point to its own server seems reasonable.

for await (let req of serve(":8000")) {
  req.server.close();
}

consider s/server/listener/

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Apr 10, 2019

Author Contributor

Brief experiment reveals 2 problems at this moment with this:

  1. We cannot simply expose the listener and allow users to directly call close on it. Since we do not automatically close connections etc. when the last JS reference is dropped, we have to do some cleanup work.
  2. There are too many panics on the Rust backend that essentially would always trigger panic here and crash the process if we call listener.close() while there is a pending read.

On the other hand, this feels like a separate issue than the one attempted to be addressed here. Probably we could make it another PR? (after fixes for Rust panics land in deno)

Show resolved Hide resolved http/racing_server.ts Outdated

kevinkassimo added some commits Apr 9, 2019

@ry

ry approved these changes Apr 13, 2019

Copy link
Contributor

left a comment

LGTM - no comments. Thanks for this important fix and sorry for the delayed review.

We should get server.close() working. It shouldn't be necessary to start a subprocess here.

@ry ry merged commit 144ef0e into denoland:master Apr 13, 2019

2 checks passed

denoland.deno_std #20190409.12 succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.