Redesign of http server module #188

keroxp · 2019-02-11T15:18:32Z

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

bartlomieju · 2019-02-11T16:37:45Z

http/server.ts

-  resolve: () => void;
-  reject: () => void;
-}
+export type HttpHandler = (req: ServerRequest) => Promise<any>;


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?

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.

How about this?

Suggested change

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

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

what about Promise<unknown> ?

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.

bartlomieju · 2019-02-11T16:38:02Z

http/server.ts

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


Same as in HttpHandler

bartlomieju · 2019-02-11T16:38:50Z

http/server.ts

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


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

bartlomieju · 2019-02-11T16:39:26Z

http/server.ts

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


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

No, just forgotten to delete

bartlomieju · 2019-02-11T16:39:45Z

http/server.ts

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


Same as for HttpServerHandler

bartlomieju · 2019-02-11T16:40:02Z

http/server.ts

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


Not used anywhere

bartlomieju · 2019-02-11T16:41:16Z

http/server.ts

+    };
+  } = Object.create(null);
+
+  handle(pattern: string, handler: HttpHandler) {


~~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

bartlomieju · 2019-02-11T16:41:55Z

http/server.ts

-      // 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>([


Can you add a comment explaining this bit?

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

bartlomieju · 2019-02-11T16:42:48Z

http/server.ts

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


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

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

ry · 2019-02-11T17:37:37Z

async/deferred.ts

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


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?

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

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

bartlomieju · 2019-02-11T17:48:30Z

http/server.ts

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


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

bartlomieju · 2019-02-11T17:50:21Z

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

bartlomieju · 2019-02-11T17:55:51Z

http/server.ts

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


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

Exactly. I will add fallbacks.

bartlomieju · 2019-02-11T18:14:09Z

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 · 2019-02-11T18:38:16Z

@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 · 2019-02-11T18:46:10Z

@kevinkassimo No not yet.

kevinkassimo · 2019-02-11T19:05:15Z

@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 · 2019-02-11T20:15:01Z

@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":[]}

bartlomieju · 2019-02-11T21:51:36Z

http/server.ts

 }

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


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.

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

keroxp · 2019-02-11T22:13:44Z

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

Qard · 2019-02-11T22:22:12Z

async/deferred.ts

+    typeof x["resolve"] === "function" &&
+    typeof x["reject"] === "function"
+  );
+}


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. 🤔

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

keroxp · 2019-02-12T23:58:31Z

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

kevinkassimo · 2019-02-13T01:46:49Z

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

ry · 2019-02-13T02:09:36Z

http/server.ts

 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";


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.

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

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

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).

…tp-server-next

bartlomieju · 2019-02-13T16:01:45Z

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 · 2019-02-14T08:20:30Z

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

ry

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.

hayd · 2019-02-15T21:55:24Z

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 · 2019-02-18T20:35:36Z

I created a PR with promises/ #202

bartlomieju · 2019-02-19T17:31:29Z

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.

We need to consider the API changes here more carefully. This reverts commit da188a7. and commit 8569f15.

ry · 2019-02-19T17:41:25Z

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

We need to consider the API changes here more carefully. This reverts commit da188a7. and commit 8569f15.

ry · 2019-02-19T18:20:21Z

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.

Original: denoland/std@8569f15

We need to consider the API changes here more carefully. This reverts commit da188a7. and commit 8569f15. Original: denoland/std@57c9176

keroxp added 2 commits February 11, 2019 11:54

wip: refactoring http server

ea7c952

fix: some bugs and add tests

0a2c55e

bartlomieju reviewed Feb 11, 2019

View reviewed changes

bartlomieju mentioned this pull request Feb 11, 2019

Handle unread body from request #186

Closed

ry reviewed Feb 11, 2019

View reviewed changes

bartlomieju reviewed Feb 11, 2019

View reviewed changes

wip: wip

ede2fff

keroxp added 2 commits February 12, 2019 05:42

wip

ea4dff1

wip: revert to previous serve implementation

74d3d45

bartlomieju reviewed Feb 11, 2019

View reviewed changes

fix: revert more

e6e31ef

Qard reviewed Feb 11, 2019

View reviewed changes

fix: changed HttpHandler signature

d210a53

ry reviewed Feb 13, 2019

View reviewed changes

keroxp added 2 commits February 13, 2019 15:29

fix: use string | RegExp instead of path-to-regex

326f199

Merge branch 'master' of https://github.com/denoland/deno_std into ht…

6a1446a

…tp-server-next

moved: deferred.ts -> util

c084a62

Update README.md

92cdb03

fmt

8b131d1

ry approved these changes Feb 15, 2019

View reviewed changes

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

syumai mentioned this pull request Feb 15, 2019

Update http module syumai/dinatra#22

Merged

keroxp mentioned this pull request Feb 18, 2019

Downloading https://deno.land/x/util/deferred.ts... NOT FOUND #201

Closed

hayd mentioned this pull request Feb 18, 2019

Add promises #202

Closed

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

Revert "Redesign of http server module (denoland#188)"

8cf7b2a

We need to consider the API changes here more carefully. This reverts commit da188a7. and commit 8569f15.

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

Revert "Redesign of http server module (#188)"

57c9176

We need to consider the API changes here more carefully. This reverts commit da188a7. and commit 8569f15.

tplk mentioned this pull request Feb 20, 2019

fix: revert http server example on the homepage denoland/deno#1814

Merged

keroxp mentioned this pull request Feb 21, 2019

Discussions: http server responding way #211

Closed

ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019

Redesign of http server module (denoland/std#188)

57f4e6a

Original: denoland/std@8569f15

ry added a commit to ry/deno that referenced this pull request Oct 9, 2019

Revert "Redesign of http server module (denoland/std#188)"

d63a2e0

We need to consider the API changes here more carefully. This reverts commit da188a7. and commit 8569f15. Original: denoland/std@57c9176

kt3k mentioned this pull request May 3, 2023

BREAKING(semver): rewrite semver #3169

Closed

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

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

Redesign of http server module #188

Redesign of http server module #188

Conversation

keroxp commented Feb 11, 2019

bartlomieju Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkassimo Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Feb 11, 2019

bartlomieju Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Feb 11, 2019

kevinkassimo commented Feb 11, 2019 • edited Loading

keroxp commented Feb 11, 2019

kevinkassimo commented Feb 11, 2019

bartlomieju commented Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keroxp commented Feb 11, 2019

wrk results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keroxp commented Feb 12, 2019

kevinkassimo commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Feb 13, 2019

hayd commented Feb 14, 2019

ry left a comment

Choose a reason for hiding this comment

hayd commented Feb 15, 2019

hayd commented Feb 18, 2019

bartlomieju commented Feb 19, 2019

ry commented Feb 19, 2019

ry commented Feb 19, 2019

bartlomieju Feb 11, 2019 •

edited

Loading

bartlomieju Feb 11, 2019 •

edited

Loading

kevinkassimo Feb 11, 2019 •

edited

Loading

bartlomieju Feb 11, 2019 •

edited

Loading

kevinkassimo commented Feb 11, 2019 •

edited

Loading

bartlomieju commented Feb 11, 2019 •

edited

Loading