refactor: "Deno.serve()" API uses "Deno.serveHttp()" internally #18568

bartlomieju · 2023-04-02T21:36:17Z

This commit changes implementation of "Deno.serve()" API to use
"Deno.serveHttp()" under the hood. This change will allow us to
remove the "flash" server implementation, bringing stability to the
"Deno.serve()" API.

"cli/tests/unit/flash_test.ts" was renamed to "serve_test.ts".

Closes #15574
Closes #15504
Closes #15646
Closes #15909
Closes #15911
Closes #16828
Closes #18046
Closes #15869

bartlomieju · 2023-04-02T22:41:47Z

cli/tests/unit/flash_test.ts

@@ -301,51 +301,6 @@ Deno.test(
  },
 );

-Deno.test(
-  { permissions: { net: true } },
-  async function httpReadHeadersAfterClose() {


This is no longer necessary. Deno.serveHttp() returns properly formed response (ie. it doesn't do lazy loading of data), so headers are still available after the connection has been closed. This will allow us to remove some code in ext/fetch/23_request.js

bartlomieju · 2023-04-02T22:42:00Z

cli/tests/unit/flash_test.ts

 // https://github.com/denoland/deno/issues/17291
 Deno.test(
-  { permissions: { net: true } },
+  { permissions: { net: true }, ignore: true },


@littledivy please take a look

It appears this test was broken as well - there's no problem pushing strings to a ReadableStream, unless it has type: "bytes"

cli/tests/unit/flash_test.ts

bartlomieju · 2023-04-02T22:42:41Z

cli/tests/unit/flash_test.ts

@@ -1122,68 +1079,6 @@ Deno.test(
  },
 );

-Deno.test("upgradeHttpRaw tcp", async () => {


This unstable API will be removed along with flash implementation. Effectively there's no way to test it and the API will always throw.

bartlomieju · 2023-04-02T22:42:57Z

cli/tests/unit/flash_test.ts

 Deno.test(
-  { permissions: { net: true } },
+  { permissions: { net: true }, ignore: true },


@littledivy please take a look

I don't understand why this test requires Content-Length: 0 for the HEAD requests. It appears Hyper is not doing that and it's not mandatory to send this header. Can we remove this test case?

There shouldn't be any need for a zero Content-Length header, implicit or explicit. Does node have one? Even then, that's probably better to keep in the polyfill.

mmastrac

LGTM! I think test changes are very reasonable.

lucacasonato

I still think the cookie coalescing behaviour is not correct. Using ; as a cookie separator is correct, but using it as a header separator is not correct. The header separator is always , . Multiple headers with the same name may be coalesced together using the , (except set-cookie, which is special cased). This behaviour must not be different for cookie headers.

It is possible that this is a bug in fetch. If it is, we can fix it in a different PR. We need to have a test that the wire representation of the below response is either cookie: foo=bar\r\ncookie: bar=foo\r\n or cookie: foo=bar, bar=foo\r\n, but not cookie: foo=bar; bar=foo\r\n.

const h = new Headers();
h.append("cookie", "foo=bar");
h.append("cookie", "bar=foo");
console.log(h.get("cookie")); // "foo=bar, bar=foo"

new Response("", { headers: h })

The fetch spec requires that Headers objects do not treat cookie specially, and it mandates that any coalescing of headers must use , .

lucacasonato · 2023-04-03T15:39:36Z

Also we should have a test where we send the HTTP server a request using something other than fetch (ideally a hand-written H1 request), with definitely not coalesced cookie headers. The representation in req.headers should be , seperated, not ;. If it isn't there is an issue in hyper we'll have to look into.

lucacasonato

Retracting my request-changes, because this behaviour is already the case with serveHttp. See

deno/ext/http/lib.rs

Lines 555 to 560 in 6d68392

    
           // We treat cookies specially, because we don't want them to get them 
        
           // mangled by the `Headers` object in JS. What we do is take all cookie 
        
           // headers and concat them into a single cookie header, separated by 
        
           // semicolons. 
        
           let cookie_sep = "; ".as_bytes(); 
        
           let mut cookies = vec![];

With #18568 landed we no longer need "ext/flash". This commit removes "deno_flash" extension completely. This should have some impact on the binary and snapshot size. Closes #17356

sant123 · 2023-04-03T21:17:48Z

Does this solve #16267?

bartlomieju · 2023-04-03T22:57:00Z

Does this solve #16267?

@sant123, yes it's now solved.

This commit changes implementation of "Deno.serve()" API to use "Deno.serveHttp()" under the hood. This change will allow us to remove the "flash" server implementation, bringing stability to the "Deno.serve()" API. "cli/tests/unit/flash_test.ts" was renamed to "serve_test.ts". Closes #15574 Closes #15504 Closes #15646 Closes #15909 Closes #15911 Closes #16828 Closes #18046 Closes #15869

With #18568 landed we no longer need "ext/flash". This commit removes "deno_flash" extension completely. This should have some impact on the binary and snapshot size. Closes #17356

bartlomieju added 11 commits March 31, 2023 01:52

temp

7e7dbf2

temp

f144a29

Merge branch 'main' into deno_serve_serve_http

0990e92

remove dead code

0db1bc7

get it working with many tests!

3394687

make it work with TLS

da0c5a6

fix cookie concatenation test

9c21133

add some notes

2c728ee

reenable cookie test

dad3603

drop headers test

7763972

remove upgradeRaw test

01be5d5

bartlomieju commented Apr 2, 2023

View reviewed changes

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

bartlomieju commented Apr 2, 2023

View reviewed changes

cli/tests/unit/flash_test.ts Show resolved Hide resolved

bartlomieju commented Apr 2, 2023

View reviewed changes

bartlomieju added 2 commits April 3, 2023 00:47

import apis directly

306113b

simplify some code

140e7bd

bartlomieju marked this pull request as ready for review April 2, 2023 22:54

bartlomieju added 5 commits April 3, 2023 00:54

Merge branch 'main' into deno_serve_serve_http

e03f7d2

lint

f5b0fcb

update the test

cad905f

reenable tests on CI

6819690

fix controller test

4fb3b7a

bartlomieju requested review from mmastrac, dsherret and littledivy April 3, 2023 00:58

reset CI

e10efe0

mmastrac approved these changes Apr 3, 2023

View reviewed changes

bartlomieju added 3 commits April 3, 2023 17:01

remove head content-length: 0 test

8c2fc63

rename flash_test.ts to serve_test.ts

a57eb0f

Merge branch 'main' into deno_serve_serve_http

3103849

lucacasonato requested changes Apr 3, 2023

View reviewed changes

lucacasonato approved these changes Apr 3, 2023

View reviewed changes

bartlomieju merged commit 2846bbe into denoland:main Apr 3, 2023

bartlomieju deleted the deno_serve_serve_http branch April 3, 2023 15:44

This was referenced Apr 3, 2023

op_flash_make_request fast ops for multiple servers #15572

Closed

refactor: remove "ext/flash" #18578

Merged

flash: Deno.serve drops response body chunks if empty chunk is enqueued #17194

Closed

httpServerRejectsOnAddrInUse is flaky #16044

Closed

This was referenced Apr 3, 2023

Used port is not freed when using Deno.run with --watch #16267

Closed

Panicked when trying to Deno.serve() a WebSocketServer #18490

Closed

fix(ext/flash): Fix request object returning uri property correctly after websocket upgrade #18519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: "Deno.serve()" API uses "Deno.serveHttp()" internally #18568

refactor: "Deno.serve()" API uses "Deno.serveHttp()" internally #18568

bartlomieju commented Apr 2, 2023 •

edited

Loading

bartlomieju Apr 2, 2023

bartlomieju Apr 2, 2023

bartlomieju Apr 3, 2023

bartlomieju Apr 2, 2023

bartlomieju Apr 2, 2023

bartlomieju Apr 3, 2023

mmastrac Apr 3, 2023

mmastrac left a comment

lucacasonato left a comment

lucacasonato commented Apr 3, 2023

lucacasonato left a comment

sant123 commented Apr 3, 2023

bartlomieju commented Apr 3, 2023

	// We treat cookies specially, because we don't want them to get them
	// mangled by the `Headers` object in JS. What we do is take all cookie
	// headers and concat them into a single cookie header, separated by
	// semicolons.
	let cookie_sep = "; ".as_bytes();
	let mut cookies = vec![];

refactor: "Deno.serve()" API uses "Deno.serveHttp()" internally #18568

refactor: "Deno.serve()" API uses "Deno.serveHttp()" internally #18568

Conversation

bartlomieju commented Apr 2, 2023 • edited Loading

bartlomieju Apr 2, 2023

Choose a reason for hiding this comment

bartlomieju Apr 2, 2023

Choose a reason for hiding this comment

bartlomieju Apr 3, 2023

Choose a reason for hiding this comment

bartlomieju Apr 2, 2023

Choose a reason for hiding this comment

bartlomieju Apr 2, 2023

Choose a reason for hiding this comment

bartlomieju Apr 3, 2023

Choose a reason for hiding this comment

mmastrac Apr 3, 2023

Choose a reason for hiding this comment

mmastrac left a comment

Choose a reason for hiding this comment

lucacasonato left a comment

Choose a reason for hiding this comment

lucacasonato commented Apr 3, 2023

lucacasonato left a comment

Choose a reason for hiding this comment

sant123 commented Apr 3, 2023

bartlomieju commented Apr 3, 2023

bartlomieju commented Apr 2, 2023 •

edited

Loading