feat: native HTTP bindings #9935

bartlomieju · 2021-03-30T16:52:16Z

streaming response
streaming request
typescript declarations
be able to wrap TLS stream

Towards #3995

op_crates/http/lib.rs

cli/bench/deno_http.js

core/core.js

cli/bench/deno_http.js

cli/tests/unit/http_test.ts

op_crates/http/00_http.js

op_crates/http/lib.rs

cli/tests/unit/http_test.ts

nayeemrmn · 2021-04-07T16:09:10Z

cli/dts/lib.deno.unstable.d.ts

+   * const requests = await Deno.startHttp(conn);
+   * ```
+   */
+  export function startHttp(conn: Conn): AsyncIterableIterator<HttpEvent>;


We should downgrade this return type to AsyncIterable<HttpEvent>, and now or in the future upgrade that to HttpConn extends AsyncIterable<HttpEvent> which exposes .rid and .close() if we want those things.

Returning or extending AsyncIterableIterator<_> (and overloading .return() for closing) isn't the way to go. I pushed against that for Listener early on, but unfortunately watchFs() slipped through and demonstrated this. I've been meaning to fix that for 2.0.

Returning or extending AsyncIterableIterator<_> (and overloading .return() for closing) isn't the way to go

What's your reasoning behind this?

For starters .return() is optional on AsyncIterableIterator: https://github.com/denoland/deno/blob/main/cli/tests/unit/fs_events_test.ts#L74. It doesn't align with the default implementation of .return() on generators, which is supposed to break any pending loops without error. I don't think this is supposed to be manually implemented. It's async for no reason, but closing should always be synchronous.

The iterator protocol methods like .next() aren't a nice thing to directly expose on structures with other things, it would be better to have just [Symbol.asyncIterator]() and maybe add an accept() method to await one request like with Listener. However we don't actually need to adjust the implementation from what it is now (other than removing .return()) since it's probably more efficient -- that would also match Listener.

Ok. I have changed this around so that there is no return method and that the requests iterator must be closed manually. Is this what you had in mind?

and that the requests iterator must be closed manually

The resource should still be closed automatically if connectionClosed is true, if I seemed to have suggested otherwise. (re 336728c#diff-2b90f55ce18f5dd4bac8df7c9f0ff3a77558866bc8fcf02146dee2548b4948abL50-R54)

runtime/js/40_http.js

cli/dts/lib.deno.unstable.d.ts

runtime/js/40_http.js

nayeemrmn

This is how to write the rewrite the .next() calls with AsyncIterable<_>. But to have a proper API for receiving one request, I suggest adding an equivalent of Listener.accept().

nayeemrmn · 2021-04-08T13:58:10Z

cli/dts/lib.deno.unstable.d.ts

+    respondWith(r: Response | Promise<Response>): void;
+  }
+
+  export interface HttpConn extends AsyncIterableIterator<RequestEvent> {


Suggested change

export interface HttpConn extends AsyncIterableIterator<RequestEvent> {

export interface HttpConn extends AsyncIterable<RequestEvent> {

nayeemrmn · 2021-04-08T13:59:26Z

cli/tests/unit/http_test.ts

+      const listener = Deno.listen({ port: 4501 });
+      const conn = await listener.accept();
+      const requests = Deno.startHttp(conn);
+      const { value: { request, respondWith }, done } = await requests.next();


Suggested change

const { value: { request, respondWith }, done } = await requests.next();

const { value: { request, respondWith }, done } = await requests[Symbol.asyncIterator]().next();

nayeemrmn · 2021-04-08T14:00:56Z

cli/tests/unit/http_test.ts

+      const listener = Deno.listen({ port: 4501 });
+      const conn = await listener.accept();
+      const requests = Deno.startHttp(conn);
+      const { value: { request, respondWith } } = await requests.next();


Suggested change

const { value: { request, respondWith } } = await requests.next();

const { value: { request, respondWith } } = await requests[Symbol.asyncIterator]().next();

nayeemrmn · 2021-04-08T14:01:10Z

cli/tests/unit/http_test.ts

+      // TODO(ry) If we don't call requests.next() here we get "error sending
+      // request for url (https://localhost:4501/): connection closed before
+      // message completed".
+      const { done } = await requests.next();


Suggested change

const { done } = await requests.next();

const { done } = await requests[Symbol.asyncIterator]().next();

nayeemrmn · 2021-04-08T14:01:50Z

cli/tests/unit/http_test.ts

+    const listener = Deno.listen({ port: 4501 });
+    const conn = await listener.accept();
+    const requests = Deno.startHttp(conn);
+    const { done } = await requests.next();


Suggested change

const { done } = await requests.next();

const { done } = await requests[Symbol.asyncIterator]().next();

nayeemrmn · 2021-04-08T14:02:01Z

cli/tests/unit/http_test.ts

+      });
+      const conn = await listener.accept();
+      const requests = Deno.startHttp(conn);
+      const { value: { request, respondWith } } = await requests.next();


Suggested change

const { value: { request, respondWith } } = await requests.next();

const { value: { request, respondWith } } = await requests[Symbol.asyncIterator]().next();

nayeemrmn · 2021-04-08T14:02:46Z

cli/tests/unit/http_test.ts

+      // TODO(ry) If we don't call requests.next() here we get "error sending
+      // request for url (https://localhost:4501/): connection closed before
+      // message completed".
+      const { done } = await requests.next();


Suggested change

const { done } = await requests.next();

const { done } = await requests[Symbol.asyncIterator]().next();

ry · 2021-04-08T14:24:07Z

@nayeemrmn That API seems rather annoying. Why shouldn't it be an AsyncIterableIterator?

lucacasonato · 2021-04-08T14:53:25Z

I think generally nayeem is on the right path. It makes sense to me if the API looked like this:

interface HttpConn extends AsyncIterable<RequestEvent> {
    readonly rid: number;

    next(): Promise<RequestEvent | null>;
    close(): void;
}

lucacasonato · 2021-04-08T14:55:57Z

next returning null could be controversial. It would replace the BadResource error for the next call. We could also do the same as listener though, where we actually throw the BadResource error. Returning null would align to how Stream#next returns an Option in Rust though.

Co-authered-by: Luca Casonato <lucacasonato@yahoo.com> Co-authered-by: Ben Noordhuis <info@bnoordhuis.nl> Co-authered-by: Ryan Dahl <ry@tinyclouds.org>

nayeemrmn · 2021-04-08T15:04:23Z

I'm in favour of the BadResource error over being nullable. If you're trying to "receive one", you probably have an assertion that you'll get it and a possible error aligns more with that. It should be a more direct implementation in terms of calling the op. The verbose iterator protocol is still accessible through httpConn[Symbol.asyncIterator]() so the nice API should go in the opposite direction. Plus it's more consistent for now.

interface HttpConn extends AsyncIterable<RequestEvent> {
    readonly rid: number;

    accept(): Promise<RequestEvent>;
    close(): void;
}

bartlomieju

Ship it 🚀

bartlomieju · 2021-04-08T20:20:25Z

runtime/ops/http.rs

+    if self.inner.borrow().is_some() {
+      Poll::Pending
+    } else {
+      Poll::Ready(Ok(()))
+    }


I believe is Poll::Pending is returned this task will never be woken up again

bartlomieju · 2021-04-08T20:20:44Z

runtime/js/40_http.js

+  function readRequest(requestRid, zeroCopyBuf) {
+    return Deno.core.jsonOpAsync(
+      "op_http_request_read",
+      requestRid,
+      zeroCopyBuf,
+    );
+  }
+
+  function respond(responseSenderRid, resp, zeroCopyBuf) {
+    return Deno.core.jsonOpSync("op_http_response", [
+      responseSenderRid,
+      resp.status ?? 200,
+      flatEntries(resp.headers ?? {}),
+    ], zeroCopyBuf);
+  }


For a follow up: these two functions seem superfluous

ry

LGTM!

Although this looks like one large PR, this is actually the cumulation of months of attempts at building a binding to hyper. @bartlomieju drove this to completion but various proof-of-concepts were done by @bnoordhuis and @lucacasonato. The binding relies heavily on the serde_v8 optimization work that @AaronO has been doing - this would have been a much larger and slower undertaking without serde_v8.

bartlomieju commented Mar 31, 2021

View reviewed changes

op_crates/http/lib.rs Outdated Show resolved Hide resolved

lucacasonato reviewed Mar 31, 2021

View reviewed changes

cli/bench/deno_http.js Outdated Show resolved Hide resolved

bartlomieju commented Mar 31, 2021

View reviewed changes

core/core.js Outdated Show resolved Hide resolved

rektide mentioned this pull request Apr 1, 2021

Support HTTP/2, HTTP/3 and QUIC in HTTP server #3995

Closed

ry force-pushed the feat_hyper_bindings branch 2 times, most recently from abc88b3 to 6af6480 Compare April 2, 2021 16:38

evanwashere reviewed Apr 2, 2021

View reviewed changes

bartlomieju commented Apr 2, 2021

View reviewed changes

op_crates/http/lib.rs Outdated Show resolved Hide resolved

ry force-pushed the feat_hyper_bindings branch from 50af461 to fbddf0a Compare April 5, 2021 19:58

bartlomieju force-pushed the feat_hyper_bindings branch 2 times, most recently from 5a4a122 to 936af2a Compare April 7, 2021 15:05

nayeemrmn reviewed Apr 7, 2021

View reviewed changes

cli/tests/unit/http_test.ts Outdated Show resolved Hide resolved

nayeemrmn reviewed Apr 7, 2021

View reviewed changes

lucacasonato reviewed Apr 7, 2021

View reviewed changes

runtime/js/40_http.js Outdated Show resolved Hide resolved

ry force-pushed the feat_hyper_bindings branch from 3a68712 to 56c7d9a Compare April 7, 2021 21:51

ry changed the title ~~[WIP] feat: native HTTP bindings~~ feat: native HTTP bindings Apr 7, 2021

nayeemrmn reviewed Apr 8, 2021

View reviewed changes

cli/dts/lib.deno.unstable.d.ts Outdated Show resolved Hide resolved

ghost reviewed Apr 8, 2021

View reviewed changes

runtime/js/40_http.js Outdated Show resolved Hide resolved

ry mentioned this pull request Apr 8, 2021

Q1 2021 roadmap #8824

Closed

22 tasks

nayeemrmn reviewed Apr 8, 2021

View reviewed changes

feat: Add hyper bindings

f6d4a4b

Co-authered-by: Luca Casonato <lucacasonato@yahoo.com> Co-authered-by: Ben Noordhuis <info@bnoordhuis.nl> Co-authered-by: Ryan Dahl <ry@tinyclouds.org>

ry force-pushed the feat_hyper_bindings branch from 6c74a71 to f6d4a4b Compare April 8, 2021 14:59

lucacasonato and others added 2 commits April 8, 2021 19:14

update binding to be more ergonomic

4537790

Merge branch 'main' into feat_hyper_bindings

67952f9

bartlomieju commented Apr 8, 2021

View reviewed changes

Fix benchmark

5194fa8

next -> nextRequest

2d4a840

ry approved these changes Apr 8, 2021

View reviewed changes

ry merged commit 70af812 into denoland:main Apr 8, 2021

kitsonk mentioned this pull request Apr 8, 2021

Support Unstable Deno Native HTTP Bindings oakserver/oak#289

Closed

irustm mentioned this pull request Apr 9, 2021

Add Deno Native HTTP bindings alosaur/alosaur#142

Closed

keroxp mentioned this pull request Apr 9, 2021

feat: Native http binding keroxp/servest#158

Merged

ynwd mentioned this pull request Apr 10, 2021

Support Unstable Deno Native HTTP Bindings fastrodev/fastro#18

Closed

bartlomieju deleted the feat_hyper_bindings branch April 12, 2021 17:16

asos-craigmorten mentioned this pull request Apr 19, 2021

[FEAT] Adopt Native HTTP Server cmorten/opine#115

Closed

waldyrious mentioned this pull request Apr 21, 2021

QUIC & HTTP/3 support #1558

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: native HTTP bindings #9935

feat: native HTTP bindings #9935

bartlomieju commented Mar 30, 2021 •

edited by lucacasonato

Loading

nayeemrmn Apr 7, 2021

ry Apr 7, 2021

nayeemrmn Apr 7, 2021 •

edited

Loading

ry Apr 8, 2021

nayeemrmn Apr 8, 2021 •

edited

Loading

nayeemrmn left a comment

nayeemrmn Apr 8, 2021

nayeemrmn Apr 8, 2021

nayeemrmn Apr 8, 2021

nayeemrmn Apr 8, 2021

nayeemrmn Apr 8, 2021

nayeemrmn Apr 8, 2021

nayeemrmn Apr 8, 2021

ry commented Apr 8, 2021

lucacasonato commented Apr 8, 2021 •

edited

Loading

lucacasonato commented Apr 8, 2021 •

edited

Loading

nayeemrmn commented Apr 8, 2021

bartlomieju left a comment

bartlomieju Apr 8, 2021

bartlomieju Apr 8, 2021

ry left a comment •

edited

Loading

	export interface HttpConn extends AsyncIterableIterator<RequestEvent> {
	export interface HttpConn extends AsyncIterable<RequestEvent> {

	const { value: { request, respondWith }, done } = await requests.next();
	const { value: { request, respondWith }, done } = await requests[Symbol.asyncIterator]().next();

	const { value: { request, respondWith } } = await requests.next();
	const { value: { request, respondWith } } = await requests[Symbol.asyncIterator]().next();

	const { done } = await requests.next();
	const { done } = await requests[Symbol.asyncIterator]().next();

feat: native HTTP bindings #9935

feat: native HTTP bindings #9935

Conversation

bartlomieju commented Mar 30, 2021 • edited by lucacasonato Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayeemrmn Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayeemrmn Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

nayeemrmn left a 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

ry commented Apr 8, 2021

lucacasonato commented Apr 8, 2021 • edited Loading

lucacasonato commented Apr 8, 2021 • edited Loading

nayeemrmn commented Apr 8, 2021

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry left a comment • edited Loading

Choose a reason for hiding this comment

bartlomieju commented Mar 30, 2021 •

edited by lucacasonato

Loading

nayeemrmn Apr 7, 2021 •

edited

Loading

nayeemrmn Apr 8, 2021 •

edited

Loading

lucacasonato commented Apr 8, 2021 •

edited

Loading

lucacasonato commented Apr 8, 2021 •

edited

Loading

ry left a comment •

edited

Loading