Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support http2 #126

Merged
merged 5 commits into from
Jan 23, 2024
Merged

feat: support http2 #126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6e03a6
feat: support http2
tsctx Jan 21, 2024
68dab3c
use `Http2ServerRequest.authority`
tsctx Jan 23, 2024
fae3065
use `requestCache.url` instead of `urlCacheKey`
tsctx Jan 23, 2024
e213fc6
perf: avoid `new URL`
tsctx Jan 23, 2024
988cb87
apply suggestions
tsctx Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Define prototype for lightweight pseudo Request object

import type { IncomingMessage } from 'node:http'
import type { Http2ServerRequest } from 'node:http2'
import { Http2ServerRequest } from 'node:http2'
import { Readable } from 'node:stream'

const newRequestFromIncoming = (
Expand All @@ -11,9 +11,12 @@ const newRequestFromIncoming = (
incoming: IncomingMessage | Http2ServerRequest
): Request => {
const headerRecord: [string, string][] = []
const len = incoming.rawHeaders.length
for (let i = 0; i < len; i += 2) {
headerRecord.push([incoming.rawHeaders[i], incoming.rawHeaders[i + 1]])
const rawHeaders = incoming.rawHeaders
for (let i = 0; i < rawHeaders.length; i += 2) {
const {[i]: key, [i + 1]: value} = rawHeaders
if (key.charCodeAt(0) !== /*:*/ 0x3a) {
headerRecord.push([key, value])
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@tsctx tsctx Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see!

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

const init = {
Expand All @@ -34,15 +37,18 @@ const newRequestFromIncoming = (
const getRequestCache = Symbol('getRequestCache')
const requestCache = Symbol('requestCache')
const incomingKey = Symbol('incomingKey')
const urlCacheKey = Symbol('urlCacheKey')

const requestPrototype: Record<string | symbol, any> = {
get method() {
return this[incomingKey].method || 'GET'
},

get url() {
const url = `http://${this[incomingKey].headers.host}${this[incomingKey].url}`
return /\.\./.test(url) ? new URL(url).href : url
if (this[urlCacheKey]) return this[urlCacheKey]
const req = this[incomingKey]
const url = `http://${req instanceof Http2ServerRequest ? req.headers[':authority'] : req.headers.host}${req.url}`
return (this[urlCacheKey] = new URL(url).href)
},

[getRequestCache]() {
Expand Down
23 changes: 23 additions & 0 deletions test/server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,12 @@ describe('SSL', () => {
describe('HTTP2', () => {
const app = new Hono()
app.get('/', (c) => c.text('Hello! Node!'))
app.get('/headers', (c) => {
// call newRequestFromIncoming
c.req.header('Accept')
return c.text('Hello! Node!')
})
app.get('/url', (c) => c.text(c.req.url))

const server = createAdaptorServer({
fetch: app.fetch,
Expand All @@ -475,6 +481,23 @@ describe('HTTP2', () => {
expect(res.headers['content-type']).toMatch(/text\/plain/)
expect(res.text).toBe('Hello! Node!')
})

it('Should return 200 response - GET /headers', async () => {
// @ts-expect-error: @types/supertest is not updated yet
const res = await request(server, { http2: true }).get('/headers').trustLocalhost()
expect(res.status).toBe(200)
expect(res.headers['content-type']).toMatch(/text\/plain/)
expect(res.text).toBe('Hello! Node!')
})

// Use :authority as the host for the url.
it('Should return 200 response - GET /url', async () => {
// @ts-expect-error: @types/supertest is not updated yet
const res = await request(server, { http2: true }).get('/url').trustLocalhost()
expect(res.status).toBe(200)
expect(res.headers['content-type']).toMatch(/text\/plain/)
expect(new URL(res.text).hostname).toBe('127.0.0.1')
})
})

describe('Hono compression', () => {
Expand Down