feat(ext/http): Rework Deno.serve using hyper 1.0-rc3 #18619

mmastrac · 2023-04-06T22:23:41Z

This is a rewrite of the Deno.serve API to live on top of hyper 1.0-rc3. The code should be more maintainable long-term, and avoids some of the slower mpsc patterns that made the older code less efficient than it could have been.

Missing features:

upgradeHttp and upgradeHttpRaw (upgradeWebSocket is available, however).
Automatic compression is unavailable on responses.

bartlomieju

A bit of a drive-by review

core/ops.rs

ext/http/01_http.js

ext/http/http2.rs

#18718) …polled ops (#18611)" This reverts commit 4317055. Reverting because we discovered while working on #18619 that it doesn't work correctly (sometimes waker just stops working).

bartlomieju · 2023-04-18T22:12:44Z

Cargo.lock

+ "hyper 0.14.26",
+ "hyper 1.0.0-rc.3",


For posterity: this is on purpose - we are going to migrate "Deno.serve()" API to Hyper 1.0.0, while "Deno.serveHttp" will stay on the older version. After merging this PR we will migrate "Deno.serveHttp()" to use "Deno.serve()" under the hood and remove old Hyper version.

bartlomieju · 2023-04-18T22:14:41Z

ext/http/00_serve.js

+  }
+}
+
+class InnerRequest {


Could you describe the purpose of this class? Can't we piggy back off of Request somehow?

I thought I had replied to this earlier, but the InnerRequest is actually wrapped in a Request which handles all the webidl identity and some level of caching.

Okay, I'm a bit worried about amount of garbage produced, but let's address that in a follow up

ext/http/00_serve.js

bartlomieju · 2023-04-18T22:15:53Z

ext/http/00_serve.js

+    // upgradeHttpRaw is async
+    if (upgradeType == "upgradeHttpRaw") {
+
+    }
+
+    // upgradeWebSocket is sync
+    if (upgradeType == "upgradeWebSocket") {
+      let [ response, ws ] = originalArgs;


Yikes, 3 different methods of upgrading seems like trouble 😬

ext/http/http_next.rs

bartlomieju · 2023-04-18T22:25:21Z

ext/http/http_next.rs

+  // TODO(mmastrac): This is faster if we can use tokio::spawn but then the send bounds get us
+  let safe_future = SafeFutureForSingleThread(Box::pin(async {


I'm still thinking that maybe we shouldn't use spawn_local here but make it an op that returns a Future - that way we're still in the same Tokio task as the main loop - let's revisit in a follow up

cli/tests/unit/serve_test.ts

bartlomieju · 2023-04-19T21:19:32Z

ext/http/00_serve.js

+  }
+}
+
+class InnerRequest {


Okay, I'm a bit worried about amount of garbage produced, but let's address that in a follow up

ext/http/00_serve.js

ext/http/http_next.rs

ext/http/request_body.rs

lucacasonato

Are we going to port Deno.serveHttp to use this new backing?

ext/http/01_http.js

lucacasonato · 2023-04-20T11:10:30Z

ext/http/00_serve.js

+    }
+
+    // upgradeWebSocket is sync
+    if (upgradeType == "upgradeWebSocket") {


The new implementation of upgradeWebSocket does not seem correct. The actual upgrade should not begin until the token Response from the Deno.upgradeWebSocket function has been returned from the serve handler. Deno.upgradeWebSocket should not initiate I/O. It only sets up the token response that can initiate an upgrade later. (It entangles a WebSocket object with a token Response object)

Ah yeah. This should probably be reflected in the documentation for upgradeWebSocket.

While I understand not wanting to break existing code, the old designs of these APIs are particularly nasty and deadlock-prone (upgradeHttp even has a deadlock warning in the doc page). Deno.serve is unstable and I think it's better to do things the right way in this API and migrate serveHttp over to the same in Deno 2.0.

As Deno.serve is unstable, there should be no breaking API concerns. Creating a response that the user has to shuttle off to the right place is a massive footgun and it seems like a big mistake to do that here rather than update the documentation

lucacasonato · 2023-04-20T11:12:44Z

ext/http/00_serve.js

+      if (this.#slabId === undefined) {
+        throw new TypeError("request closed");
+      }


It is exceedingly likely that this will break existing code, because significant existing code using the existing HTTP API relies on the fact that headers and url etc can be accessed even after the response is sent. An example is the GA4 handler in dotland.

Has there been any investigative work on this?

ext/http/00_serve.js

ext/http/http_next.rs

ext/http/response_body.rs

lucacasonato · 2023-04-20T11:32:20Z

ext/http/response_body.rs

+              return std::task::Poll::Ready(None);
+            }
+            // Re-arm the future
+            *future = stm.clone().read(64 * 1024);


I am not convinced always using 64kb is good.

deno_core's op_read_all dynamically grows the buffer over time until diminishing returns, and uses resource.size_hint to inform an optimal starting buffer size. Something like this would probably be useful here too.

https://github.com/denoland/deno/blob/main/core/ops_builtin.rs#L209-L275

64kB is exactly what's being read/written in op_http_write_resource right now so we're not really losing anything there, and there's already quite a bit of work in this PR.

Ok, maybe a good follow up.

Yeah, totally. I'll keep this for a future task as we might see some wins here for some tasks.

ext/http/response_body.rs

ext/net/raw.rs

bartlomieju · 2023-04-20T11:37:34Z

Are we going to port Deno.serveHttp to use this new backing?

Yes, see #18619 (comment)

cli/bench/http/deno_http_serve_https.js

ext/http/http_next.rs

cli/bench/http/deno_http_serve_https.js

littledivy

op_ws_create is not working properly. wss stream WPT are failing.

It works If I revert back to tokio_rustls TlsConnector. See littledivy@8beb63e

ext/websocket/lib.rs

littledivy

LGTM with the above comment addressed. Great work @mmastrac !

lucacasonato

Sorry for the late comment, but I just realized that this PR breaks h2c (plain text HTTP/2).

We should support this, as Deno.serveHttp supports it. It is widely used, for example when using h2c on Google Cloud Run, or Fly.io.

We need a test to ensure h2c support (or we need to add this as an immediate follow up).

…s requests cannot cause a panic (#18810) Fix a bug where we weren't saving `slabId` in #18619, plus add some robustness checks around multiple upgrades (w/test).

mmastrac marked this pull request as draft April 7, 2023 20:09

mmastrac force-pushed the http branch from d773a7b to 616742f Compare April 10, 2023 19:28

bartlomieju reviewed Apr 14, 2023

View reviewed changes

core/ops.rs Outdated Show resolved Hide resolved

ext/http/01_http.js Outdated Show resolved Hide resolved

ext/http/01_http.js Outdated Show resolved Hide resolved

ext/http/01_http.js Outdated Show resolved Hide resolved

ext/http/01_http.js Outdated Show resolved Hide resolved

bartlomieju reviewed Apr 15, 2023

View reviewed changes

ext/http/http2.rs Outdated Show resolved Hide resolved

mmastrac force-pushed the http branch 2 times, most recently from dc58d9c to 1549020 Compare April 15, 2023 01:05

bartlomieju mentioned this pull request Apr 15, 2023

Revert "perf(core): immediately schedule another tick if there are un… #18718

Merged

GulajavaMinistudio mentioned this pull request Apr 16, 2023

Create a new pull request by comparing changes across two branches javascript-indonesias/deno#499

Merged

mmastrac force-pushed the http branch 2 times, most recently from 9fd62a6 to 3c7971c Compare April 17, 2023 23:54

mmastrac force-pushed the http branch from 3c7971c to d3587ec Compare April 18, 2023 22:01

bartlomieju reviewed Apr 18, 2023

View reviewed changes

mmastrac force-pushed the http branch from cc9c8ce to 77b9485 Compare April 19, 2023 21:11

bartlomieju reviewed Apr 19, 2023

View reviewed changes

mmastrac changed the title ~~[WIP] Extremely experimental HTTP rewrite~~ Rework Deno.serve to live on top of Hyper 1.0 Apr 19, 2023

mmastrac marked this pull request as ready for review April 19, 2023 22:19

mmastrac changed the title ~~Rework Deno.serve to live on top of Hyper 1.0~~ feat(ext/http): Rework Deno.serve to live on top of Hyper 1.0 Apr 19, 2023

mmastrac changed the title ~~feat(ext/http): Rework Deno.serve to live on top of Hyper 1.0~~ feat(ext/http): Rework Deno.serve using hyper 1.0-rc3 Apr 19, 2023

mmastrac force-pushed the http branch from e0df9fb to 19397f1 Compare April 19, 2023 22:33

lucacasonato reviewed Apr 20, 2023

View reviewed changes

cli/bench/http/deno_http_serve_https.js Outdated Show resolved Hide resolved

mmastrac force-pushed the http branch from 2c06d15 to 6d399f7 Compare April 20, 2023 14:10

lucacasonato reviewed Apr 20, 2023

View reviewed changes

ext/http/http_next.rs Outdated Show resolved Hide resolved

mmastrac force-pushed the http branch from 48ef552 to a370e6d Compare April 20, 2023 20:14

ry reviewed Apr 21, 2023

View reviewed changes

cli/bench/http/deno_http_serve_https.js Outdated Show resolved Hide resolved

mmastrac force-pushed the http branch 2 times, most recently from 134b689 to 4e39eaa Compare April 21, 2023 15:00

mmastrac force-pushed the http branch 2 times, most recently from 92e2f42 to 35bf426 Compare April 21, 2023 18:50

mmastrac added 11 commits April 21, 2023 12:50

feat(ext/http): Rework Deno.serve using hyper 1.0-rc3

53b41c7

readTextFileSync

9969654

close() and upgrade sentinel cleanup

d2f2eb1

Defer websocket messages until we leave the callback

f51144d

lint

9044797

disable one-isolate-per-http test

54eac50

Add a wss test for websockets

74b1678

Allow embedders to extract network streams, and add a wss test

b8ff749

We can connect w/the ext/net connector directly

b4a6637

Force http/1 for websockets

0c09891

Comment out secure test

0db2191

mmastrac force-pushed the http branch 2 times, most recently from b29f5c7 to 65863d2 Compare April 21, 2023 18:53

Fix dep

74605e7

mmastrac force-pushed the http branch from 65863d2 to 74605e7 Compare April 21, 2023 18:55

littledivy requested changes Apr 22, 2023

View reviewed changes

ext/websocket/lib.rs Outdated Show resolved Hide resolved

ext/websocket/lib.rs Outdated Show resolved Hide resolved

bartlomieju added 2 commits April 22, 2023 17:15

Divy's review

4b11357

Merge branch 'main' into http

2e97b0e

littledivy reviewed Apr 22, 2023

View reviewed changes

ext/websocket/lib.rs Outdated Show resolved Hide resolved

littledivy approved these changes Apr 22, 2023

View reviewed changes

Divy's review 2

a772eee

lucacasonato reviewed Apr 22, 2023

View reviewed changes

mmastrac merged commit bdffcb4 into denoland:main Apr 22, 2023
11 checks passed

mmastrac mentioned this pull request Apr 23, 2023

fix(ext/http): ensure that multiple upgrades and multiple simultaneous requests cannot cause a panic #18810

Merged

fbaltor mentioned this pull request Jul 19, 2023

Accessing .url on cloned request throws in new Deno.serve API #19818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ext/http): Rework Deno.serve using hyper 1.0-rc3 #18619

feat(ext/http): Rework Deno.serve using hyper 1.0-rc3 #18619

mmastrac commented Apr 6, 2023 •

edited

bartlomieju left a comment

bartlomieju Apr 18, 2023

bartlomieju Apr 18, 2023

mmastrac Apr 19, 2023

bartlomieju Apr 19, 2023

bartlomieju Apr 18, 2023

bartlomieju Apr 18, 2023

bartlomieju Apr 19, 2023

lucacasonato left a comment

lucacasonato Apr 20, 2023

mmastrac Apr 20, 2023

lucacasonato Apr 20, 2023

lucacasonato Apr 20, 2023

mmastrac Apr 20, 2023 •

edited

lucacasonato Apr 20, 2023

mmastrac Apr 20, 2023

bartlomieju commented Apr 20, 2023

littledivy left a comment

littledivy left a comment

lucacasonato left a comment

		// TODO(mmastrac): This is faster if we can use tokio::spawn but then the send bounds get us
		let safe_future = SafeFutureForSingleThread(Box::pin(async {

feat(ext/http): Rework Deno.serve using hyper 1.0-rc3 #18619

feat(ext/http): Rework Deno.serve using hyper 1.0-rc3 #18619

Conversation

mmastrac commented Apr 6, 2023 • edited

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

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

lucacasonato 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

mmastrac Apr 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Apr 20, 2023

littledivy left a comment

Choose a reason for hiding this comment

littledivy left a comment

Choose a reason for hiding this comment

lucacasonato left a comment

Choose a reason for hiding this comment

mmastrac commented Apr 6, 2023 •

edited

mmastrac Apr 20, 2023 •

edited