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

Redesign of http server module #188

Merged
merged 12 commits into from Feb 15, 2019

Conversation

7 participants
@keroxp
Copy link
Contributor

keroxp commented Feb 11, 2019

  • added HttpServer type for general purpose http server
  • ported path-to-regexp module
  • refactored server module
    • changed ServerRequest from class to interface
  • added e2e test for server
  • let serve() cancelable
resolve: () => void;
reject: () => void;
}
export type HttpHandler = (req: ServerRequest) => Promise<any>;

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

I think Promise<any> return type is bad, since it's not type safe. Can't it be Promise<void> since we don't expect any return value?

This comment has been minimized.

@keroxp

keroxp Feb 11, 2019

Author Contributor

I know. but :Promise<void> function can't accept one-line arrow function with non-void return vlaue:

server.handle("/", req => notVoidFunction(req))

I don't know which is better. But I chose one not restrictive.

This comment has been minimized.

@j-f1

j-f1 Feb 11, 2019

How about this?

Suggested change
export type HttpHandler = (req: ServerRequest) => Promise<any>;
export type HttpHandler = (req: ServerRequest) => void | Promise<void>;

This comment has been minimized.

@ry

ry Feb 11, 2019

Contributor

what about Promise<unknown> ?

This comment has been minimized.

@j-f1

j-f1 Feb 11, 2019

Or just

Suggested change
export type HttpHandler = (req: ServerRequest) => Promise<any>;
export type HttpHandler = (req: ServerRequest) => unknown;

since TS should let you await something that’s not a promise.

function serveConn(env: ServeEnv, conn: Conn, bufr?: BufReader) {
readRequest(conn, bufr).then(maybeHandleReq.bind(null, env, conn));
}
export type ServerResponder = (response: ServerResponse) => Promise<any>;

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

Same as in HttpHandler

}
yield readRequest(raced, async response => {
await writeResponse(raced, response);

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

I'm pretty sure we'll need access to ServerRequest during response, this need already arose in #186

status?: number;
headers?: Headers;
body?: Uint8Array | Reader;
export type HttpServerHandler = HttpHandler;

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

this type is not used anywhere and it's just an alias, is it needed?

This comment has been minimized.

@keroxp

keroxp Feb 11, 2019

Author Contributor

No, just forgotten to delete

public async body(): Promise<Uint8Array> {
return readAllIterator(this.bodyStream());
}
class ServerRequestInternal {

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

Same as for HttpServerHandler

async respond(r: Response): Promise<void> {
return writeResponse(this.w, r);
}
function isServerRequest(x): x is ServerRequest {

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

Not used anywhere

};
} = Object.create(null);

handle(pattern: string, handler: HttpHandler) {

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

I'm not sure if adding routing here is a good thing, but nevertheless I'd suggest addHandler name for this function.

Actually seen example on Twitter and I like it very much

// Continue read more from conn when user is done with the current req
// Moving this here makes it easier to manage
serveConn(env, result.conn, result.r);
const raced = await Promise.race<Conn | void>([

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

Can you add a comment explaining this bit?

This comment has been minimized.

@kevinkassimo

kevinkassimo Feb 11, 2019

Contributor

I remember that Promise.race has a chance of starving? As it always scans from left to right

reqQueue: ServerRequest[];
serveDeferred: Deferred;
}
export type ServerRequest = {

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

Out of curiosity, what's the advantage of using interface over class?

This comment has been minimized.

@keroxp

keroxp Feb 11, 2019

Author Contributor

For test mocking. In test codes, HttpHandler will be able to accept mocked request and dummy responder.

@@ -0,0 +1,27 @@
// Copyright 2018-2019 the Deno authors. All rights reserved. MIT license.

This comment has been minimized.

@ry

ry Feb 11, 2019

Contributor

I don't know about /async ... seems a bit too specific.

I feel like /util would be a useful junk drawer for one-off things like this.
@hayd what do you think?

This comment has been minimized.

@keroxp

keroxp Feb 11, 2019

Author Contributor

I also considered to let it promise FYI. I don't have any special reason to async

This comment has been minimized.

@ry

ry Feb 13, 2019

Contributor

Can you rename this to //util/deferred.ts ?

keys?: Key[];
handler: HttpHandler;
};
} = Object.create(null);

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

I think Map would be better suited for this, you'd get rid of Object.* methods

This comment has been minimized.

@keroxp

keroxp Feb 11, 2019

Author Contributor

Sure.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Feb 11, 2019

@keroxp could you also add example from Twitter to README?

};
}

async listen(addr: string, cancel: Deferred<void> = defer<void>()) {

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

I see 2 edge cases in this method:

  • developer not calling request.respond in handler
  • no URL match

I think these situations would hang the request...
I'd opt to throw error if respond is not called and return 404 response in respective situations

This comment has been minimized.

@keroxp

keroxp Feb 11, 2019

Author Contributor

Exactly. I will add fallbacks.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Feb 11, 2019

Just one last thing for discussion: I'm really not a fan of req.respond semantics and prefer to explicitly return response from handler: handler(req: ServerRequest) => Promise<ServerResponse>. Then it's on type level to make sure that handler is used properly.
I find this approach a bit easier to grok. What are your opinions?

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

kevinkassimo commented Feb 11, 2019

@keroxp Do you have a benchmark of this for http_bench.ts (using wrk) compared to the previous version? With this patch, it seems that listener.accept() would not be scheduled until the user handling code completes. No so sure about the performance implication...

@keroxp

This comment has been minimized.

Copy link
Contributor Author

keroxp commented Feb 11, 2019

@kevinkassimo No not yet.

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

kevinkassimo commented Feb 11, 2019

@keroxp FYI http_bench.ts is dying every single time when I test with wrk (I corrected the imports so it is another issue)

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Feb 11, 2019

@kevinkassimo

deno -D --allow-net ./http_bench.ts
wrk http://127.0.0.1:4500

results in:

DEBUG RS - msg_from_js Listen sync true
DEBUG RS - resource lookup 3
DEBUG RS - msg_from_js Accept sync false
DEBUG RS - resource lookup 4
DEBUG RS - msg_from_js Read sync false
DEBUG RS - v8_exception
{"message":"Uncaught EOF","sourceLine":"","lineNumber":0,"startPosition":-1,"endPosition":-1,"errorLevel":8,"startColumn":-1,"endColumn":-1,"isSharedCrossOrigin":true,"isOpaque":false,"frames":[]}
}

export async function* serve(addr: string) {
export async function* serve(

This comment has been minimized.

@bartlomieju

bartlomieju Feb 11, 2019

Contributor

After some more investigation I'm skeptical about rewriting this function (minus readRequest/writeResponse bit) right now. We still need to figure out how to reuse connections and support keep-alive and this solution doesn't solve these problems. We should discuss and cross check in other languages.

This comment has been minimized.

@keroxp

keroxp Feb 11, 2019

Author Contributor

I reverted implementation of serve() as possible. But race for cancellation may inevitably make performance degraded.

@keroxp

This comment has been minimized.

Copy link
Contributor Author

keroxp commented Feb 11, 2019

wrk results

Performance slightly degraded.

master

Running 30s test @ http://127.0.0.1:4500/
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    20.52ms    5.21ms 114.29ms   89.79%
    Req/Sec     1.52k   174.80     2.20k    86.44%
  543997 requests in 30.01s, 25.94MB read
  Socket errors: connect 0, read 429, write 0, timeout 0
Requests/sec:  18125.61
Transfer/sec:      0.86MB

latest

Running 30s test @ http://127.0.0.1:4500/
  12 threads and 400 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    22.23ms    3.68ms  64.95ms   81.69%
    Req/Sec     1.42k   181.19     3.82k    86.01%
  507838 requests in 30.10s, 24.22MB read
  Socket errors: connect 0, read 445, write 0, timeout 0
Requests/sec:  16869.39
Transfer/sec:    823.70KB
typeof x["resolve"] === "function" &&
typeof x["reject"] === "function"
);
}

This comment has been minimized.

@Qard

Qard Feb 11, 2019

Isn't this testing the same thing as the Deferred type restriction itself? And it's not even used anywhere outside of the test file. 🤔

This comment has been minimized.

@keroxp

keroxp Feb 12, 2019

Author Contributor

It's custom type guard for Deferred object. Mostly used to resolve it from union type.

@keroxp

This comment has been minimized.

Copy link
Contributor Author

keroxp commented Feb 12, 2019

@kevinkassimo @bartlomieju Could you run benchmark on your machine again? I don't know precisely how performance changed after reverting.

@kevinkassimo

This comment has been minimized.

Copy link
Contributor

kevinkassimo commented Feb 13, 2019

@keroxp The req/sec seems very similar to master now on my machine.
(Sorry for the delay, was debugging denoland/deno#1756)

import { TextProtoReader } from "../textproto/mod.ts";
import { STATUS_TEXT } from "./http_status.ts";
import { assert } from "../testing/mod.ts";
import { defer, Deferred } from "../async/deferred.ts";
import { Key, pathToRegexp } from "./path_to_regexp.ts";

This comment has been minimized.

@ry

ry Feb 13, 2019

Contributor

I don't think we should have fancy path matching in this module. Happy to have something like //http/router.ts but in this server.ts module things should be very simple.

In Go, ServeMux does not do such complex things: https://golang.org/pkg/net/http/#ServeMux

I would very happily have a ServeMux class here that implemented the above routing logic.

This comment has been minimized.

@keroxp

keroxp Feb 13, 2019

Author Contributor

@ry dropped path-to-regex, and implemented pure regexp based route matching

This comment has been minimized.

@ry

ry Feb 13, 2019

Contributor

I'm still concerned that this is using RegExp rather than simple prefix matching... Is it possible to move this routing logic out of server.ts ?

server.ts is meant to be a very low-level web server, which everyone case use. In the case of the routing, I can imagine different people having different solutions.

I think the most complex routing inside server.ts should be limited to what is described here: https://golang.org/pkg/net/http/#ServeMux

This comment has been minimized.

@keroxp

keroxp Feb 14, 2019

Author Contributor

I don't think that it is much complicated...routing with string or regex can be used for general purpose. serve() should be considered as the lowest http server api.

Is it possible to move this routing logic out of server.ts ?

Of course. But even if logic become more simple, code won't be changed. We eventually call req.url.match(pattern). Differences are only storing match result in req or not. I hesitate that we have two similar server api (ServerMux and Router).

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Feb 13, 2019

Current:

Running 10s test @ http://127.0.0.1:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     0.86ms  341.42us  13.80ms   94.92%
    Req/Sec     5.96k   628.81     6.38k    94.06%
  119869 requests in 10.10s, 5.72MB read
Requests/sec:  11866.09
Transfer/sec:    579.40KB

master:

Running 10s test @ http://127.0.0.1:4500
  2 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   820.35us  285.22us  11.99ms   93.66%
    Req/Sec     6.20k   716.39     6.71k    88.12%
  124593 requests in 10.10s, 5.94MB read
Requests/sec:  12335.59
Transfer/sec:    602.32KB
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Feb 14, 2019

Please don't create a "util" directory...

fmt
@ry

ry approved these changes Feb 15, 2019

Copy link
Contributor

ry left a comment

LGTM - this is a great improvement - I don't want to hold it up any longer.

Regarding @hayd's comment on util... I'm going to land this now - but can we think about a better place for things like deferred()? I agree "util" is quite generic (which I assume is @hayd's complaint) but I think "async" is too specific. I want to avoid having a very large number of directories in the root of deno_std - it should be less than, say, 100. But this requires that we carefully categorize modules, which is difficult. Any organizational help here would be appreciated. Renaming things now rather than later is better for stability.

@ry ry merged commit 8569f15 into denoland:master Feb 15, 2019

2 checks passed

denoland.deno_std #20190215.5 succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@syumai syumai referenced this pull request Feb 15, 2019

Merged

Update http module #22

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Feb 15, 2019

The use of util means that /x/http/server.ts is broken atm. Putting it temporarily in http be okay, but using either /async or /promise, and adding that to the registry seems preferable...

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Feb 18, 2019

I created a PR with promises/ #202

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Feb 19, 2019

Since this PR caused chaos in libraries using http, might we consider reverting it for v0.3? I think we should wait with changes to existing serve API and make a discussion about new design.

ry added a commit to ry/deno_std that referenced this pull request Feb 19, 2019

Revert "Redesign of http server module (denoland#188)"
We need to consider the API changes here more carefully.

This reverts commit da188a7.
and commit 8569f15.
@ry

This comment has been minimized.

Copy link
Contributor

ry commented Feb 19, 2019

Yeah - I'm in agreement. I skimmed some of the API changes here that I want to consider more carefully - also not so happy with the regex usage.

#206

ry added a commit that referenced this pull request Feb 19, 2019

Revert "Redesign of http server module (#188)"
We need to consider the API changes here more carefully.

This reverts commit da188a7.
and commit 8569f15.
@ry

This comment has been minimized.

Copy link
Contributor

ry commented Feb 19, 2019

Revert is landed.

Sorry @keroxp - we need to break up this refactor into smaller bits - I'm happy to have improvements - but we need to more carefully consider the API changes.

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.