feat: support http2

[tsctx]

No description provided.

[tsctx]

Undici had a bug that allowed ':', which was not allowed in the header name, but now that it is fixed.

[yusukebe]

Are these lines really necessary? These affect performance, so I would like to remove them if it is not a practical problem.

[tsctx]

What version of undici are you using?
The whatwg fetch spec does not allow h2 pseudo-headers containing : and throws an error. This check is necessary.

> $ node
Welcome to Node.js v21.6.1.
Type ".help" for more information.
> new Headers({ ":authority": "localhost" });
Uncaught TypeError: Headers.append: ":authority" is an invalid header name.
    at webidl.errors.exception (node:internal/deps/undici/undici:1636:14)
    at webidl.errors.invalidArgument (node:internal/deps/undici/undici:1647:28)
    at appendHeader (node:internal/deps/undici/undici:2053:29)
    at fill (node:internal/deps/undici/undici:2039:11)
    at new Headers (node:internal/deps/undici/undici:2167:11)

> $ node
Welcome to Node.js v20.11.0.
Type ".help" for more information.
> new Headers({ ":authority": "localhost" }); // allowed
HeadersList {
  cookies: null,
  [Symbol(headers map)]: Map(1) { ':authority' => { name: ':authority', value: 'localhost' } },
  [Symbol(headers map sorted)]: null
}

[yusukebe]

I see!

@usualoma Can you see this? I think we have to go with this code, but any thoughts?

[usualoma]

Hi @tsctx
Thanks for creating the PR, it is very informative!

@yusukebe
newRequestFromIncoming() is often not called on simple requests, and key.charCodeAt(0) ! == /*:*/ 0x3a, I do not think the overhead caused by such code is a problem.

[tsctx]

cc @yusukebe

[yusukebe]

Hi @tsctx

Sorry for the delay. I've commented.

[usualoma]

As for get url(), looking at the reference again, I think it would be better to do the following.

const url = `http://${req instanceof Http2ServerRequest ? req.authority : req.headers.host}${req.url}`

https://nodejs.org/api/http2.html#requestauthority
https://nodejs.org/api/http.html#requesthost

this[urlCacheKey]

c.req.url is always called here once in every request, but I think it is often only this once. Therefore, I believe the disadvantages of increased code and overhead due to caching outweigh the advantages.

https://github.com/honojs/hono/blob/95ead59fa6f51fb94477d8a0c28c0c35401c63c0/src/utils/url.ts#L73

Should we always enforce new URL(url).href?

/\.\./ is fast, but it may not be perfect, so some may argue that we should always use new URL(url).href. Nevertheless, this change is not for "http2" and should be discussed separately or at least separate commits.

[usualoma]

Sorry, I was looking in the wrong place, so I modified my comment a bit.

[tsctx]

cc @usualoma @yusukebe

As for get url(), looking at the reference again, I think it would be better to do the following.

const url = `http://${req instanceof Http2ServerRequest ? req.authority : req.headers.host}${req.url}`

https://nodejs.org/api/http2.html#requestauthority https://nodejs.org/api/http.html#requesthost

Thanks for sharing this with me. It should always be so.

this[urlCacheKey]

c.req.url is always called here once in every request, but I think it is often only this once. Therefore, I believe the disadvantages of increased code and overhead due to caching outweigh the advantages.

https://github.com/honojs/hono/blob/95ead59fa6f51fb94477d8a0c28c0c35401c63c0/src/utils/url.ts#L73

No, this should be cached. Because many are called for.

Fixed fae3065

Should we always enforce new URL(url).href?

/\.\./ is fast, but it may not be perfect, so some may argue that we should always use new URL(url).href. Nevertheless, this change is not for "http2" and should be discussed separately or at least separate commits.

IMO we should always do new URL().
IncomingMessage.url does not normalize the url, so the result will be different in the following cases.

// server.mjs
import * as http from 'node:http'

// without `new URL()`
http.createServer((req, res) => res.end(req.url)).listen(3000, '0.0.0.0')

// with `new URL()`
http.createServer((req, res) => res.end(new URL(`http://${req.headers.host}${req.url}`).pathname)).listen(3001, '0.0.0.0')

Result:

> $ curl "http://localhost:3000/path\\"
/path\\
> $ curl "http://localhost:3001/path\\"
/path//

[usualoma]

@tsctx Thank you. I understand.
If new URL().href is always needed, I think it is better to normalize it at the first generation.

diff --git a/src/request.ts b/src/request.ts
index 9c7ec49..74565af 100644
--- a/src/request.ts
+++ b/src/request.ts
@@ -7,12 +7,13 @@ import { Readable } from 'node:stream'
 
 const newRequestFromIncoming = (
   method: string,
+  url: string,
   incoming: IncomingMessage | Http2ServerRequest
 ): Request => {
   const headerRecord: [string, string][] = []
   const rawHeaders = incoming.rawHeaders
   for (let i = 0; i < rawHeaders.length; i += 2) {
-    const {[i]: key, [i + 1]: value} = rawHeaders
+    const { [i]: key, [i + 1]: value } = rawHeaders
     if (key.charCodeAt(0) !== /*:*/ 0x3a) {
       headerRecord.push([key, value])
     }
@@ -30,12 +31,13 @@ const newRequestFromIncoming = (
     ;(init as any).duplex = 'half'
   }
 
-  return new Request(`http://${incoming instanceof Http2ServerRequest ? incoming.authority : incoming.headers.host}${incoming.url}`, init)
+  return new Request(url, init)
 }
 
 const getRequestCache = Symbol('getRequestCache')
 const requestCache = Symbol('requestCache')
 const incomingKey = Symbol('incomingKey')
+const urlKey = Symbol('urlKey')
 
 const requestPrototype: Record<string | symbol, any> = {
   get method() {
@@ -43,13 +45,15 @@ const requestPrototype: Record<string | symbol, any> = {
   },
 
   get url() {
-    if (this[requestCache]) return this[requestCache].url
-    const req = this[incomingKey]
-    return new URL(`http://${req instanceof Http2ServerRequest ? req.authority : req.headers.host}${req.url}`).href
+    return this[urlKey]
   },
 
   [getRequestCache]() {
-    return (this[requestCache] ||= newRequestFromIncoming(this.method, this[incomingKey]))
+    return (this[requestCache] ||= newRequestFromIncoming(
+      this.method,
+      this[urlKey],
+      this[incomingKey]
+    ))
   },
 }
 ;[
@@ -84,5 +88,10 @@ Object.setPrototypeOf(requestPrototype, global.Request.prototype)
 export const newRequest = (incoming: IncomingMessage | Http2ServerRequest) => {
   const req = Object.create(requestPrototype)
   req[incomingKey] = incoming
+  req[urlKey] = new URL(
+    `http://${incoming instanceof Http2ServerRequest ? incoming.authority : incoming.headers.host}${
+      incoming.url
+    }`
+  ).href
   return req
 }

[tsctx]

@yusukebe ptal

[tsctx]

@usualoma Thanks!

[usualoma]

@tsctx Thank you!

[yusukebe]

@tsctx @usualoma

Thanks both. Looks great! Merge now.