Skip to content

chore: make client-h2.js more resilient #4564

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

chore: make client-h2.js more resilient #4564

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 51 additions & 15 deletions lib/dispatcher/client-h2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ const {
} = require('../core/symbols.js')
const { channels } = require('../core/diagnostics.js')

/** @typedef {import('../../types/client.js').default} Client */
/** @typedef {import('http2').ClientHttp2Session} ClientHttp2Session */
/** @typedef {import('node:net').Socket} Socket */

const kOpenStreams = Symbol('open streams')

let extractBody
Expand Down Expand Up @@ -77,6 +81,10 @@ function parseH2Headers (headers) {
return result
}

/**
* @param {Client} client
* @param {Socket} socket
*/
function connectH2 (client, socket) {
client[kSocket] = socket

Expand Down Expand Up @@ -151,13 +159,23 @@ function resumeH2 (client) {
}
}

/**
* @this {ClientHttp2Session}
* @param {Error} err
*/
function onHttp2SessionError (err) {
assert(err.code !== 'ERR_TLS_CERT_ALTNAME_INVALID')

this[kSocket][kError] = err
this[kClient][kOnError](err)
}

/**
* @this {ClientHttp2Session}
* @param {number} type
* @param {number} code
* @param {number} id
*/
function onHttp2FrameError (type, code, id) {
if (id === 0) {
const err = new InformationalError(`HTTP/2: "frameError" received - type ${type}, code ${code}`)
Expand All @@ -166,6 +184,7 @@ function onHttp2FrameError (type, code, id) {
}
}

/** @this {ClientHttp2Session} */
function onHttp2SessionEnd () {
const err = new SocketError('other side closed', util.getSocketInfo(this[kSocket]))
this.destroy(err)
Expand All @@ -177,23 +196,19 @@ function onHttp2SessionEnd () {
* We need to handle GOAWAY frames properly, and trigger the session close
* along with the socket right away
*
* @this {import('http2').ClientHttp2Session}
* @this {ClientHttp2Session}
* @param {number} errorCode
* @param {number} lastStreamID
* @param {Buffer} opaqueData
*/
function onHttp2SessionGoAway (errorCode) {
function onHttp2SessionGoAway (errorCode, lastStreamID, opaqueData) {
// TODO(mcollina): Verify if GOAWAY implements the spec correctly:
// https://datatracker.ietf.org/doc/html/rfc7540#section-6.8
// Specifically, we do not verify the "valid" stream id.

const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, util.getSocketInfo(this[kSocket]))
const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] && util.getSocketInfo(this[kSocket]))
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

The null check this[kSocket] && will pass false to util.getSocketInfo() when socket is null/undefined, which may cause unexpected behavior. Consider using a ternary operator: this[kSocket] ? util.getSocketInfo(this[kSocket]) : null

Suggested change
const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] && util.getSocketInfo(this[kSocket]))
const err = this[kError] || new SocketError(`HTTP/2: "GOAWAY" frame received with code ${errorCode}`, this[kSocket] ? util.getSocketInfo(this[kSocket]) : null)

Copilot uses AI. Check for mistakes.

const client = this[kClient]

client[kSocket] = null
client[kHTTPContext] = null

// this is an HTTP2 session
this.close()
this[kHTTP2Session] = null

util.destroy(this[kSocket], err)

Expand All @@ -213,9 +228,10 @@ function onHttp2SessionGoAway (errorCode) {
client[kResume]()
}

/** @this {ClientHttp2Session} */
function onHttp2SessionClose () {
const { [kClient]: client } = this
const { [kSocket]: socket } = client
const client = this[kClient]
const socket = client[kSocket]

const err = this[kSocket][kError] || this[kError] || new SocketError('closed', util.getSocketInfo(socket))

Expand All @@ -234,6 +250,7 @@ function onHttp2SessionClose () {
}
}

/** @this {Socket} */
function onHttp2SocketClose () {
const err = this[kError] || new SocketError('closed', util.getSocketInfo(this))

Expand All @@ -255,6 +272,10 @@ function onHttp2SocketClose () {
client[kResume]()
}

/**
* @this {Socket}
* @param {Error} err
*/
function onHttp2SocketError (err) {
assert(err.code !== 'ERR_TLS_CERT_ALTNAME_INVALID')

Expand All @@ -263,15 +284,30 @@ function onHttp2SocketError (err) {
this[kClient][kOnError](err)
}

/** @this {Socket} */
function onHttp2SocketEnd () {
util.destroy(this, new SocketError('other side closed', util.getSocketInfo(this)))
}

/** @this {Socket} */
function onSocketClose () {
this[kClosed] = true
}

// https://www.rfc-editor.org/rfc/rfc7230#section-3.3.2
/**
* @overload
* @param {'GET' | 'HEAD' | 'OPTIONS' | 'TRACE' | 'CONNECT'} method
* @returns {false}
*//**
* @overload
* @param {Uppercase<string>} method
* @returns {true}
*//**
* @param {Uppercase<string>} method
* @returns {boolean}
*
* @see https://www.rfc-editor.org/rfc/rfc7230#section-3.3.2
*/
function shouldSendContentLength (method) {
return method !== 'GET' && method !== 'HEAD' && method !== 'OPTIONS' && method !== 'TRACE' && method !== 'CONNECT'
}
Expand Down Expand Up @@ -318,7 +354,7 @@ function writeH2 (client, request) {
}

/** @type {import('node:http2').ClientHttp2Stream} */
let stream = null
let stream
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

[nitpick] Variable stream is declared without initialization. Consider initializing it explicitly as let stream = undefined for clarity, or use the original null initialization if that was intentional.

Suggested change
let stream
let stream = undefined

Copilot uses AI. Check for mistakes.


const { hostname, port } = client[kUrl]

Expand All @@ -334,7 +370,7 @@ function writeH2 (client, request) {

util.errorRequest(client, request, err)

if (stream != null) {
if (stream !== undefined) {
// Some chunks might still come after abort,
// let's ignore them
stream.removeAllListeners('data')
Expand Down Expand Up @@ -371,7 +407,7 @@ function writeH2 (client, request) {
// `ready` event is triggered
// We disabled endStream to allow the user to write to the stream
stream = session.request(headers, { endStream: false, signal })
stream.on('response', headers => {
stream.on('response', (headers, flags, rawHeaders) => {
const { [HTTP2_HEADER_STATUS]: statusCode, ...realHeaders } = headers

request.onUpgrade(statusCode, parseH2Headers(realHeaders), stream)
Expand Down
Loading