From c635305ac120d95b151c8071973ca6a91bfe1c91 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 12 Apr 2023 14:01:45 +0200 Subject: [PATCH 01/55] feat: port H2 work with latest main --- lib/client.js | 248 ++++++++++++++++++++++++++++++++++++++------ lib/core/connect.js | 2 + lib/core/symbols.js | 4 +- 3 files changed, 224 insertions(+), 30 deletions(-) diff --git a/lib/client.js b/lib/client.js index 7d9ec8d7c27..6347a99ccfa 100644 --- a/lib/client.js +++ b/lib/client.js @@ -6,6 +6,7 @@ const assert = require('assert') const net = require('net') +const http2 = require('http2') const util = require('./core/util') const timers = require('./timers') const Request = require('./core/request') @@ -67,7 +68,10 @@ const { kDispatch, kInterceptors, kLocalAddress, - kMaxResponseSize + kMaxResponseSize, + // HTTP2 + kHost, + kHTTP2Session, } = require('./core/symbols') const FastBuffer = Buffer[Symbol.species] @@ -241,6 +245,10 @@ class Client extends DispatcherBase { this[kClosedResolve] = null this[kMaxResponseSize] = maxResponseSize > -1 ? maxResponseSize : -1 + // HTTP/2 + this[kHTTP2Session] = null + this[kHost] = `${this[kUrl].hostname}${this[kUrl].port ? `:${this[kUrl].port}` : ''}` + // kQueue is built up of 3 sections separated by // the kRunningIdx and kPendingIdx indices. // | complete | running | pending | @@ -356,6 +364,15 @@ class Client extends DispatcherBase { } } + +function onHttp2SessionError (err) { + assert(err.code !== 'ERR_TLS_CERT_ALTNAME_INVALID') + + this[kError] = err + + onError(this[kClient], err) +} + const constants = require('./llhttp/constants') const createRedirectInterceptor = require('./interceptor/redirectInterceptor') const EMPTY_BUF = Buffer.alloc(0) @@ -998,14 +1015,16 @@ function onSocketEnd () { function onSocketClose () { const { [kClient]: client } = this - if (!this[kError] && this[kParser].statusCode && !this[kParser].shouldKeepAlive) { - // We treat all incoming data so far as a valid response. - this[kParser].onMessageComplete() + if (!this[kHTTP2Session]) { + if (!this[kError] && this[kParser].statusCode && !this[kParser].shouldKeepAlive) { + // We treat all incoming data so far as a valid response. + this[kParser].onMessageComplete() + } + + this[kParser].destroy() + this[kParser] = null } - this[kParser].destroy() - this[kParser] = null - const err = this[kError] || new SocketError('closed', util.getSocketInfo(this)) client[kSocket] = null @@ -1092,29 +1111,43 @@ async function connect (client) { return } - if (!llhttpInstance) { - llhttpInstance = await llhttpPromise - llhttpPromise = null - } - client[kConnecting] = false assert(socket) - socket[kNoRef] = false - socket[kWriting] = false - socket[kReset] = false - socket[kBlocking] = false - socket[kError] = null - socket[kParser] = new Parser(client, socket, llhttpInstance) - socket[kClient] = client - socket[kCounter] = 0 - socket[kMaxRequests] = client[kMaxRequests] - socket - .on('error', onSocketError) - .on('readable', onSocketReadable) - .on('end', onSocketEnd) - .on('close', onSocketClose) + if (socket.alpnProtocol === 'h2') { + const session = http2.connect(client[kUrl], { + createConnection: () => socket + }) + + session[kError] = null + session[kClient] = client + session.on('error', onHttp2SessionError) + session.on('close', onSocketClose) + session.unref() + + client[kHTTP2Session] = session + } else { + if (!llhttpInstance) { + llhttpInstance = await llhttpPromise + llhttpPromise = null + } + + socket[kNoRef] = false + socket[kWriting] = false + socket[kReset] = false + socket[kBlocking] = false + socket[kError] = null + socket[kParser] = new Parser(client, socket, llhttpInstance) + socket[kClient] = client + socket[kCounter] = 0 + socket[kMaxRequests] = client[kMaxRequests] + socket + .on('error', onSocketError) + .on('readable', onSocketReadable) + .on('end', onSocketEnd) + .on('close', onSocketClose) + } client[kSocket] = socket @@ -1208,7 +1241,7 @@ function _resume (client, sync) { const socket = client[kSocket] - if (socket && !socket.destroyed) { + if (socket && !socket.destroyed && socket.alpnProtocol !== 'h2') { if (client[kSize] === 0) { if (!socket[kNoRef] && socket.unref) { socket.unref() @@ -1273,12 +1306,14 @@ function _resume (client, sync) { return } - if (!socket) { + if (!socket && !client[kHTTP2Session]) { connect(client) return } - if (socket.destroyed || socket[kWriting] || socket[kReset] || socket[kBlocking]) { + if ((socket.destroyed || socket[kWriting] || socket[kReset] || socket[kBlocking]) || + (client[kHTTP2Session] && client[kHTTP2Session].destroyed)) { + // TODO(HTTP/2): what if exceeds max concurrent streams or can't accept new return } @@ -1334,6 +1369,12 @@ function _resume (client, sync) { } function write (client, request) { + if (client[kHTTP2Session]) { + console.log('http/2') + writeH2(client, client[kHTTP2Session], request) + return + } + const { body, method, path, host, upgrade, headers, blocking, reset } = request // https://tools.ietf.org/html/rfc7231#section-4.3.1 @@ -1489,6 +1530,155 @@ function write (client, request) { return true } +function writeH2 (client, session, request) { + // TODO(HTTP/2): upgrade is not supported in HTTP/2 + const { body, method, path, host, upgrade } = request + + // https://tools.ietf.org/html/rfc7231#section-4.3.1 + // https://tools.ietf.org/html/rfc7231#section-4.3.2 + // https://tools.ietf.org/html/rfc7231#section-4.3.5 + + // Sending a payload body on a request that does not + // expect it can cause undefined behavior on some + // servers and corrupt connection state. Do not + // re-use the connection for further requests. + + + const expectsPayload = ( + method === 'PUT' || + method === 'POST' || + method === 'PATCH' + ) + + if (body && typeof body.read === 'function') { + // Try to read EOF in order to get length. + body.read(0) + } + + let contentLength = util.bodyLength(body) + + if (contentLength == null) { + contentLength = request.contentLength + } + + if (contentLength === 0 || !expectsPayload) { + // https://tools.ietf.org/html/rfc7230#section-3.3.2 + // A user agent SHOULD NOT send a Content-Length header field when + // the request message does not contain a payload body and the method + // semantics do not anticipate such a body. + + contentLength = null + } + + if (request.contentLength != null && request.contentLength !== contentLength) { + if (client[kStrictContentLength]) { + errorRequest(client, request, new RequestContentLengthMismatchError()) + return false + } + + process.emitWarning(new RequestContentLengthMismatchError()) + } + + try { + // TODO(HTTP/2): Should we call onConnect immediately or on stream ready event? + request.onConnect((err) => { + if (request.aborted || request.completed) { + return + } + + errorRequest(client, request, err || new RequestAbortedError()) + }) + } catch (err) { + errorRequest(client, request, err) + } + + if (request.aborted) { + return false + } + + const headers = { ...request.headers } + headers[':authority'] = host || client[kHost] + headers[':path'] = path + + // TODO(HTTP/2): Expect: 100-continue + + /* istanbul ignore else: assertion */ + if (!body) { + if (contentLength === 0) { + headers['content-length'] = '0' + } else { + assert(contentLength == null, 'no body must not have content length') + } + } else if (util.isBuffer(body)) { + assert(contentLength === body.byteLength, 'buffer body must have content length') + + headers['content-length'] = String(contentLength) + + process.nextTick(() => { + stream.end(body) + request.onBodySent(body) + }) + } else if (util.isBlob(body)) { + process.nextTick(() => { + writeBlob({ client, request, stream, contentLength, expectsPayload }) + }) + } else if (util.isStream(body)) { + process.nextTick(() => { + writeStream({ client, request, stream, contentLength, expectsPayload }) + }) + } else if (util.isIterable(body)) { + process.nextTick(() => { + writeIterable({ client, request, stream, contentLength, expectsPayload }) + }) + } else { + assert(false) + } + + console.log('http/2 request') + // TODO(HTTP/2): ref only if current streams count is 0 + session.ref() + // TODO(HTTP/2): The handler expects an array but the native http2 module provides us with an object. What should we do? + const stream = session.request(headers) + + stream.on('response', headers => { + if (request.onHeaders(Number(headers[':status']), headers, stream.resume.bind(stream), '') === false) { + stream.pause() + } + }) + + stream.on('data', chunk => { + if (request.onData(chunk) === false) { + stream.pause() + } + }) + + stream.on('trailers', headers => { + // TODO(HTTP/2): Suppor trailers + }) + stream.on('end', () => { + request.onComplete([]) + }) + + stream.on('aborted', () => { + // TODO(HTTP/2): Support aborted + }) + + stream.on('ready', () => { + // TODO(HTTP/2): Support ready + }) + + stream.on('timeout', () => { + // TODO(HTTP/2): Support timeout + }) + + stream.on('close', () => { + // TODO(HTTP/2): unref only if current streams count is 0 + session.unref() + }) + + return true +} + function writeStream ({ body, client, request, socket, contentLength, header, expectsPayload }) { assert(contentLength !== 0 || client[kRunning] === 0, 'stream body cannot be pipelined') diff --git a/lib/core/connect.js b/lib/core/connect.js index f3b5cc33edd..b63786bae7f 100644 --- a/lib/core/connect.js +++ b/lib/core/connect.js @@ -99,6 +99,8 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { servername, session, localAddress, + // TODO(HTTP/2): Should we support h2c? + ALPNProtocols: ['h2', 'http/1.1'], socket: httpSocket, // upgrade socket connection port: port || 443, host: hostname diff --git a/lib/core/symbols.js b/lib/core/symbols.js index c852107a72a..ae96d4efa97 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -51,5 +51,7 @@ module.exports = { kProxy: Symbol('proxy agent options'), kCounter: Symbol('socket request counter'), kInterceptors: Symbol('dispatch interceptors'), - kMaxResponseSize: Symbol('max response size') + kMaxResponseSize: Symbol('max response size'), + kHost: Symbol('host'), + kHTTP2Session: Symbol('http2Session') } From 0a383cec72c7c60bc7faa9a1bd0214b0374055aa Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 19 Apr 2023 10:36:22 +0200 Subject: [PATCH 02/55] fix: linting errors --- lib/client.js | 8 +++----- lib/core/symbols.js | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/client.js b/lib/client.js index 6347a99ccfa..66bd58781ca 100644 --- a/lib/client.js +++ b/lib/client.js @@ -71,7 +71,7 @@ const { kMaxResponseSize, // HTTP2 kHost, - kHTTP2Session, + kHTTP2Session } = require('./core/symbols') const FastBuffer = Buffer[Symbol.species] @@ -364,7 +364,6 @@ class Client extends DispatcherBase { } } - function onHttp2SessionError (err) { assert(err.code !== 'ERR_TLS_CERT_ALTNAME_INVALID') @@ -1020,7 +1019,7 @@ function onSocketClose () { // We treat all incoming data so far as a valid response. this[kParser].onMessageComplete() } - + this[kParser].destroy() this[kParser] = null } @@ -1543,7 +1542,6 @@ function writeH2 (client, session, request) { // servers and corrupt connection state. Do not // re-use the connection for further requests. - const expectsPayload = ( method === 'PUT' || method === 'POST' || @@ -1658,7 +1656,7 @@ function writeH2 (client, session, request) { stream.on('end', () => { request.onComplete([]) }) - + stream.on('aborted', () => { // TODO(HTTP/2): Support aborted }) diff --git a/lib/core/symbols.js b/lib/core/symbols.js index ae96d4efa97..79137fc0c53 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -52,6 +52,5 @@ module.exports = { kCounter: Symbol('socket request counter'), kInterceptors: Symbol('dispatch interceptors'), kMaxResponseSize: Symbol('max response size'), - kHost: Symbol('host'), kHTTP2Session: Symbol('http2Session') } From e83ff26530e23349573e53b48929cfb05c5aba3b Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 19 Apr 2023 14:08:25 +0200 Subject: [PATCH 03/55] refactor: adjust support for headers and set testing --- lib/client.js | 8 ++++---- lib/core/request.js | 9 ++++++++- lib/core/symbols.js | 3 ++- lib/core/util.js | 2 ++ 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/lib/client.js b/lib/client.js index 66bd58781ca..eea2cb731ac 100644 --- a/lib/client.js +++ b/lib/client.js @@ -71,7 +71,8 @@ const { kMaxResponseSize, // HTTP2 kHost, - kHTTP2Session + kHTTP2Session, + kRawRequestHeaders } = require('./core/symbols') const FastBuffer = Buffer[Symbol.species] @@ -1014,7 +1015,7 @@ function onSocketEnd () { function onSocketClose () { const { [kClient]: client } = this - if (!this[kHTTP2Session]) { + if (client[kHTTP2Session] == null) { if (!this[kError] && this[kParser].statusCode && !this[kParser].shouldKeepAlive) { // We treat all incoming data so far as a valid response. this[kParser].onMessageComplete() @@ -1594,7 +1595,7 @@ function writeH2 (client, session, request) { return false } - const headers = { ...request.headers } + const headers = { ...request[kRawRequestHeaders] } headers[':authority'] = host || client[kHost] headers[':path'] = path @@ -1635,7 +1636,6 @@ function writeH2 (client, session, request) { console.log('http/2 request') // TODO(HTTP/2): ref only if current streams count is 0 session.ref() - // TODO(HTTP/2): The handler expects an array but the native http2 module provides us with an object. What should we do? const stream = session.request(headers) stream.on('response', headers => { diff --git a/lib/core/request.js b/lib/core/request.js index 6c9a24d5d59..422e1892016 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -5,6 +5,7 @@ const { NotSupportedError } = require('./errors') const assert = require('assert') +const { kRawRequestHeaders } = require('./symbols') const util = require('./util') // tokenRegExp and headerCharRegex have been lifted from @@ -155,7 +156,12 @@ class Request { throw new InvalidArgumentError('headers array must be even') } for (let i = 0; i < headers.length; i += 2) { - processHeader(this, headers[i], headers[i + 1]) + const headerKey = headers[i] + const headerValue = headers[i + 1] + processHeader(this, headerKey, headerValue) + this[kRawRequestHeaders] = { + [headerKey]: headerValue + } } } else if (headers && typeof headers === 'object') { const keys = Object.keys(headers) @@ -163,6 +169,7 @@ class Request { const key = keys[i] processHeader(this, key, headers[key]) } + this[kRawRequestHeaders] = headers } else if (headers != null) { throw new InvalidArgumentError('headers must be an object or an array') } diff --git a/lib/core/symbols.js b/lib/core/symbols.js index 79137fc0c53..b617e7a9330 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -52,5 +52,6 @@ module.exports = { kCounter: Symbol('socket request counter'), kInterceptors: Symbol('dispatch interceptors'), kMaxResponseSize: Symbol('max response size'), - kHTTP2Session: Symbol('http2Session') + kHTTP2Session: Symbol('http2Session'), + kRawRequestHeaders: Symbol('request raw headers') } diff --git a/lib/core/util.js b/lib/core/util.js index 4f8c1f8f1a1..a43530da95d 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -218,6 +218,8 @@ function parseKeepAliveTimeout (val) { } function parseHeaders (headers, obj = {}) { + // For H2 support + if (typeof headers === 'object') return headers for (let i = 0; i < headers.length; i += 2) { const key = headers[i].toString().toLowerCase() let val = obj[key] From 4360f9780ba867ce153ec17b3a23895de9809b3b Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 21 Apr 2023 10:58:16 +0200 Subject: [PATCH 04/55] test: add testing for h2 --- lib/core/util.js | 13 ++++++++-- test/http2.js | 64 +++++++++++++++++++++++++++++++++--------------- 2 files changed, 55 insertions(+), 22 deletions(-) diff --git a/lib/core/util.js b/lib/core/util.js index a43530da95d..287b76bde95 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -5,7 +5,7 @@ const { kDestroyed, kBodyUsed } = require('./symbols') const { IncomingMessage } = require('http') const stream = require('stream') const net = require('net') -const { InvalidArgumentError } = require('./errors') +const { InvalidArgumentError, ClientDestroyedError } = require('./errors') const { Blob } = require('buffer') const nodeUtil = require('util') const { stringify } = require('querystring') @@ -199,7 +199,16 @@ function destroy (stream, err) { // See: https://github.com/nodejs/node/pull/38505/files stream.socket = null } - stream.destroy(err) + + // HTTP/2 - It causes to throw uncaught exceptions due to the + // error passed down the socket.destroy + if (stream.alpnProtocol === 'h2' && + Object.getPrototypeOf(err).constructor === ClientDestroyedError + ) { + stream.destroy() + } else { + stream.destroy(err) + } } else if (err) { process.nextTick((stream, err) => { stream.emit('error', err) diff --git a/test/http2.js b/test/http2.js index ab8752a7816..840fd6b2252 100644 --- a/test/http2.js +++ b/test/http2.js @@ -1,32 +1,56 @@ 'use strict' +const { createSecureServer } = require('node:http2') +const { once } = require('node:events') + const { test } = require('tap') -const { Client, errors } = require('..') -const { createSecureServer } = require('http2') const pem = require('https-pem') -test('throw http2 not supported error', (t) => { - t.plan(1) +const { Client } = require('..') + +test('Should support H2 connection', async t => { + const body = [] + const server = createSecureServer(pem) + + server.on('stream', (stream, headers) => { + t.equal(headers['x-my-header'], 'foo') + t.equal(headers[':method'], 'GET') + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': 'hello', + ':status': 200 + }) + stream.end('hello h2!') + }) + + server.listen(0) + await once(server, 'listening') - const server = createSecureServer({ key: pem.key, cert: pem.cert }, (req, res) => { - res.stream.respond({ 'content-type': 'text/plain' }) - res.stream.end('hello') - }).on('unknownProtocol', (socket) => { - // continue sending data in http2 to our http1.1 client to trigger error - socket.write('PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n') + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } }) + + t.plan(6) t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) - server.listen(0, () => { - const client = new Client(`https://localhost:${server.address().port}`, { - tls: { - rejectUnauthorized: false - } - }) - t.teardown(client.close.bind(client)) + const response = await client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) - client.request({ path: '/', method: 'GET' }, (err, data) => { - t.type(err, errors.HTTPParserError) - }) + response.body.on('data', chunk => { + body.push(chunk) }) + + await once(response.body, 'end') + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'hello') + t.equal(Buffer.concat(body).toString('utf8'), 'hello h2!') }) From 385a5406ac3843f8f4ec8bad9d3b6ba3b4791eee Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 26 Apr 2023 10:14:13 +0200 Subject: [PATCH 05/55] refactor: make http2 session handle shorter --- lib/core/util.js | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lib/core/util.js b/lib/core/util.js index 287b76bde95..3a54a356836 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -202,13 +202,10 @@ function destroy (stream, err) { // HTTP/2 - It causes to throw uncaught exceptions due to the // error passed down the socket.destroy - if (stream.alpnProtocol === 'h2' && + const isHTTP2 = stream.alpnProtocol === 'h2' && Object.getPrototypeOf(err).constructor === ClientDestroyedError - ) { - stream.destroy() - } else { - stream.destroy(err) - } + + stream.destroy(!isHTTP2 ? err : null) } else if (err) { process.nextTick((stream, err) => { stream.emit('error', err) From 4da1170506ae890423550a453e8b741fe64b4439 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 3 May 2023 10:47:34 +0200 Subject: [PATCH 06/55] feat: add support for sending body over http2 --- lib/client.js | 106 +++++++++++++++++++++++++++++++++++++------------- test/http2.js | 66 ++++++++++++++++++++++++++++++- 2 files changed, 144 insertions(+), 28 deletions(-) diff --git a/lib/client.js b/lib/client.js index eea2cb731ac..cd8c5ce00c9 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1598,39 +1598,91 @@ function writeH2 (client, session, request) { const headers = { ...request[kRawRequestHeaders] } headers[':authority'] = host || client[kHost] headers[':path'] = path + headers[':method'] = method // TODO(HTTP/2): Expect: 100-continue + session.ref() + let stream + if (expectContinue) { + headers.Expect = '100-continue' + /** + * @type {import('node:http2').ClientHttp2Stream} + */ + stream = session.request(headers) + + stream.on('continue', () => { + // TODO[H2]: Shall we trigger an event when 100-continue operations + console.log('this is the body:', body) + /* istanbul ignore else: assertion */ + if (!body) { + if (contentLength === 0) { + headers['content-length'] = '0' + } else { + assert(contentLength == null, 'no body must not have content length') + stream.write('\r\b') + } - /* istanbul ignore else: assertion */ - if (!body) { - if (contentLength === 0) { - headers['content-length'] = '0' - } else { - assert(contentLength == null, 'no body must not have content length') - } - } else if (util.isBuffer(body)) { - assert(contentLength === body.byteLength, 'buffer body must have content length') + request.onRequestSent() + } else if (util.isBuffer(body)) { + console.log('as buffer') + assert(contentLength === body.byteLength, 'buffer body must have content length') + + headers['content-length'] = `${contentLength}` + stream.end(body) + request.onBodySent(body) + request.onRequestSent() + console.log('ended?') + } else if (util.isBlob(body)) { + console.log('as blob') + if (typeof body.stream === 'function') { + writeIterable({ body: body.stream(), client, request, socket: client[kSocket], contentLength, expectsPayload }) + } else { + writeBlob({ body, client, request, socket: client[kSocket], contentLength, header, expectsPayload }) + } + } else if (util.isStream(body)) { + writeStream({ client, request, socket: client[kSocket], contentLength, expectsPayload }) + } else if (util.isIterable(body)) { + writeIterable({ client, request, socket: client[kSocket], contentLength, expectsPayload }) + } else { + assert(false) + } + }) + } else { + /** @type {import('node:http2').ClientHttp2Stream} */ + stream = session.request(headers) + /* istanbul ignore else: assertion */ + if (!body) { + if (contentLength === 0) { + headers['content-length'] = '0' + } else { + assert(contentLength == null, 'no body must not have content length') + } - headers['content-length'] = String(contentLength) + request.onRequestSent() + } else if (util.isBuffer(body)) { + console.log('as buffer') + assert(contentLength === body.byteLength, 'buffer body must have content length') - process.nextTick(() => { - stream.end(body) + headers['content-length'] = `${contentLength}` + console.log('stream:', stream.write) + stream.write(body) request.onBodySent(body) - }) - } else if (util.isBlob(body)) { - process.nextTick(() => { - writeBlob({ client, request, stream, contentLength, expectsPayload }) - }) - } else if (util.isStream(body)) { - process.nextTick(() => { - writeStream({ client, request, stream, contentLength, expectsPayload }) - }) - } else if (util.isIterable(body)) { - process.nextTick(() => { - writeIterable({ client, request, stream, contentLength, expectsPayload }) - }) - } else { - assert(false) + request.onRequestSent() + console.log('ended?') + } else if (util.isBlob(body)) { + console.log('as blob') + if (typeof body.stream === 'function') { + writeIterable({ body: body.stream(), client, request, socket: client[kSocket], contentLength, expectsPayload }) + } else { + writeBlob({ body, client, request, socket: client[kSocket], contentLength, header, expectsPayload }) + } + } else if (util.isStream(body)) { + writeStream({ client, request, socket: client[kSocket], contentLength, expectsPayload }) + } else if (util.isIterable(body)) { + writeIterable({ client, request, socket: client[kSocket], contentLength, expectsPayload }) + } else { + assert(false) + } } console.log('http/2 request') diff --git a/test/http2.js b/test/http2.js index 840fd6b2252..337367d55d6 100644 --- a/test/http2.js +++ b/test/http2.js @@ -3,11 +3,13 @@ const { createSecureServer } = require('node:http2') const { once } = require('node:events') -const { test } = require('tap') +const { test, plan } = require('tap') const pem = require('https-pem') const { Client } = require('..') +plan(2) + test('Should support H2 connection', async t => { const body = [] const server = createSecureServer(pem) @@ -54,3 +56,65 @@ test('Should support H2 connection', async t => { t.equal(response.headers['x-custom-h2'], 'hello') t.equal(Buffer.concat(body).toString('utf8'), 'hello h2!') }) + +test('Should handle h2 request with body (string or buffer)', async t => { + const server = createSecureServer(pem) + const responseBody = [] + const requestBody = [] + + // server.on('checkContinue', (request, response) => { + // t.equal(request.headers['x-my-header'], 'foo') + // t.equal(request.headers[':method'], 'POST') + + // console.log(request) + + // response.writeContinue() + // }) + + server.on('stream', async (stream, headers) => { + stream.on('data', chunk => requestBody.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': 'hello', + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(5) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo' + }, + body: 'hello from client!' + // expectContinue: true + }) + + response.body.on('data', chunk => { + responseBody.push(chunk) + }) + + await once(response.body, 'end') + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'hello') + t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') + t.equal(Buffer.concat(requestBody).toString('utf-8'), 'hello from client!') +}) From 690194e993ee8b8713f7b322edf36a740b804315 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 10 May 2023 13:33:46 +0200 Subject: [PATCH 07/55] feat: ensure support for streams over H2 --- lib/client.js | 31 +++++++++++-- test/http2.js | 117 ++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 136 insertions(+), 12 deletions(-) diff --git a/lib/client.js b/lib/client.js index cd8c5ce00c9..bba8d2885a0 100644 --- a/lib/client.js +++ b/lib/client.js @@ -7,6 +7,7 @@ const assert = require('assert') const net = require('net') const http2 = require('http2') +const { pipeline } = require('stream') const util = require('./core/util') const timers = require('./timers') const Request = require('./core/request') @@ -1656,6 +1657,8 @@ function writeH2 (client, session, request) { headers['content-length'] = '0' } else { assert(contentLength == null, 'no body must not have content length') + // Indicate no more data will be written + stream.end() } request.onRequestSent() @@ -1664,8 +1667,7 @@ function writeH2 (client, session, request) { assert(contentLength === body.byteLength, 'buffer body must have content length') headers['content-length'] = `${contentLength}` - console.log('stream:', stream.write) - stream.write(body) + stream.end(body) request.onBodySent(body) request.onRequestSent() console.log('ended?') @@ -1677,7 +1679,30 @@ function writeH2 (client, session, request) { writeBlob({ body, client, request, socket: client[kSocket], contentLength, header, expectsPayload }) } } else if (util.isStream(body)) { - writeStream({ client, request, socket: client[kSocket], contentLength, expectsPayload }) + // For HTTP/2, is enough to pipe the stream + // TODO: adapt the HTTP2Stream to API compatible with current undici + // expectations from a stream + // writeStream({ body, client, request, socket: client[kSocket], contentLength, expectsPayload }) + const pipe = pipeline( + body, + stream, + (err) => { + if (err) { + util.destroy(body, err) + } + } + ) + pipe.on('data', onPipeData) + pipe.once('end', () => { + pipe.removeListener('data', onPipeData) + util.destroy(pipe) + request.onRequestSent() + }) + body.once('end', () => util.destroy(body)) + + function onPipeData (chunk) { + request.onBodySent(chunk) + } } else if (util.isIterable(body)) { writeIterable({ client, request, socket: client[kSocket], contentLength, expectsPayload }) } else { diff --git a/test/http2.js b/test/http2.js index 337367d55d6..32cf642e4f8 100644 --- a/test/http2.js +++ b/test/http2.js @@ -1,6 +1,7 @@ 'use strict' const { createSecureServer } = require('node:http2') +const { createReadStream, readFileSync } = require('node:fs') const { once } = require('node:events') const { test, plan } = require('tap') @@ -8,7 +9,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(2) +plan(3) test('Should support H2 connection', async t => { const body = [] @@ -59,8 +60,11 @@ test('Should support H2 connection', async t => { test('Should handle h2 request with body (string or buffer)', async t => { const server = createSecureServer(pem) - const responseBody = [] - const requestBody = [] + const responseBody1 = [] + const responseBody2 = [] + const requestBodyString = [] + const requestBodyBuffer = [] + let reqCounter = 0 // server.on('checkContinue', (request, response) => { // t.equal(request.headers['x-my-header'], 'foo') @@ -72,18 +76,23 @@ test('Should handle h2 request with body (string or buffer)', async t => { // }) server.on('stream', async (stream, headers) => { - stream.on('data', chunk => requestBody.push(chunk)) + reqCounter++ + if (reqCounter === 1) { + stream.on('data', chunk => requestBodyString.push(chunk)) + } else { + stream.on('data', chunk => requestBodyBuffer.push(chunk)) + } stream.respond({ 'content-type': 'text/plain; charset=utf-8', - 'x-custom-h2': 'hello', + 'x-custom-h2': headers['x-my-header'], ':status': 200 }) stream.end('hello h2!') }) - t.plan(5) + t.plan(10) server.listen(0) await once(server, 'listening') @@ -97,7 +106,7 @@ test('Should handle h2 request with body (string or buffer)', async t => { t.teardown(server.close.bind(server)) t.teardown(client.close.bind(client)) - const response = await client.request({ + const response1 = await client.request({ path: '/', method: 'POST', headers: { @@ -107,14 +116,104 @@ test('Should handle h2 request with body (string or buffer)', async t => { // expectContinue: true }) + response1.body.on('data', chunk => { + responseBody1.push(chunk) + }) + + await once(response1.body, 'end') + + const response2 = await client.request({ + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo' + }, + body: Buffer.from('hello from client!', 'utf-8') + // expectContinue: true + }) + + response2.body.on('data', chunk => { + responseBody2.push(chunk) + }) + + await once(response2.body, 'end') + + t.equal(response1.statusCode, 200) + t.equal(response1.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response1.headers['x-custom-h2'], 'foo') + t.equal(Buffer.concat(responseBody1).toString('utf-8'), 'hello h2!') + t.equal( + Buffer.concat(requestBodyString).toString('utf-8'), + 'hello from client!' + ) + + t.equal(response2.statusCode, 200) + t.equal(response2.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response2.headers['x-custom-h2'], 'foo') + t.equal(Buffer.concat(responseBody2).toString('utf-8'), 'hello h2!') + t.equal( + Buffer.concat(requestBodyBuffer).toString('utf-8'), + 'hello from client!' + ) +}) + +test('Should handle h2 request with body (stream)', async t => { + const server = createSecureServer(pem) + const expectedBody = readFileSync(__filename, 'utf-8') + const stream = createReadStream(__filename) + const requestChunks = [] + const responseBody = [] + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'PUT') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + console.log('request received and valid') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'PUT', + headers: { + 'x-my-header': 'foo' + }, + body: stream + }) + response.body.on('data', chunk => { responseBody.push(chunk) }) await once(response.body, 'end') + t.equal(response.statusCode, 200) t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') - t.equal(response.headers['x-custom-h2'], 'hello') + t.equal(response.headers['x-custom-h2'], 'foo') t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') - t.equal(Buffer.concat(requestBody).toString('utf-8'), 'hello from client!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) }) From 5c3825ec0f92c43d18b276f559b912f16e20b4d5 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 10 May 2023 13:34:35 +0200 Subject: [PATCH 08/55] refactor: remove noisy logs --- lib/api/api-request.js | 2 +- lib/client.js | 26 +++++++++++++------------- test/http2.js | 2 -- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/lib/api/api-request.js b/lib/api/api-request.js index 71d7e926b4c..df636ca561c 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -95,7 +95,7 @@ class RequestHandler extends AsyncResource { this.callback = null this.res = body - + // console.log('called', callback !== null) if (callback !== null) { if (this.throwOnError && statusCode >= 400) { this.runInAsyncScope(getResolveErrorBodyCallback, null, diff --git a/lib/client.js b/lib/client.js index bba8d2885a0..aca53e82e1d 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1116,6 +1116,7 @@ async function connect (client) { assert(socket) + // console.log(socket) if (socket.alpnProtocol === 'h2') { const session = http2.connect(client[kUrl], { createConnection: () => socket @@ -1371,7 +1372,7 @@ function _resume (client, sync) { function write (client, request) { if (client[kHTTP2Session]) { - console.log('http/2') + // console.log('http/2') writeH2(client, client[kHTTP2Session], request) return } @@ -1613,7 +1614,7 @@ function writeH2 (client, session, request) { stream.on('continue', () => { // TODO[H2]: Shall we trigger an event when 100-continue operations - console.log('this is the body:', body) + // console.log('this is the body:', body) /* istanbul ignore else: assertion */ if (!body) { if (contentLength === 0) { @@ -1625,16 +1626,16 @@ function writeH2 (client, session, request) { request.onRequestSent() } else if (util.isBuffer(body)) { - console.log('as buffer') + // console.log('as buffer') assert(contentLength === body.byteLength, 'buffer body must have content length') headers['content-length'] = `${contentLength}` stream.end(body) request.onBodySent(body) request.onRequestSent() - console.log('ended?') + // console.log('ended?') } else if (util.isBlob(body)) { - console.log('as blob') + // console.log('as blob') if (typeof body.stream === 'function') { writeIterable({ body: body.stream(), client, request, socket: client[kSocket], contentLength, expectsPayload }) } else { @@ -1663,16 +1664,15 @@ function writeH2 (client, session, request) { request.onRequestSent() } else if (util.isBuffer(body)) { - console.log('as buffer') + // console.log('as buffer') assert(contentLength === body.byteLength, 'buffer body must have content length') headers['content-length'] = `${contentLength}` stream.end(body) request.onBodySent(body) request.onRequestSent() - console.log('ended?') - } else if (util.isBlob(body)) { - console.log('as blob') + } else if (util.isBlobLike(body)) { + // console.log('as blob') if (typeof body.stream === 'function') { writeIterable({ body: body.stream(), client, request, socket: client[kSocket], contentLength, expectsPayload }) } else { @@ -1710,12 +1710,11 @@ function writeH2 (client, session, request) { } } - console.log('http/2 request') - // TODO(HTTP/2): ref only if current streams count is 0 - session.ref() - const stream = session.request(headers) + // console.log('http/2 request') + // const stream = session.request(headers) stream.on('response', headers => { + // console.log('h2 response:', headers) if (request.onHeaders(Number(headers[':status']), headers, stream.resume.bind(stream), '') === false) { stream.pause() } @@ -1748,6 +1747,7 @@ function writeH2 (client, session, request) { stream.on('close', () => { // TODO(HTTP/2): unref only if current streams count is 0 + // console.log('closed?') session.unref() }) diff --git a/test/http2.js b/test/http2.js index 32cf642e4f8..15ee9ca227b 100644 --- a/test/http2.js +++ b/test/http2.js @@ -169,8 +169,6 @@ test('Should handle h2 request with body (stream)', async t => { t.equal(headers[':path'], '/') t.equal(headers[':scheme'], 'https') - console.log('request received and valid') - stream.on('data', chunk => requestChunks.push(chunk)) stream.respond({ From 73ab2194568b9c66e0aaf953390f044b8aec69bb Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 12 May 2023 11:14:13 +0200 Subject: [PATCH 09/55] feat: support 100 continue --- lib/client.js | 43 +++++++++++++++++++++++++++------- lib/core/request.js | 9 +++++++- test/http2.js | 56 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 98 insertions(+), 10 deletions(-) diff --git a/lib/client.js b/lib/client.js index aca53e82e1d..74214124d3c 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1534,7 +1534,9 @@ function write (client, request) { function writeH2 (client, session, request) { // TODO(HTTP/2): upgrade is not supported in HTTP/2 - const { body, method, path, host, upgrade } = request + const { body, method, path, host, upgrade, expectContinue } = request + + assert(!upgrade, 'Upgrade not supported for H2') // https://tools.ietf.org/html/rfc7231#section-4.3.1 // https://tools.ietf.org/html/rfc7231#section-4.3.2 @@ -1612,16 +1614,19 @@ function writeH2 (client, session, request) { */ stream = session.request(headers) - stream.on('continue', () => { + stream.once('continue', () => { // TODO[H2]: Shall we trigger an event when 100-continue operations // console.log('this is the body:', body) + /** @type {import('node:http2').ClientHttp2Stream} */ + stream = session.request(headers) /* istanbul ignore else: assertion */ if (!body) { if (contentLength === 0) { headers['content-length'] = '0' } else { assert(contentLength == null, 'no body must not have content length') - stream.write('\r\b') + // Indicate no more data will be written + stream.end() } request.onRequestSent() @@ -1633,8 +1638,7 @@ function writeH2 (client, session, request) { stream.end(body) request.onBodySent(body) request.onRequestSent() - // console.log('ended?') - } else if (util.isBlob(body)) { + } else if (util.isBlobLike(body)) { // console.log('as blob') if (typeof body.stream === 'function') { writeIterable({ body: body.stream(), client, request, socket: client[kSocket], contentLength, expectsPayload }) @@ -1642,9 +1646,32 @@ function writeH2 (client, session, request) { writeBlob({ body, client, request, socket: client[kSocket], contentLength, header, expectsPayload }) } } else if (util.isStream(body)) { - writeStream({ client, request, socket: client[kSocket], contentLength, expectsPayload }) + // For HTTP/2, is enough to pipe the stream + // TODO: adapt the HTTP2Stream to API compatible with current undici + // expectations from a stream + // writeStream({ body, client, request, socket: client[kSocket], contentLength, expectsPayload }) + const pipe = pipeline( + body, + stream, + (err) => { + if (err) { + util.destroy(body, err) + } + } + ) + pipe.on('data', onPipeData) + pipe.once('end', () => { + pipe.removeListener('data', onPipeData) + util.destroy(pipe) + request.onRequestSent() + }) + body.once('end', () => util.destroy(body)) + + function onPipeData (chunk) { + request.onBodySent(chunk) + } } else if (util.isIterable(body)) { - writeIterable({ client, request, socket: client[kSocket], contentLength, expectsPayload }) + writeIterable({ body, client, request, socket: client[kSocket], contentLength, expectsPayload }) } else { assert(false) } @@ -1704,7 +1731,7 @@ function writeH2 (client, session, request) { request.onBodySent(chunk) } } else if (util.isIterable(body)) { - writeIterable({ client, request, socket: client[kSocket], contentLength, expectsPayload }) + writeIterable({ body, client, request, socket: client[kSocket], contentLength, expectsPayload }) } else { assert(false) } diff --git a/lib/core/request.js b/lib/core/request.js index 422e1892016..8d61508a73e 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -63,7 +63,8 @@ class Request { headersTimeout, bodyTimeout, reset, - throwOnError + throwOnError, + expectContinue }, handler) { if (typeof path !== 'string') { throw new InvalidArgumentError('path must be a string') @@ -99,6 +100,10 @@ class Request { throw new InvalidArgumentError('invalid reset') } + if (expectContinue != null && typeof expectContinue !== 'boolean') { + throw new InvalidArgumentError('invalid expectContinue') + } + this.headersTimeout = headersTimeout this.bodyTimeout = bodyTimeout @@ -151,6 +156,8 @@ class Request { this.headers = '' + this.expectContinue = expectContinue != null ? expectContinue : false + if (Array.isArray(headers)) { if (headers.length % 2 !== 0) { throw new InvalidArgumentError('headers array must be even') diff --git a/test/http2.js b/test/http2.js index 15ee9ca227b..7b8a1798838 100644 --- a/test/http2.js +++ b/test/http2.js @@ -9,7 +9,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(3) +plan(4) test('Should support H2 connection', async t => { const body = [] @@ -58,6 +58,60 @@ test('Should support H2 connection', async t => { t.equal(Buffer.concat(body).toString('utf8'), 'hello h2!') }) +test('Should handle h2 continue', async t => { + const requestBody = [] + const server = createSecureServer(pem) + const responseBody = [] + + server.on('request', (request, response) => { + t.equal(request.headers['x-my-header'], 'foo') + t.equal(request.headers[':method'], 'POST') + + request.on('data', chunk => requestBody.push(chunk)) + + response.writeHead(200, { + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': 'foo' + }) + response.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + expectContinue: true + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo' + }, + expectContinue: true + }) + + response.body.on('data', chunk => { + responseBody.push(chunk) + }) + + await once(response.body, 'end') + + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'foo') + t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') +}) + test('Should handle h2 request with body (string or buffer)', async t => { const server = createSecureServer(pem) const responseBody1 = [] From ca5855e9071a70f46f9e832960460e185671a504 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 17 May 2023 13:31:33 +0200 Subject: [PATCH 10/55] feat: support for iterators --- lib/client.js | 36 +++++++++++++++++++---- test/http2.js | 80 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 99 insertions(+), 17 deletions(-) diff --git a/lib/client.js b/lib/client.js index 74214124d3c..d7091ad81eb 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1604,7 +1604,6 @@ function writeH2 (client, session, request) { headers[':path'] = path headers[':method'] = method - // TODO(HTTP/2): Expect: 100-continue session.ref() let stream if (expectContinue) { @@ -1615,7 +1614,7 @@ function writeH2 (client, session, request) { stream = session.request(headers) stream.once('continue', () => { - // TODO[H2]: Shall we trigger an event when 100-continue operations + // TODO[H2]: Shall we trigger an event when 100-continue operations? // console.log('this is the body:', body) /** @type {import('node:http2').ClientHttp2Stream} */ stream = session.request(headers) @@ -1709,7 +1708,6 @@ function writeH2 (client, session, request) { // For HTTP/2, is enough to pipe the stream // TODO: adapt the HTTP2Stream to API compatible with current undici // expectations from a stream - // writeStream({ body, client, request, socket: client[kSocket], contentLength, expectsPayload }) const pipe = pipeline( body, stream, @@ -1731,7 +1729,7 @@ function writeH2 (client, session, request) { request.onBodySent(chunk) } } else if (util.isIterable(body)) { - writeIterable({ body, client, request, socket: client[kSocket], contentLength, expectsPayload }) + writeIterable({ h2stream: stream, body, client, request, socket: client[kSocket], contentLength, expectsPayload }) } else { assert(false) } @@ -1774,7 +1772,6 @@ function writeH2 (client, session, request) { stream.on('close', () => { // TODO(HTTP/2): unref only if current streams count is 0 - // console.log('closed?') session.unref() }) @@ -1892,7 +1889,7 @@ async function writeBlob ({ body, client, request, socket, contentLength, header } } -async function writeIterable ({ body, client, request, socket, contentLength, header, expectsPayload }) { +async function writeIterable ({ h2stream, body, client, request, socket, contentLength, header, expectsPayload }) { assert(contentLength !== 0 || client[kRunning] === 0, 'iterator body cannot be pipelined') let callback = null @@ -1914,6 +1911,33 @@ async function writeIterable ({ body, client, request, socket, contentLength, he } }) + if (socket.alpnProtocol === 'h2') { + h2stream + .on('close', onDrain) + .on('drain', onDrain) + + try { + // It's up to the user to somehow abort the async iterable. + for await (const chunk of body) { + if (socket[kError]) { + throw socket[kError] + } + + if (!h2stream.write(chunk)) { + await waitForDrain() + } + } + } catch (err) { + h2stream.destroy(err) + } finally { + h2stream + .off('close', onDrain) + .off('drain', onDrain) + } + + return + } + socket .on('close', onDrain) .on('drain', onDrain) diff --git a/test/http2.js b/test/http2.js index 7b8a1798838..5849bb0a374 100644 --- a/test/http2.js +++ b/test/http2.js @@ -9,7 +9,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(4) +plan(5) test('Should support H2 connection', async t => { const body = [] @@ -120,15 +120,6 @@ test('Should handle h2 request with body (string or buffer)', async t => { const requestBodyBuffer = [] let reqCounter = 0 - // server.on('checkContinue', (request, response) => { - // t.equal(request.headers['x-my-header'], 'foo') - // t.equal(request.headers[':method'], 'POST') - - // console.log(request) - - // response.writeContinue() - // }) - server.on('stream', async (stream, headers) => { reqCounter++ if (reqCounter === 1) { @@ -183,7 +174,6 @@ test('Should handle h2 request with body (string or buffer)', async t => { 'x-my-header': 'foo' }, body: Buffer.from('hello from client!', 'utf-8') - // expectContinue: true }) response2.body.on('data', chunk => { @@ -269,3 +259,71 @@ test('Should handle h2 request with body (stream)', async t => { t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) }) + +test('Should handle h2 request with body (iterable)', async t => { + const server = createSecureServer(pem) + const expectedBody = 'hello' + const requestChunks = [] + const responseBody = [] + const iterableBody = { + [Symbol.iterator]: function * () { + const end = expectedBody.length - 1 + for (let i = 0; i < end + 1; i++) { + yield expectedBody[i] + } + + return expectedBody[end] + } + } + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'POST') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo' + }, + body: iterableBody + }) + + response.body.on('data', chunk => { + responseBody.push(chunk) + }) + + await once(response.body, 'end') + + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'foo') + t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) +}) From 69e093bc1586df74424fc2693b1f1306ea0aa8a9 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 17 May 2023 13:56:13 +0200 Subject: [PATCH 11/55] feat: add support for Blobs --- lib/client.js | 34 ++++++++++++-- test/http2.js | 126 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 154 insertions(+), 6 deletions(-) diff --git a/lib/client.js b/lib/client.js index d7091ad81eb..0b742ff881c 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1700,9 +1700,27 @@ function writeH2 (client, session, request) { } else if (util.isBlobLike(body)) { // console.log('as blob') if (typeof body.stream === 'function') { - writeIterable({ body: body.stream(), client, request, socket: client[kSocket], contentLength, expectsPayload }) + // @ts-ignore + writeIterable({ + h2stream: stream, + body: body.stream(), + client, + request, + socket: client[kSocket], + contentLength, + expectsPayload + }) } else { - writeBlob({ body, client, request, socket: client[kSocket], contentLength, header, expectsPayload }) + writeBlob({ + h2stream: stream, + body, + client, + request, + socket: client[kSocket], + contentLength, + header, + expectsPayload + }) } } else if (util.isStream(body)) { // For HTTP/2, is enough to pipe the stream @@ -1861,7 +1879,7 @@ function writeStream ({ body, client, request, socket, contentLength, header, ex .on('error', onFinished) } -async function writeBlob ({ body, client, request, socket, contentLength, header, expectsPayload }) { +async function writeBlob ({ h2stream, body, client, request, socket, contentLength, header, expectsPayload }) { assert(contentLength === body.size, 'blob body must have content length') try { @@ -1872,8 +1890,14 @@ async function writeBlob ({ body, client, request, socket, contentLength, header const buffer = Buffer.from(await body.arrayBuffer()) socket.cork() - socket.write(`${header}content-length: ${contentLength}\r\n\r\n`, 'latin1') - socket.write(buffer) + + if (socket.alpnProtocol === 'h2') { + h2stream.write(buffer) + } else { + socket.write(`${header}content-length: ${contentLength}\r\n\r\n`, 'latin1') + socket.write(buffer) + } + socket.uncork() request.onBodySent(buffer) diff --git a/test/http2.js b/test/http2.js index 5849bb0a374..1edcc371d17 100644 --- a/test/http2.js +++ b/test/http2.js @@ -3,13 +3,14 @@ const { createSecureServer } = require('node:http2') const { createReadStream, readFileSync } = require('node:fs') const { once } = require('node:events') +const { Blob } = require('node:buffer') const { test, plan } = require('tap') const pem = require('https-pem') const { Client } = require('..') -plan(5) +plan(7) test('Should support H2 connection', async t => { const body = [] @@ -327,3 +328,126 @@ test('Should handle h2 request with body (iterable)', async t => { t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) }) + +test('Should handle h2 request with body (Blob)', { skip: !Blob }, async t => { + const server = createSecureServer(pem) + const expectedBody = 'asd' + const requestChunks = [] + const responseBody = [] + const body = new Blob(['asd'], { + type: 'application/json' + }) + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'POST') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo' + }, + body + }) + + response.body.on('data', chunk => { + responseBody.push(chunk) + }) + + await once(response.body, 'end') + + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'foo') + t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) +}) + +test('Should handle h2 request with body (Blob:ArrayBuffer)', { skip: !Blob }, async t => { + const server = createSecureServer(pem) + const expectedBody = 'hello' + const requestChunks = [] + const responseBody = [] + const buf = Buffer.from(expectedBody) + const body = new ArrayBuffer(buf.byteLength) + + buf.copy(new Uint8Array(body)) + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'POST') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo' + }, + body + }) + + response.body.on('data', chunk => { + responseBody.push(chunk) + }) + + await once(response.body, 'end') + + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'foo') + t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) +}) From 2c5a5830c3828fb0e0aaed35db358f55043caf67 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 17 May 2023 14:04:28 +0200 Subject: [PATCH 12/55] refactor: adapt contracts to h2 support --- lib/client.js | 148 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 96 insertions(+), 52 deletions(-) diff --git a/lib/client.js b/lib/client.js index 0b742ff881c..9e428640314 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1599,6 +1599,7 @@ function writeH2 (client, session, request) { return false } + // TODO: Content length needs to be calculated at this stage const headers = { ...request[kRawRequestHeaders] } headers[':authority'] = host || client[kHost] headers[':path'] = path @@ -1640,37 +1641,53 @@ function writeH2 (client, session, request) { } else if (util.isBlobLike(body)) { // console.log('as blob') if (typeof body.stream === 'function') { - writeIterable({ body: body.stream(), client, request, socket: client[kSocket], contentLength, expectsPayload }) + // @ts-ignore + writeIterable({ + client, + request, + contentLength, + h2stream: stream, + expectsPayload, + body: body.stream(), + socket: client[kSocket], + header: '' + }) } else { - writeBlob({ body, client, request, socket: client[kSocket], contentLength, header, expectsPayload }) + writeBlob({ + body, + client, + request, + contentLength, + expectsPayload, + h2stream: stream, + header: '', + socket: client[kSocket] + }) } } else if (util.isStream(body)) { - // For HTTP/2, is enough to pipe the stream // TODO: adapt the HTTP2Stream to API compatible with current undici // expectations from a stream - // writeStream({ body, client, request, socket: client[kSocket], contentLength, expectsPayload }) - const pipe = pipeline( + writeStream({ body, - stream, - (err) => { - if (err) { - util.destroy(body, err) - } - } - ) - pipe.on('data', onPipeData) - pipe.once('end', () => { - pipe.removeListener('data', onPipeData) - util.destroy(pipe) - request.onRequestSent() + client, + request, + contentLength, + expectsPayload, + socket: client[kSocket], + h2Stream: stream, + header: '' }) - body.once('end', () => util.destroy(body)) - - function onPipeData (chunk) { - request.onBodySent(chunk) - } } else if (util.isIterable(body)) { - writeIterable({ body, client, request, socket: client[kSocket], contentLength, expectsPayload }) + writeIterable({ + body, + client, + request, + contentLength, + expectsPayload, + header: '', + h2stream: stream, + socket: client[kSocket] + }) } else { assert(false) } @@ -1702,52 +1719,51 @@ function writeH2 (client, session, request) { if (typeof body.stream === 'function') { // @ts-ignore writeIterable({ - h2stream: stream, - body: body.stream(), client, request, - socket: client[kSocket], contentLength, - expectsPayload + h2stream: stream, + expectsPayload, + body: body.stream(), + socket: client[kSocket], + header: '' }) } else { writeBlob({ - h2stream: stream, body, client, request, - socket: client[kSocket], contentLength, - header, - expectsPayload + expectsPayload, + h2stream: stream, + header: '', + socket: client[kSocket] }) } } else if (util.isStream(body)) { - // For HTTP/2, is enough to pipe the stream // TODO: adapt the HTTP2Stream to API compatible with current undici // expectations from a stream - const pipe = pipeline( + writeStream({ body, - stream, - (err) => { - if (err) { - util.destroy(body, err) - } - } - ) - pipe.on('data', onPipeData) - pipe.once('end', () => { - pipe.removeListener('data', onPipeData) - util.destroy(pipe) - request.onRequestSent() + client, + request, + contentLength, + expectsPayload, + socket: client[kSocket], + h2Stream: stream, + header: '' }) - body.once('end', () => util.destroy(body)) - - function onPipeData (chunk) { - request.onBodySent(chunk) - } } else if (util.isIterable(body)) { - writeIterable({ h2stream: stream, body, client, request, socket: client[kSocket], contentLength, expectsPayload }) + writeIterable({ + body, + client, + request, + contentLength, + expectsPayload, + header: '', + h2stream: stream, + socket: client[kSocket] + }) } else { assert(false) } @@ -1796,9 +1812,37 @@ function writeH2 (client, session, request) { return true } -function writeStream ({ body, client, request, socket, contentLength, header, expectsPayload }) { +function writeStream ({ h2Stream, body, client, request, socket, contentLength, header, expectsPayload }) { assert(contentLength !== 0 || client[kRunning] === 0, 'stream body cannot be pipelined') + if (socket.alpnProtocol === 'h2') { + // For HTTP/2, is enough to pipe the stream + const pipe = pipeline( + body, + h2Stream, + (err) => { + if (err) { + util.destroy(body, err) + } + } + ) + + pipe.on('data', onPipeData) + pipe.once('end', () => { + pipe.removeListener('data', onPipeData) + util.destroy(pipe) + request.onRequestSent() + }) + + body.once('end', () => util.destroy(body)) + + function onPipeData (chunk) { + request.onBodySent(chunk) + } + + return + } + let finished = false const writer = new AsyncWriter({ socket, request, contentLength, client, expectsPayload, header }) From 5e4dae1c498b914b4a417a496fbaa0c0b3fe27e5 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 19 May 2023 11:32:40 +0200 Subject: [PATCH 13/55] refactor: cleanup --- lib/client.js | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lib/client.js b/lib/client.js index 9e428640314..519b398f282 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1116,7 +1116,6 @@ async function connect (client) { assert(socket) - // console.log(socket) if (socket.alpnProtocol === 'h2') { const session = http2.connect(client[kUrl], { createConnection: () => socket @@ -1372,7 +1371,6 @@ function _resume (client, sync) { function write (client, request) { if (client[kHTTP2Session]) { - // console.log('http/2') writeH2(client, client[kHTTP2Session], request) return } @@ -1533,7 +1531,6 @@ function write (client, request) { } function writeH2 (client, session, request) { - // TODO(HTTP/2): upgrade is not supported in HTTP/2 const { body, method, path, host, upgrade, expectContinue } = request assert(!upgrade, 'Upgrade not supported for H2') @@ -1707,7 +1704,6 @@ function writeH2 (client, session, request) { request.onRequestSent() } else if (util.isBuffer(body)) { - // console.log('as buffer') assert(contentLength === body.byteLength, 'buffer body must have content length') headers['content-length'] = `${contentLength}` @@ -1715,9 +1711,7 @@ function writeH2 (client, session, request) { request.onBodySent(body) request.onRequestSent() } else if (util.isBlobLike(body)) { - // console.log('as blob') if (typeof body.stream === 'function') { - // @ts-ignore writeIterable({ client, request, @@ -1769,9 +1763,6 @@ function writeH2 (client, session, request) { } } - // console.log('http/2 request') - // const stream = session.request(headers) - stream.on('response', headers => { // console.log('h2 response:', headers) if (request.onHeaders(Number(headers[':status']), headers, stream.resume.bind(stream), '') === false) { From 3b9b6b667a9c6be16d2b46b145185f50f7754aa0 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 19 May 2023 11:33:15 +0200 Subject: [PATCH 14/55] feat: support for content-length --- lib/client.js | 16 +++++++--------- lib/core/connect.js | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/client.js b/lib/client.js index 519b398f282..00faf175926 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1602,6 +1602,11 @@ function writeH2 (client, session, request) { headers[':path'] = path headers[':method'] = method + if (contentLength != null) { + assert(body, 'no body must not have content length') + headers['content-length'] = `${contentLength}` + } + session.ref() let stream if (expectContinue) { @@ -1694,19 +1699,12 @@ function writeH2 (client, session, request) { stream = session.request(headers) /* istanbul ignore else: assertion */ if (!body) { - if (contentLength === 0) { - headers['content-length'] = '0' - } else { - assert(contentLength == null, 'no body must not have content length') - // Indicate no more data will be written - stream.end() - } + // Indicate no more data will be written + stream.end() request.onRequestSent() } else if (util.isBuffer(body)) { assert(contentLength === body.byteLength, 'buffer body must have content length') - - headers['content-length'] = `${contentLength}` stream.end(body) request.onBodySent(body) request.onRequestSent() diff --git a/lib/core/connect.js b/lib/core/connect.js index b63786bae7f..2f42519c866 100644 --- a/lib/core/connect.js +++ b/lib/core/connect.js @@ -99,7 +99,7 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { servername, session, localAddress, - // TODO(HTTP/2): Should we support h2c? + // TODO(HTTP/2): Add support for h2c ALPNProtocols: ['h2', 'http/1.1'], socket: httpSocket, // upgrade socket connection port: port || 443, From 7d27bf6f11425a9868f113ee598bedf08904f482 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 19 May 2023 11:33:38 +0200 Subject: [PATCH 15/55] refactor: body write --- lib/client.js | 83 ++++----------------------------------------------- test/http2.js | 3 +- 2 files changed, 7 insertions(+), 79 deletions(-) diff --git a/lib/client.js b/lib/client.js index 00faf175926..5873af36e39 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1616,87 +1616,14 @@ function writeH2 (client, session, request) { */ stream = session.request(headers) - stream.once('continue', () => { - // TODO[H2]: Shall we trigger an event when 100-continue operations? - // console.log('this is the body:', body) - /** @type {import('node:http2').ClientHttp2Stream} */ - stream = session.request(headers) - /* istanbul ignore else: assertion */ - if (!body) { - if (contentLength === 0) { - headers['content-length'] = '0' - } else { - assert(contentLength == null, 'no body must not have content length') - // Indicate no more data will be written - stream.end() - } - - request.onRequestSent() - } else if (util.isBuffer(body)) { - // console.log('as buffer') - assert(contentLength === body.byteLength, 'buffer body must have content length') - - headers['content-length'] = `${contentLength}` - stream.end(body) - request.onBodySent(body) - request.onRequestSent() - } else if (util.isBlobLike(body)) { - // console.log('as blob') - if (typeof body.stream === 'function') { - // @ts-ignore - writeIterable({ - client, - request, - contentLength, - h2stream: stream, - expectsPayload, - body: body.stream(), - socket: client[kSocket], - header: '' - }) - } else { - writeBlob({ - body, - client, - request, - contentLength, - expectsPayload, - h2stream: stream, - header: '', - socket: client[kSocket] - }) - } - } else if (util.isStream(body)) { - // TODO: adapt the HTTP2Stream to API compatible with current undici - // expectations from a stream - writeStream({ - body, - client, - request, - contentLength, - expectsPayload, - socket: client[kSocket], - h2Stream: stream, - header: '' - }) - } else if (util.isIterable(body)) { - writeIterable({ - body, - client, - request, - contentLength, - expectsPayload, - header: '', - h2stream: stream, - socket: client[kSocket] - }) - } else { - assert(false) - } - }) + stream.once('continue', writeBodyH2) } else { /** @type {import('node:http2').ClientHttp2Stream} */ stream = session.request(headers) + writeBodyH2() + } + + function writeBodyH2 () { /* istanbul ignore else: assertion */ if (!body) { // Indicate no more data will be written diff --git a/test/http2.js b/test/http2.js index 1edcc371d17..2ec8ad2ba29 100644 --- a/test/http2.js +++ b/test/http2.js @@ -65,6 +65,7 @@ test('Should handle h2 continue', async t => { const responseBody = [] server.on('request', (request, response) => { + t.equal(request.headers.expect, '100-continue') t.equal(request.headers['x-my-header'], 'foo') t.equal(request.headers[':method'], 'POST') @@ -77,7 +78,7 @@ test('Should handle h2 continue', async t => { response.end('hello h2!') }) - t.plan(8) + t.plan(7) server.listen(0) await once(server, 'listening') From 986d5eac7abf88de507edb7a785a8132b7cde550 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 24 May 2023 10:19:06 +0200 Subject: [PATCH 16/55] test: refactor check continue test --- test/http2.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/http2.js b/test/http2.js index 2ec8ad2ba29..5d18d925875 100644 --- a/test/http2.js +++ b/test/http2.js @@ -61,13 +61,16 @@ test('Should support H2 connection', async t => { test('Should handle h2 continue', async t => { const requestBody = [] - const server = createSecureServer(pem) + const server = createSecureServer(pem, (request, response) => { + console.log('request handler called') + }) const responseBody = [] - server.on('request', (request, response) => { + server.on('checkContinue', (request, response) => { t.equal(request.headers.expect, '100-continue') t.equal(request.headers['x-my-header'], 'foo') t.equal(request.headers[':method'], 'POST') + response.writeContinue() request.on('data', chunk => requestBody.push(chunk)) @@ -160,7 +163,6 @@ test('Should handle h2 request with body (string or buffer)', async t => { 'x-my-header': 'foo' }, body: 'hello from client!' - // expectContinue: true }) response1.body.on('data', chunk => { From add512bfc80ac02dd5536defa0f8b4e36963ec44 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 24 May 2023 10:46:22 +0200 Subject: [PATCH 17/55] fix: bad check for headers --- lib/client.js | 3 +-- lib/core/util.js | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/client.js b/lib/client.js index 5873af36e39..62c094080c9 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1371,8 +1371,7 @@ function _resume (client, sync) { function write (client, request) { if (client[kHTTP2Session]) { - writeH2(client, client[kHTTP2Session], request) - return + return writeH2(client, client[kHTTP2Session], request) } const { body, method, path, host, upgrade, headers, blocking, reset } = request diff --git a/lib/core/util.js b/lib/core/util.js index 3a54a356836..8972ff3857f 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -225,7 +225,8 @@ function parseKeepAliveTimeout (val) { function parseHeaders (headers, obj = {}) { // For H2 support - if (typeof headers === 'object') return headers + if (!Array.isArray(headers)) return headers + for (let i = 0; i < headers.length; i += 2) { const key = headers[i].toString().toLowerCase() let val = obj[key] From 4ad0eb0e2e5c34938bae0e8648f160efab7669a1 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 24 May 2023 10:52:48 +0200 Subject: [PATCH 18/55] fix: bad change --- lib/client.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index 62c094080c9..5873af36e39 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1371,7 +1371,8 @@ function _resume (client, sync) { function write (client, request) { if (client[kHTTP2Session]) { - return writeH2(client, client[kHTTP2Session], request) + writeH2(client, client[kHTTP2Session], request) + return } const { body, method, path, host, upgrade, headers, blocking, reset } = request From d0d9a0a13b5f8886364a1a7c4ef1eaa2a5b64b5c Mon Sep 17 00:00:00 2001 From: Michael Kaufman <2073135+mkaufmaner@users.noreply.github.com> Date: Tue, 30 May 2023 09:59:31 -0400 Subject: [PATCH 19/55] chore: add http2 alpn test (#34) * chore: add http2 alpn test using fastify * chore: update to test https 1 with http2 * chore: update alpn test to return server request alpn protocol and http version * chore: add alpn with body * fix: remove fastify from package json --- test/http2-alpn.js | 275 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 275 insertions(+) create mode 100644 test/http2-alpn.js diff --git a/test/http2-alpn.js b/test/http2-alpn.js new file mode 100644 index 00000000000..712a493cb3c --- /dev/null +++ b/test/http2-alpn.js @@ -0,0 +1,275 @@ +'use strict' + +const https = require('node:https') +const { once } = require('node:events') +const { createSecureServer } = require('node:http2') +const { readFileSync } = require('node:fs') +const { join } = require('node:path') +const { test } = require('tap') + +const { Client } = require('..') + +// get the crypto fixtures +const key = readFileSync(join(__dirname, 'fixtures', 'key.pem'), 'utf8') +const cert = readFileSync(join(__dirname, 'fixtures', 'cert.pem'), 'utf8') +const ca = readFileSync(join(__dirname, 'fixtures', 'ca.pem'), 'utf8') + +test('Should upgrade to HTTP/2 when HTTPS/1 is available for GET', async (t) => { + t.plan(10) + + const body = [] + const httpsBody = [] + + // create the server and server stream handler + const server = createSecureServer( + { + key, + cert, + allowHTTP1: true + }, + (req, res) => { + const { socket: { alpnProtocol } } = req.httpVersion === '2.0' ? req.stream.session : req + + // handle http/1 requests + res.writeHead(200, { + 'content-type': 'application/json; charset=utf-8', + 'x-custom-request-header': req.headers['x-custom-request-header'] || '', + 'x-custom-response-header': `using ${req.httpVersion}` + }) + res.end(JSON.stringify({ + alpnProtocol, + httpVersion: req.httpVersion + })) + } + ) + + server.listen(0) + await once(server, 'listening') + + // close the server on teardown + t.teardown(server.close.bind(server)) + + // set the port + const port = server.address().port + + // test undici against http/2 + const client = new Client(`https://localhost:${port}`, { + connect: { + ca, + servername: 'agent1' + } + }) + + // close the client on teardown + t.teardown(client.close.bind(client)) + + // make an undici request using where it wants http/2 + const response = await client.request({ + path: '/', + method: 'GET', + headers: { + 'x-custom-request-header': 'want 2.0' + } + }) + + response.body.on('data', chunk => { + body.push(chunk) + }) + + await once(response.body, 'end') + + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'application/json; charset=utf-8') + t.equal(response.headers['x-custom-request-header'], 'want 2.0') + t.equal(response.headers['x-custom-response-header'], 'using 2.0') + t.equal(Buffer.concat(body).toString('utf8'), JSON.stringify({ + alpnProtocol: 'h2', + httpVersion: '2.0' + })) + + // make an https request for http/1 to confirm undici is using http/2 + const httpsOptions = { + ca, + servername: 'agent1', + headers: { + 'x-custom-request-header': 'want 1.1' + } + } + + const httpsResponse = await new Promise((resolve, reject) => { + const httpsRequest = https.get(`https://localhost:${port}/`, httpsOptions, (res) => { + res.on('data', (chunk) => { + httpsBody.push(chunk) + }) + + res.on('end', () => { + resolve(res) + }) + }).on('error', (err) => { + reject(err) + }) + + t.teardown(httpsRequest.destroy.bind(httpsRequest)) + }) + + t.equal(httpsResponse.statusCode, 200) + t.equal(httpsResponse.headers['content-type'], 'application/json; charset=utf-8') + t.equal(httpsResponse.headers['x-custom-request-header'], 'want 1.1') + t.equal(httpsResponse.headers['x-custom-response-header'], 'using 1.1') + t.equal(Buffer.concat(httpsBody).toString('utf8'), JSON.stringify({ + alpnProtocol: false, + httpVersion: '1.1' + })) +}) + +test('Should upgrade to HTTP/2 when HTTPS/1 is available for POST', async (t) => { + t.plan(15) + + const requestChunks = [] + const responseBody = [] + + const httpsRequestChunks = [] + const httpsResponseBody = [] + + const expectedBody = 'hello' + const buf = Buffer.from(expectedBody) + const body = new ArrayBuffer(buf.byteLength) + + buf.copy(new Uint8Array(body)) + + // create the server and server stream handler + const server = createSecureServer( + { + key, + cert, + allowHTTP1: true + }, + (req, res) => { + // use the stream handler for http2 + if (req.httpVersion === '2.0') { + return + } + + const { socket: { alpnProtocol } } = req + + req.on('data', (chunk) => { + httpsRequestChunks.push(chunk) + }) + + req.on('end', () => { + // handle http/1 requests + res.writeHead(201, { + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-request-header': req.headers['x-custom-request-header'] || '', + 'x-custom-alpn-protocol': alpnProtocol + }) + res.end('hello http/1!') + }) + } + ) + + server.on('stream', (stream, headers) => { + t.equal(headers[':method'], 'POST') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + const { socket: { alpnProtocol } } = stream.session + + stream.on('data', (chunk) => { + requestChunks.push(chunk) + }) + + stream.respond({ + ':status': 201, + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-request-header': headers['x-custom-request-header'] || '', + 'x-custom-alpn-protocol': alpnProtocol + }) + + stream.end('hello h2!') + }) + + server.listen(0) + await once(server, 'listening') + + // close the server on teardown + t.teardown(server.close.bind(server)) + + // set the port + const port = server.address().port + + // test undici against http/2 + const client = new Client(`https://localhost:${port}`, { + connect: { + ca, + servername: 'agent1' + } + }) + + // close the client on teardown + t.teardown(client.close.bind(client)) + + // make an undici request using where it wants http/2 + const response = await client.request({ + path: '/', + method: 'POST', + headers: { + 'x-custom-request-header': 'want 2.0' + }, + body + }) + + response.body.on('data', (chunk) => { + responseBody.push(chunk) + }) + + await once(response.body, 'end') + + t.equal(response.statusCode, 201) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-request-header'], 'want 2.0') + t.equal(response.headers['x-custom-alpn-protocol'], 'h2') + t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) + + // make an https request for http/1 to confirm undici is using http/2 + const httpsOptions = { + ca, + servername: 'agent1', + method: 'POST', + headers: { + 'content-type': 'text/plain; charset=utf-8', + 'content-length': Buffer.byteLength(body), + 'x-custom-request-header': 'want 1.1' + } + } + + const httpsResponse = await new Promise((resolve, reject) => { + const httpsRequest = https.request(`https://localhost:${port}/`, httpsOptions, (res) => { + res.on('data', (chunk) => { + httpsResponseBody.push(chunk) + }) + + res.on('end', () => { + resolve(res) + }) + }).on('error', (err) => { + reject(err) + }) + + httpsRequest.on('error', (err) => { + reject(err) + }) + + httpsRequest.write(Buffer.from(body)) + + t.teardown(httpsRequest.destroy.bind(httpsRequest)) + }) + + t.equal(httpsResponse.statusCode, 201) + t.equal(httpsResponse.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(httpsResponse.headers['x-custom-request-header'], 'want 1.1') + t.equal(httpsResponse.headers['x-custom-alpn-protocol'], 'false') + t.equal(Buffer.concat(httpsResponseBody).toString('utf-8'), 'hello http/1!') + t.equal(Buffer.concat(httpsRequestChunks).toString('utf-8'), expectedBody) +}) From 0da19331fa319a5d557a06ae46e06b0cabdac0f3 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 31 May 2023 13:15:08 +0200 Subject: [PATCH 20/55] refactor: remove leftover --- lib/client.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index 5873af36e39..361f4ec94da 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1689,7 +1689,6 @@ function writeH2 (client, session, request) { } stream.on('response', headers => { - // console.log('h2 response:', headers) if (request.onHeaders(Number(headers[':status']), headers, stream.resume.bind(stream), '') === false) { stream.pause() } From 916d62e3fa7c997f15a71780697cfa404140acd4 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 2 Jun 2023 11:05:19 +0200 Subject: [PATCH 21/55] test: ensure dispatch feature --- lib/api/api-request.js | 1 - test/http2.js | 123 +++++++++++++++++------------------------ 2 files changed, 50 insertions(+), 74 deletions(-) diff --git a/lib/api/api-request.js b/lib/api/api-request.js index df636ca561c..f130ecc9867 100644 --- a/lib/api/api-request.js +++ b/lib/api/api-request.js @@ -95,7 +95,6 @@ class RequestHandler extends AsyncResource { this.callback = null this.res = body - // console.log('called', callback !== null) if (callback !== null) { if (this.throwOnError && statusCode >= 400) { this.runInAsyncScope(getResolveErrorBodyCallback, null, diff --git a/test/http2.js b/test/http2.js index 5d18d925875..efba27186cd 100644 --- a/test/http2.js +++ b/test/http2.js @@ -61,9 +61,7 @@ test('Should support H2 connection', async t => { test('Should handle h2 continue', async t => { const requestBody = [] - const server = createSecureServer(pem, (request, response) => { - console.log('request handler called') - }) + const server = createSecureServer(pem, () => {}) const responseBody = [] server.on('checkContinue', (request, response) => { @@ -117,21 +115,14 @@ test('Should handle h2 continue', async t => { t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') }) -test('Should handle h2 request with body (string or buffer)', async t => { +test('Should handle h2 request with body (string or buffer) - dispatch', t => { const server = createSecureServer(pem) - const responseBody1 = [] - const responseBody2 = [] - const requestBodyString = [] - const requestBodyBuffer = [] - let reqCounter = 0 + const expectedBody = 'hello from client!' + const response = [] + const requestBody = [] server.on('stream', async (stream, headers) => { - reqCounter++ - if (reqCounter === 1) { - stream.on('data', chunk => requestBodyString.push(chunk)) - } else { - stream.on('data', chunk => requestBodyBuffer.push(chunk)) - } + stream.on('data', chunk => requestBody.push(chunk)) stream.respond({ 'content-type': 'text/plain; charset=utf-8', @@ -142,67 +133,53 @@ test('Should handle h2 request with body (string or buffer)', async t => { stream.end('hello h2!') }) - t.plan(10) - - server.listen(0) - await once(server, 'listening') - - const client = new Client(`https://localhost:${server.address().port}`, { - connect: { - rejectUnauthorized: false - } - }) - - t.teardown(server.close.bind(server)) - t.teardown(client.close.bind(client)) - - const response1 = await client.request({ - path: '/', - method: 'POST', - headers: { - 'x-my-header': 'foo' - }, - body: 'hello from client!' - }) - - response1.body.on('data', chunk => { - responseBody1.push(chunk) - }) - - await once(response1.body, 'end') + t.plan(7) - const response2 = await client.request({ - path: '/', - method: 'POST', - headers: { - 'x-my-header': 'foo' - }, - body: Buffer.from('hello from client!', 'utf-8') - }) + server.listen(0, () => { + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) - response2.body.on('data', chunk => { - responseBody2.push(chunk) + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + client.dispatch({ + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo', + 'content-type': 'text/plain' + }, + body: expectedBody + }, { + onConnect () { + t.ok(true) + }, + onError (err) { + t.error(err) + }, + onHeaders (statusCode, headers) { + t.equal(statusCode, 200) + t.equal(headers['content-type'], 'text/plain; charset=utf-8') + t.equal(headers['x-custom-h2'], 'foo') + }, + onData (chunk) { + response.push(chunk) + }, + onBodySent (body) { + t.equal(body.toString('utf-8'), expectedBody) + }, + onComplete () { + t.equal(Buffer.concat(response).toString('utf-8'), 'hello h2!') + t.equal( + Buffer.concat(requestBody).toString('utf-8'), + 'hello from client!' + ) + } + }) }) - - await once(response2.body, 'end') - - t.equal(response1.statusCode, 200) - t.equal(response1.headers['content-type'], 'text/plain; charset=utf-8') - t.equal(response1.headers['x-custom-h2'], 'foo') - t.equal(Buffer.concat(responseBody1).toString('utf-8'), 'hello h2!') - t.equal( - Buffer.concat(requestBodyString).toString('utf-8'), - 'hello from client!' - ) - - t.equal(response2.statusCode, 200) - t.equal(response2.headers['content-type'], 'text/plain; charset=utf-8') - t.equal(response2.headers['x-custom-h2'], 'foo') - t.equal(Buffer.concat(responseBody2).toString('utf-8'), 'hello h2!') - t.equal( - Buffer.concat(requestBodyBuffer).toString('utf-8'), - 'hello from client!' - ) }) test('Should handle h2 request with body (stream)', async t => { From 625b00889a5fdc9b209c8c277441b619b26f040c Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 21 Jun 2023 13:39:38 +0200 Subject: [PATCH 22/55] feat(h2): support connect --- lib/api/api-connect.js | 10 ++++-- lib/client.js | 77 ++++++++++++++++++++++++++---------------- test/http2.js | 48 +++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 32 deletions(-) diff --git a/lib/api/api-connect.js b/lib/api/api-connect.js index 0503b1a2f0e..92f317fb842 100644 --- a/lib/api/api-connect.js +++ b/lib/api/api-connect.js @@ -1,7 +1,7 @@ 'use strict' -const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors') const { AsyncResource } = require('async_hooks') +const { InvalidArgumentError, RequestAbortedError, SocketError } = require('../core/errors') const util = require('../core/util') const { addSignal, removeSignal } = require('./abort-signal') @@ -50,7 +50,13 @@ class ConnectHandler extends AsyncResource { removeSignal(this) this.callback = null - const headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders) + + let headers + // Indicates is an HTTP2Session + if (headers != null) { + headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders) + } + this.runInAsyncScope(callback, null, null, { statusCode, headers, diff --git a/lib/client.js b/lib/client.js index 361f4ec94da..efa9353010d 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1531,10 +1531,56 @@ function write (client, request) { } function writeH2 (client, session, request) { - const { body, method, path, host, upgrade, expectContinue } = request - + const { body, method, path, host, upgrade, expectContinue, signal } = request assert(!upgrade, 'Upgrade not supported for H2') + try { + // TODO(HTTP/2): Should we call onConnect immediately or on stream ready event? + request.onConnect((err) => { + if (request.aborted || request.completed) { + return + } + + errorRequest(client, request, err || new RequestAbortedError()) + }) + } catch (err) { + errorRequest(client, request, err) + } + + if (request.aborted) { + return false + } + + let stream + const headers = { ...request[kRawRequestHeaders] } + headers[':authority'] = host || client[kHost] + headers[':path'] = path + + if (method === 'CONNECT') { + session.ref() + // we are already connected, streams are pending, first request + // will create a new stream. We trigger a request to create the stream and wait until + // `ready` event is triggered + stream = session.request(headers, { endStream: false, signal }) + + if (stream.state.state > 0 && stream.id) { + request.onUpgrade(null, null, stream) + } else { + stream.once('ready', () => { + request.onUpgrade(null, null, stream) + }) + } + + stream.on('close', () => { + // TODO(HTTP/2): unref only if current streams count is 0 + session.unref() + }) + + return true + } else { + headers[':method'] = method + } + // https://tools.ietf.org/html/rfc7231#section-4.3.1 // https://tools.ietf.org/html/rfc7231#section-4.3.2 // https://tools.ietf.org/html/rfc7231#section-4.3.5 @@ -1579,36 +1625,13 @@ function writeH2 (client, session, request) { process.emitWarning(new RequestContentLengthMismatchError()) } - try { - // TODO(HTTP/2): Should we call onConnect immediately or on stream ready event? - request.onConnect((err) => { - if (request.aborted || request.completed) { - return - } - - errorRequest(client, request, err || new RequestAbortedError()) - }) - } catch (err) { - errorRequest(client, request, err) - } - - if (request.aborted) { - return false - } - // TODO: Content length needs to be calculated at this stage - const headers = { ...request[kRawRequestHeaders] } - headers[':authority'] = host || client[kHost] - headers[':path'] = path - headers[':method'] = method - if (contentLength != null) { assert(body, 'no body must not have content length') headers['content-length'] = `${contentLength}` } session.ref() - let stream if (expectContinue) { headers.Expect = '100-continue' /** @@ -1711,10 +1734,6 @@ function writeH2 (client, session, request) { // TODO(HTTP/2): Support aborted }) - stream.on('ready', () => { - // TODO(HTTP/2): Support ready - }) - stream.on('timeout', () => { // TODO(HTTP/2): Support timeout }) diff --git a/test/http2.js b/test/http2.js index efba27186cd..570045ae232 100644 --- a/test/http2.js +++ b/test/http2.js @@ -10,7 +10,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(7) +plan(8) test('Should support H2 connection', async t => { const body = [] @@ -115,6 +115,52 @@ test('Should handle h2 continue', async t => { t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') }) +test('Dispatcher#Connect', t => { + const server = createSecureServer(pem) + const expectedBody = 'hello from client!' + let requestBody = '' + + server.on('stream', async (stream, headers) => { + stream.setEncoding('utf-8') + stream.on('data', chunk => { + requestBody += chunk + }) + + stream.respond({ ':status': 200, 'x-custom': 'custom-header' }) + stream.end('hello h2!') + }) + + t.plan(6) + + server.listen(0, () => { + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + let result = '' + client.connect({ path: '/' }, (err, { socket }) => { + t.error(err) + socket.on('data', chunk => { result += chunk }) + socket.on('response', (headers) => { + t.equal(headers[':status'], 200) + t.equal(headers['x-custom'], 'custom-header') + t.notOk(socket.closed) + }) + + socket.once('end', () => { + t.equal(requestBody, expectedBody) + t.equal(result, 'hello h2!') + }) + socket.end(expectedBody) + }) + }) +}) + test('Should handle h2 request with body (string or buffer) - dispatch', t => { const server = createSecureServer(pem) const expectedBody = 'hello from client!' From 55d4a565c7456f7d9b166853918621fea86a722f Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 21 Jun 2023 13:40:00 +0200 Subject: [PATCH 23/55] fix: pass signal down the road --- lib/client.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/client.js b/lib/client.js index efa9353010d..43004464147 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1637,12 +1637,12 @@ function writeH2 (client, session, request) { /** * @type {import('node:http2').ClientHttp2Stream} */ - stream = session.request(headers) + stream = session.request(headers, { endStream: false, signal }) stream.once('continue', writeBodyH2) } else { /** @type {import('node:http2').ClientHttp2Stream} */ - stream = session.request(headers) + stream = session.request(headers, { endStream: false, signal }) writeBodyH2() } From d909fd7e82ac452472ddf8209b94d00a743ec70b Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Jun 2023 10:30:14 +0200 Subject: [PATCH 24/55] test: ensure stream works as expected --- test/http2.js | 232 +++++++++++++++++++++++++++++++------------------- 1 file changed, 145 insertions(+), 87 deletions(-) diff --git a/test/http2.js b/test/http2.js index 570045ae232..e992c7fd129 100644 --- a/test/http2.js +++ b/test/http2.js @@ -4,13 +4,14 @@ const { createSecureServer } = require('node:http2') const { createReadStream, readFileSync } = require('node:fs') const { once } = require('node:events') const { Blob } = require('node:buffer') +const { Writable } = require('node:stream') const { test, plan } = require('tap') const pem = require('https-pem') const { Client } = require('..') -plan(8) +plan(9) test('Should support H2 connection', async t => { const body = [] @@ -115,6 +116,54 @@ test('Should handle h2 continue', async t => { t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') }) +test('Dispatcher#Stream', t => { + const server = createSecureServer(pem) + const expectedBody = 'hello from client!' + const bufs = [] + let requestBody = '' + + server.on('stream', async (stream, headers) => { + stream.setEncoding('utf-8') + stream.on('data', chunk => { + requestBody += chunk + }) + + stream.respond({ ':status': 200, 'x-custom': 'custom-header' }) + stream.end('hello h2!') + }) + + t.plan(4) + + server.listen(0, async () => { + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + await client.stream( + { path: '/', opaque: { bufs }, method: 'POST', body: expectedBody }, + ({ statusCode, headers, opaque: { bufs } }) => { + t.equal(statusCode, 200) + t.equal(headers['x-custom'], 'custom-header') + + return new Writable({ + write (chunk, _encoding, cb) { + bufs.push(chunk) + cb() + } + }) + } + ) + + t.equal(Buffer.concat(bufs).toString('utf-8'), 'hello h2!') + t.equal(requestBody, expectedBody) + }) +}) + test('Dispatcher#Connect', t => { const server = createSecureServer(pem) const expectedBody = 'hello from client!' @@ -145,8 +194,10 @@ test('Dispatcher#Connect', t => { let result = '' client.connect({ path: '/' }, (err, { socket }) => { t.error(err) - socket.on('data', chunk => { result += chunk }) - socket.on('response', (headers) => { + socket.on('data', chunk => { + result += chunk + }) + socket.on('response', headers => { t.equal(headers[':status'], 200) t.equal(headers['x-custom'], 'custom-header') t.notOk(socket.closed) @@ -191,40 +242,43 @@ test('Should handle h2 request with body (string or buffer) - dispatch', t => { t.teardown(server.close.bind(server)) t.teardown(client.close.bind(client)) - client.dispatch({ - path: '/', - method: 'POST', - headers: { - 'x-my-header': 'foo', - 'content-type': 'text/plain' - }, - body: expectedBody - }, { - onConnect () { - t.ok(true) - }, - onError (err) { - t.error(err) - }, - onHeaders (statusCode, headers) { - t.equal(statusCode, 200) - t.equal(headers['content-type'], 'text/plain; charset=utf-8') - t.equal(headers['x-custom-h2'], 'foo') + client.dispatch( + { + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo', + 'content-type': 'text/plain' + }, + body: expectedBody }, - onData (chunk) { - response.push(chunk) - }, - onBodySent (body) { - t.equal(body.toString('utf-8'), expectedBody) - }, - onComplete () { - t.equal(Buffer.concat(response).toString('utf-8'), 'hello h2!') - t.equal( - Buffer.concat(requestBody).toString('utf-8'), - 'hello from client!' - ) + { + onConnect () { + t.ok(true) + }, + onError (err) { + t.error(err) + }, + onHeaders (statusCode, headers) { + t.equal(statusCode, 200) + t.equal(headers['content-type'], 'text/plain; charset=utf-8') + t.equal(headers['x-custom-h2'], 'foo') + }, + onData (chunk) { + response.push(chunk) + }, + onBodySent (body) { + t.equal(body.toString('utf-8'), expectedBody) + }, + onComplete () { + t.equal(Buffer.concat(response).toString('utf-8'), 'hello h2!') + t.equal( + Buffer.concat(requestBody).toString('utf-8'), + 'hello from client!' + ) + } } - }) + ) }) }) @@ -293,7 +347,7 @@ test('Should handle h2 request with body (iterable)', async t => { const requestChunks = [] const responseBody = [] const iterableBody = { - [Symbol.iterator]: function * () { + [Symbol.iterator]: function* () { const end = expectedBody.length - 1 for (let i = 0; i < end + 1; i++) { yield expectedBody[i] @@ -416,64 +470,68 @@ test('Should handle h2 request with body (Blob)', { skip: !Blob }, async t => { t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) }) -test('Should handle h2 request with body (Blob:ArrayBuffer)', { skip: !Blob }, async t => { - const server = createSecureServer(pem) - const expectedBody = 'hello' - const requestChunks = [] - const responseBody = [] - const buf = Buffer.from(expectedBody) - const body = new ArrayBuffer(buf.byteLength) - - buf.copy(new Uint8Array(body)) - - server.on('stream', async (stream, headers) => { - t.equal(headers[':method'], 'POST') - t.equal(headers[':path'], '/') - t.equal(headers[':scheme'], 'https') - - stream.on('data', chunk => requestChunks.push(chunk)) +test( + 'Should handle h2 request with body (Blob:ArrayBuffer)', + { skip: !Blob }, + async t => { + const server = createSecureServer(pem) + const expectedBody = 'hello' + const requestChunks = [] + const responseBody = [] + const buf = Buffer.from(expectedBody) + const body = new ArrayBuffer(buf.byteLength) + + buf.copy(new Uint8Array(body)) + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'POST') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) - stream.respond({ - 'content-type': 'text/plain; charset=utf-8', - 'x-custom-h2': headers['x-my-header'], - ':status': 200 + stream.end('hello h2!') }) - stream.end('hello h2!') - }) - - t.plan(8) + t.plan(8) - server.listen(0) - await once(server, 'listening') + server.listen(0) + await once(server, 'listening') - const client = new Client(`https://localhost:${server.address().port}`, { - connect: { - rejectUnauthorized: false - } - }) + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) - t.teardown(server.close.bind(server)) - t.teardown(client.close.bind(client)) + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) - const response = await client.request({ - path: '/', - method: 'POST', - headers: { - 'x-my-header': 'foo' - }, - body - }) + const response = await client.request({ + path: '/', + method: 'POST', + headers: { + 'x-my-header': 'foo' + }, + body + }) - response.body.on('data', chunk => { - responseBody.push(chunk) - }) + response.body.on('data', chunk => { + responseBody.push(chunk) + }) - await once(response.body, 'end') + await once(response.body, 'end') - t.equal(response.statusCode, 200) - t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') - t.equal(response.headers['x-custom-h2'], 'foo') - t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') - t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) -}) + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'foo') + t.equal(Buffer.concat(responseBody).toString('utf-8'), 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) + } +) From e67372e5f818efc202d666822d684c7deed44ed0 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Jun 2023 10:43:25 +0200 Subject: [PATCH 25/55] test: ensure pipeline works as expected --- test/http2.js | 65 ++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/test/http2.js b/test/http2.js index e992c7fd129..cbee6239a74 100644 --- a/test/http2.js +++ b/test/http2.js @@ -4,14 +4,14 @@ const { createSecureServer } = require('node:http2') const { createReadStream, readFileSync } = require('node:fs') const { once } = require('node:events') const { Blob } = require('node:buffer') -const { Writable } = require('node:stream') +const { Writable, pipeline, PassThrough, Readable } = require('node:stream') const { test, plan } = require('tap') const pem = require('https-pem') const { Client } = require('..') -plan(9) +plan(10) test('Should support H2 connection', async t => { const body = [] @@ -164,6 +164,65 @@ test('Dispatcher#Stream', t => { }) }) +test('Dispatcher#Pipeline', t => { + const server = createSecureServer(pem) + const expectedBody = 'hello from client!' + const bufs = [] + let requestBody = '' + + server.on('stream', async (stream, headers) => { + stream.setEncoding('utf-8') + stream.on('data', chunk => { + requestBody += chunk + }) + + stream.respond({ ':status': 200, 'x-custom': 'custom-header' }) + stream.end('hello h2!') + }) + + t.plan(5) + + server.listen(0, () => { + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + pipeline( + new Readable({ + read () { + this.push(Buffer.from(expectedBody)) + this.push(null) + } + }), + client.pipeline( + { path: '/', method: 'POST', body: expectedBody }, + ({ statusCode, headers, body }) => { + t.equal(statusCode, 200) + t.equal(headers['x-custom'], 'custom-header') + + return pipeline(body, new PassThrough(), () => {}) + } + ), + new Writable({ + write (chunk, _, cb) { + bufs.push(chunk) + cb() + } + }), + (err) => { + t.error(err) + t.equal(Buffer.concat(bufs).toString('utf-8'), 'hello h2!') + t.equal(requestBody, expectedBody) + } + ) + }) +}) + test('Dispatcher#Connect', t => { const server = createSecureServer(pem) const expectedBody = 'hello from client!' @@ -347,7 +406,7 @@ test('Should handle h2 request with body (iterable)', async t => { const requestChunks = [] const responseBody = [] const iterableBody = { - [Symbol.iterator]: function* () { + [Symbol.iterator]: function * () { const end = expectedBody.length - 1 for (let i = 0; i < end + 1; i++) { yield expectedBody[i] From 1919c251f80c36b48f3cbb09c62dca7b8490b7f1 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Jun 2023 10:59:54 +0200 Subject: [PATCH 26/55] test: ensure upgrade fails --- lib/client.js | 6 +++++- test/http2.js | 29 ++++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/lib/client.js b/lib/client.js index 43004464147..ff48fbdde8b 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1532,7 +1532,11 @@ function write (client, request) { function writeH2 (client, session, request) { const { body, method, path, host, upgrade, expectContinue, signal } = request - assert(!upgrade, 'Upgrade not supported for H2') + + if (upgrade) { + errorRequest(client, request, new Error('Upgrade not supported for H2')) + return false + } try { // TODO(HTTP/2): Should we call onConnect immediately or on stream ready event? diff --git a/test/http2.js b/test/http2.js index cbee6239a74..e8d65c16e1c 100644 --- a/test/http2.js +++ b/test/http2.js @@ -11,7 +11,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(10) +plan(11) test('Should support H2 connection', async t => { const body = [] @@ -271,6 +271,33 @@ test('Dispatcher#Connect', t => { }) }) +test('Dispatcher#Upgrade', t => { + const server = createSecureServer(pem) + + server.on('stream', async (stream, headers) => { + stream.end() + }) + + t.plan(1) + + server.listen(0, async () => { + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + try { + await client.upgrade({ path: '/' }) + } catch (error) { + t.equal(error.message, 'Upgrade not supported for H2') + } + }) +}) + test('Should handle h2 request with body (string or buffer) - dispatch', t => { const server = createSecureServer(pem) const expectedBody = 'hello from client!' From 047598804d67a79f89faa40a9c278e50b2ab7764 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 23 Jun 2023 13:40:54 +0200 Subject: [PATCH 27/55] test: ensure destroy works as expected --- test/http2.js | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/test/http2.js b/test/http2.js index e8d65c16e1c..dc6e3ac9581 100644 --- a/test/http2.js +++ b/test/http2.js @@ -11,7 +11,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(11) +plan(12) test('Should support H2 connection', async t => { const body = [] @@ -298,6 +298,59 @@ test('Dispatcher#Upgrade', t => { }) }) +test('Dispatcher#destroy', async t => { + const promises = [] + const server = createSecureServer(pem) + + server.on('stream', (stream, headers) => { + setTimeout(stream.end.bind(stream), 1500) + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.plan(3) + t.teardown(server.close.bind(server)) + + promises.push(client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + })) + + promises.push(client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + })) + + promises.push(client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + })) + + await client.destroy() + + const results = await Promise.allSettled(promises) + + t.equal(results[0].status, 'rejected') + t.equal(results[1].status, 'rejected') + t.equal(results[2].status, 'rejected') +}) + test('Should handle h2 request with body (string or buffer) - dispatch', t => { const server = createSecureServer(pem) const expectedBody = 'hello from client!' From 00187b7671f3c8b849793271033184572be37f02 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 28 Jun 2023 10:26:27 +0200 Subject: [PATCH 28/55] feat: allow to disable H2 calls upon request --- lib/client.js | 11 ++++- lib/core/connect.js | 6 +-- test/http2.js | 97 ++++++++++++++++++++++++++++++++++----------- 3 files changed, 86 insertions(+), 28 deletions(-) diff --git a/lib/client.js b/lib/client.js index ff48fbdde8b..3e945d55e08 100644 --- a/lib/client.js +++ b/lib/client.js @@ -128,7 +128,8 @@ class Client extends DispatcherBase { localAddress, maxResponseSize, autoSelectFamily, - autoSelectFamilyAttemptTimeout + autoSelectFamilyAttemptTimeout, + allowH2, } = {}) { super() @@ -200,6 +201,10 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('localAddress must be valid string IP address') } + if (allowH2 != null && typeof allowH2 !== 'boolean') { + throw new InvalidArgumentError('allowH2 must be a valid boolean value') + } + if (maxResponseSize != null && (!Number.isInteger(maxResponseSize) || maxResponseSize < -1)) { throw new InvalidArgumentError('maxResponseSize must be a positive number') } @@ -215,6 +220,7 @@ class Client extends DispatcherBase { connect = buildConnector({ ...tls, maxCachedSessions, + allowH2, socketPath, timeout: connectTimeout, ...(util.nodeHasAutoSelectFamily && autoSelectFamily ? { autoSelectFamily, autoSelectFamilyAttemptTimeout } : undefined), @@ -1116,7 +1122,8 @@ async function connect (client) { assert(socket) - if (socket.alpnProtocol === 'h2') { + const isH2 = socket.alpnProtocol === 'h2' + if (isH2) { const session = http2.connect(client[kUrl], { createConnection: () => socket }) diff --git a/lib/core/connect.js b/lib/core/connect.js index 2f42519c866..8673e747793 100644 --- a/lib/core/connect.js +++ b/lib/core/connect.js @@ -71,7 +71,7 @@ if (global.FinalizationRegistry) { } } -function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { +function buildConnector ({ allowH2, maxCachedSessions, socketPath, timeout, ...opts }) { if (maxCachedSessions != null && (!Number.isInteger(maxCachedSessions) || maxCachedSessions < 0)) { throw new InvalidArgumentError('maxCachedSessions must be a positive integer or zero') } @@ -79,7 +79,7 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { const options = { path: socketPath, ...opts } const sessionCache = new SessionCache(maxCachedSessions == null ? 100 : maxCachedSessions) timeout = timeout == null ? 10e3 : timeout - + allowH2 = allowH2 != null ? allowH2 : true return function connect ({ hostname, host, protocol, port, servername, localAddress, httpSocket }, callback) { let socket if (protocol === 'https:') { @@ -100,7 +100,7 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) { session, localAddress, // TODO(HTTP/2): Add support for h2c - ALPNProtocols: ['h2', 'http/1.1'], + ALPNProtocols: allowH2 ? ['http/1.1', 'h2'] : ['http/1.1'], socket: httpSocket, // upgrade socket connection port: port || 443, host: hostname diff --git a/test/http2.js b/test/http2.js index dc6e3ac9581..f8d8ac38fbe 100644 --- a/test/http2.js +++ b/test/http2.js @@ -11,7 +11,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(12) +plan(14) test('Should support H2 connection', async t => { const body = [] @@ -60,6 +60,51 @@ test('Should support H2 connection', async t => { t.equal(Buffer.concat(body).toString('utf8'), 'hello h2!') }) +test('Should throw if bad allowH2 has been pased', async t => { + try { + // eslint-disable-next-line + new Client('https://localhost:1000', { + allowH2: 'true' + }) + t.fail() + } catch (error) { + t.equal(error.message, 'allowH2 must be a valid boolean value') + } +}) + +test('Request should fail if allowH2 is false and server advertises h2 only', async t => { + const server = createSecureServer({ + ...pem, + allowHTTP1: false, + ALPNProtocols: ['http/1.1'] + }, (req, res) => { + t.fail('Should not create a valid h2 stream') + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + allowH2: false, + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) + + t.equal(response.statusCode, 403) +}) + test('Should handle h2 continue', async t => { const requestBody = [] const server = createSecureServer(pem, () => {}) @@ -214,7 +259,7 @@ test('Dispatcher#Pipeline', t => { cb() } }), - (err) => { + err => { t.error(err) t.equal(Buffer.concat(bufs).toString('utf-8'), 'hello h2!') t.equal(requestBody, expectedBody) @@ -318,29 +363,35 @@ test('Dispatcher#destroy', async t => { t.plan(3) t.teardown(server.close.bind(server)) - promises.push(client.request({ - path: '/', - method: 'GET', - headers: { - 'x-my-header': 'foo' - } - })) + promises.push( + client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) + ) - promises.push(client.request({ - path: '/', - method: 'GET', - headers: { - 'x-my-header': 'foo' - } - })) + promises.push( + client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) + ) - promises.push(client.request({ - path: '/', - method: 'GET', - headers: { - 'x-my-header': 'foo' - } - })) + promises.push( + client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) + ) await client.destroy() From c18c6bf7f0555821df56fe6a6dbb9f64f275553b Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 28 Jun 2023 10:37:10 +0200 Subject: [PATCH 29/55] fix: linting --- lib/client.js | 2 +- types/client.d.ts | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index 3e945d55e08..e77d109f437 100644 --- a/lib/client.js +++ b/lib/client.js @@ -129,7 +129,7 @@ class Client extends DispatcherBase { maxResponseSize, autoSelectFamily, autoSelectFamilyAttemptTimeout, - allowH2, + allowH2 } = {}) { super() diff --git a/types/client.d.ts b/types/client.d.ts index 56074a15ae7..cfdbf977e55 100644 --- a/types/client.d.ts +++ b/types/client.d.ts @@ -72,6 +72,8 @@ export declare namespace Client { autoSelectFamily?: boolean; /** The amount of time in milliseconds to wait for a connection attempt to finish before trying the next address when using the `autoSelectFamily` option. */ autoSelectFamilyAttemptTimeout?: number; + // TODO + allowH2?: boolean; } export interface SocketInfo { localAddress?: string From 4758fc2e5139a58eba858d0bebfb4a2883ab2211 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 3 Jul 2023 17:16:26 +0200 Subject: [PATCH 30/55] feat: support GOAWAY frame (server-side) --- lib/client.js | 23 +++++++++++++++++++ test/http2.js | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/lib/client.js b/lib/client.js index e77d109f437..cf5114d123d 100644 --- a/lib/client.js +++ b/lib/client.js @@ -335,6 +335,8 @@ class Client extends DispatcherBase { } async [kClose] () { + // TODO: for H2 we need to gracefully flush the remaining enqueued + // request and close each stream. return new Promise((resolve) => { if (!this[kSize]) { resolve(null) @@ -380,6 +382,26 @@ function onHttp2SessionError (err) { onError(this[kClient], err) } +function onHTTP2GoAway (code) { + // TODO; iterate over streams and + // shutdown each one of them before + // fully closing and destroying the Socket + // NOTE: The last ID of the frame received is the last processed + // stream. Remaining streams should finish its work before + // closing them. New streams cannot be created + // When started on client side, the same logic applies. + // Remaining streams should finish its work before + // closing the connection. No new streams can be opened. + this.emit('disconnect', + this[kUrl], + [this], + new InformationalError(`HTTP/2: "GOAWAY" frame received with code ${code}`) + ) + + // TODO: Gracefully shutdown each stream + this.close() +} + const constants = require('./llhttp/constants') const createRedirectInterceptor = require('./interceptor/redirectInterceptor') const EMPTY_BUF = Buffer.alloc(0) @@ -1131,6 +1153,7 @@ async function connect (client) { session[kError] = null session[kClient] = client session.on('error', onHttp2SessionError) + session.on('goaway', onHTTP2GoAway.bind(client)) session.on('close', onSocketClose) session.unref() diff --git a/test/http2.js b/test/http2.js index f8d8ac38fbe..ec5972267ae 100644 --- a/test/http2.js +++ b/test/http2.js @@ -11,7 +11,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(14) +plan(15) test('Should support H2 connection', async t => { const body = [] @@ -60,6 +60,65 @@ test('Should support H2 connection', async t => { t.equal(Buffer.concat(body).toString('utf8'), 'hello h2!') }) +test('Should support H2 GOAWAY (server-side)', async t => { + const body = [] + const server = createSecureServer(pem) + + server.on('stream', (stream, headers) => { + t.equal(headers['x-my-header'], 'foo') + t.equal(headers[':method'], 'GET') + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': 'hello', + ':status': 200 + }) + stream.end('hello h2!') + }) + + server.on('session', (session) => { + setTimeout(() => { + session.goaway(204) + }, 1000) + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.plan(9) + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) + + response.body.on('data', chunk => { + body.push(chunk) + }) + + await once(response.body, 'end') + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'hello') + t.equal(Buffer.concat(body).toString('utf8'), 'hello h2!') + + const [url, disconnectClient, err] = await once(client, 'disconnect') + + t.type(url, URL) + t.same(disconnectClient, [client]) + t.equal(err.message, 'HTTP/2: "GOAWAY" frame received with code 204') +}) + test('Should throw if bad allowH2 has been pased', async t => { try { // eslint-disable-next-line From cae86574918e95a3067c50ea09c6be44131acb9c Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 4 Jul 2023 17:45:22 +0200 Subject: [PATCH 31/55] refactor; use h2 constants --- lib/client.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/client.js b/lib/client.js index cf5114d123d..80e1ff2690c 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1587,8 +1587,8 @@ function writeH2 (client, session, request) { let stream const headers = { ...request[kRawRequestHeaders] } - headers[':authority'] = host || client[kHost] - headers[':path'] = path + headers[http2.constants.HTTP2_HEADER_AUTHORITY] = host || client[kHost] + headers[http2.constants.HTTP2_HEADER_PATH] = path if (method === 'CONNECT') { session.ref() @@ -1612,7 +1612,7 @@ function writeH2 (client, session, request) { return true } else { - headers[':method'] = method + headers[http2.constants.HTTP2_HEADER_METHOD] = method } // https://tools.ietf.org/html/rfc7231#section-4.3.1 @@ -1636,7 +1636,6 @@ function writeH2 (client, session, request) { } let contentLength = util.bodyLength(body) - if (contentLength == null) { contentLength = request.contentLength } @@ -1659,15 +1658,14 @@ function writeH2 (client, session, request) { process.emitWarning(new RequestContentLengthMismatchError()) } - // TODO: Content length needs to be calculated at this stage if (contentLength != null) { assert(body, 'no body must not have content length') - headers['content-length'] = `${contentLength}` + headers[http2.constants.HTTP2_HEADER_CONTENT_LENGTH] = `${contentLength}` } session.ref() if (expectContinue) { - headers.Expect = '100-continue' + headers[http2.constants.HTTP2_HEADER_EXPECT] = '100-continue' /** * @type {import('node:http2').ClientHttp2Stream} */ From d9ea2cf0302132bd67b66b656cc2cdc7c19c0012 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 12 Jul 2023 13:10:09 +0200 Subject: [PATCH 32/55] feat: initial shape of concurrent stream handling --- lib/client.js | 75 +++++++++++++++++++++++++++++++++++---------- lib/core/symbols.js | 1 + 2 files changed, 59 insertions(+), 17 deletions(-) diff --git a/lib/client.js b/lib/client.js index 80e1ff2690c..543c6925fcb 100644 --- a/lib/client.js +++ b/lib/client.js @@ -27,6 +27,7 @@ const { ClientDestroyedError } = require('./core/errors') const buildConnector = require('./core/connect') +const FixedQueue = require('./node/fixed-queue') const { kUrl, kReset, @@ -73,8 +74,20 @@ const { // HTTP2 kHost, kHTTP2Session, + kHTTP2SessionState, kRawRequestHeaders } = require('./core/symbols') +const { + constants: { + HTTP2_HEADER_AUTHORITY, + HTTP2_HEADER_METHOD, + HTTP2_HEADER_PATH, + HTTP2_HEADER_CONTENT_LENGTH, + HTTP2_HEADER_EXPECT, + HTTP2_HEADER_STATUS + } +} = http2 + const FastBuffer = Buffer[Symbol.species] const kClosedResolve = Symbol('kClosedResolve') @@ -255,6 +268,11 @@ class Client extends DispatcherBase { // HTTP/2 this[kHTTP2Session] = null + this[kHTTP2SessionState] = { + streams: null, // Fixed queue of streams + openStreams: 0, + maxStreams: 100 // Max peerConcurrentStreams for a Node h2 server + } this[kHost] = `${this[kUrl].hostname}${this[kUrl].port ? `:${this[kUrl].port}` : ''}` // kQueue is built up of 3 sections separated by @@ -363,6 +381,10 @@ class Client extends DispatcherBase { resolve() } + if (this[kHTTP2Session] != null) { + this[kHTTP2Session].destroy() + } + if (!this[kSocket]) { queueMicrotask(callback) } else { @@ -1147,7 +1169,8 @@ async function connect (client) { const isH2 = socket.alpnProtocol === 'h2' if (isH2) { const session = http2.connect(client[kUrl], { - createConnection: () => socket + createConnection: () => socket, + peerMaxConcurrentStreams: client[kHTTP2SessionState].maxStreams }) session[kError] = null @@ -1158,6 +1181,7 @@ async function connect (client) { session.unref() client[kHTTP2Session] = session + client[kHTTP2SessionState].streams = new FixedQueue() } else { if (!llhttpInstance) { llhttpInstance = await llhttpPromise @@ -1587,8 +1611,8 @@ function writeH2 (client, session, request) { let stream const headers = { ...request[kRawRequestHeaders] } - headers[http2.constants.HTTP2_HEADER_AUTHORITY] = host || client[kHost] - headers[http2.constants.HTTP2_HEADER_PATH] = path + headers[HTTP2_HEADER_AUTHORITY] = host || client[kHost] + headers[HTTP2_HEADER_PATH] = path if (method === 'CONNECT') { session.ref() @@ -1597,7 +1621,7 @@ function writeH2 (client, session, request) { // `ready` event is triggered stream = session.request(headers, { endStream: false, signal }) - if (stream.state.state > 0 && stream.id) { + if (stream.id && !stream.pending) { request.onUpgrade(null, null, stream) } else { stream.once('ready', () => { @@ -1612,7 +1636,7 @@ function writeH2 (client, session, request) { return true } else { - headers[http2.constants.HTTP2_HEADER_METHOD] = method + headers[HTTP2_HEADER_METHOD] = method } // https://tools.ietf.org/html/rfc7231#section-4.3.1 @@ -1660,12 +1684,21 @@ function writeH2 (client, session, request) { if (contentLength != null) { assert(body, 'no body must not have content length') - headers[http2.constants.HTTP2_HEADER_CONTENT_LENGTH] = `${contentLength}` + headers[HTTP2_HEADER_CONTENT_LENGTH] = `${contentLength}` } session.ref() + const shouldCreateNewStream = client[kHTTP2SessionState].streams.isEmpty() || + client[kHTTP2SessionState].openStreams < client[kHTTP2SessionState].maxStreams + if (expectContinue) { - headers[http2.constants.HTTP2_HEADER_EXPECT] = '100-continue' + headers[HTTP2_HEADER_EXPECT] = '100-continue' + + if (!shouldCreateNewStream) { + errorRequest(client, request, new Error('H2: Max streams reached')) + return false + } + /** * @type {import('node:http2').ClientHttp2Stream} */ @@ -1681,15 +1714,16 @@ function writeH2 (client, session, request) { function writeBodyH2 () { /* istanbul ignore else: assertion */ if (!body) { - // Indicate no more data will be written - stream.end() - request.onRequestSent() } else if (util.isBuffer(body)) { assert(contentLength === body.byteLength, 'buffer body must have content length') - stream.end(body) + stream.cork() + stream.write(body) + stream.uncork() request.onBodySent(body) request.onRequestSent() + + client[kHTTP2SessionState].streams.push(stream) } else if (util.isBlobLike(body)) { if (typeof body.stream === 'function') { writeIterable({ @@ -1789,6 +1823,7 @@ function writeStream ({ h2Stream, body, client, request, socket, contentLength, (err) => { if (err) { util.destroy(body, err) + util.destroy(h2Stream, err) } } ) @@ -1798,6 +1833,7 @@ function writeStream ({ h2Stream, body, client, request, socket, contentLength, pipe.removeListener('data', onPipeData) util.destroy(pipe) request.onRequestSent() + client[kHTTP2SessionState].streams.push(h2Stream) }) body.once('end', () => util.destroy(body)) @@ -1892,6 +1928,7 @@ function writeStream ({ h2Stream, body, client, request, socket, contentLength, async function writeBlob ({ h2stream, body, client, request, socket, contentLength, header, expectsPayload }) { assert(contentLength === body.size, 'blob body must have content length') + const isH2 = socket.alpnProtocol === 'h2' try { if (contentLength != null && contentLength !== body.size) { throw new RequestContentLengthMismatchError() @@ -1899,17 +1936,19 @@ async function writeBlob ({ h2stream, body, client, request, socket, contentLeng const buffer = Buffer.from(await body.arrayBuffer()) - socket.cork() - - if (socket.alpnProtocol === 'h2') { + if (isH2) { + h2stream.cork() h2stream.write(buffer) + h2stream.uncork() + + client[kHTTP2SessionState].streams.push(h2stream) } else { + socket.cork() socket.write(`${header}content-length: ${contentLength}\r\n\r\n`, 'latin1') socket.write(buffer) + socket.uncork() } - socket.uncork() - request.onBodySent(buffer) request.onRequestSent() @@ -1919,7 +1958,7 @@ async function writeBlob ({ h2stream, body, client, request, socket, contentLeng resume(client) } catch (err) { - util.destroy(socket, err) + util.destroy(isH2 ? h2stream : socket, err) } } @@ -1961,6 +2000,8 @@ async function writeIterable ({ h2stream, body, client, request, socket, content await waitForDrain() } } + + client[kHTTP2SessionState].streams.push(h2stream) } catch (err) { h2stream.destroy(err) } finally { diff --git a/lib/core/symbols.js b/lib/core/symbols.js index b617e7a9330..5ac7557037f 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -53,5 +53,6 @@ module.exports = { kInterceptors: Symbol('dispatch interceptors'), kMaxResponseSize: Symbol('max response size'), kHTTP2Session: Symbol('http2Session'), + kHTTP2SessionState: Symbol('http2Session state'), kRawRequestHeaders: Symbol('request raw headers') } From bb83cdf0b18112391fc5ba4e1075530f17a94415 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 14 Jul 2023 11:03:22 +0200 Subject: [PATCH 33/55] refactor: header processing --- lib/core/request.js | 6 +++++- test/http2.js | 47 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/lib/core/request.js b/lib/core/request.js index 8d61508a73e..4f92a08d927 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -174,7 +174,11 @@ class Request { const keys = Object.keys(headers) for (let i = 0; i < keys.length; i++) { const key = keys[i] - processHeader(this, key, headers[key]) + const headerValue = headers[key] + processHeader(this, key, headerValue) + this[kRawRequestHeaders] = { + [key]: headerValue + } } this[kRawRequestHeaders] = headers } else if (headers != null) { diff --git a/test/http2.js b/test/http2.js index ec5972267ae..3c9068f2d4d 100644 --- a/test/http2.js +++ b/test/http2.js @@ -11,7 +11,7 @@ const pem = require('https-pem') const { Client } = require('..') -plan(15) +plan(16) test('Should support H2 connection', async t => { const body = [] @@ -60,6 +60,51 @@ test('Should support H2 connection', async t => { t.equal(Buffer.concat(body).toString('utf8'), 'hello h2!') }) +test('Should support H2 connection (headers as array)', async t => { + const body = [] + const server = createSecureServer(pem) + + server.on('stream', (stream, headers) => { + t.equal(headers['x-my-header'], 'foo') + t.equal(headers[':method'], 'GET') + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': 'hello', + ':status': 200 + }) + stream.end('hello h2!') + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.plan(6) + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await client.request({ + path: '/', + method: 'GET', + headers: ['x-my-header', 'foo'] + }) + + response.body.on('data', chunk => { + body.push(chunk) + }) + + await once(response.body, 'end') + t.equal(response.statusCode, 200) + t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') + t.equal(response.headers['x-custom-h2'], 'hello') + t.equal(Buffer.concat(body).toString('utf8'), 'hello h2!') +}) + test('Should support H2 GOAWAY (server-side)', async t => { const body = [] const server = createSecureServer(pem) From ff5202351e910bbd719ea6c589d2ebafccaaaef4 Mon Sep 17 00:00:00 2001 From: Michael Kaufman <2073135+mkaufmaner@users.noreply.github.com> Date: Mon, 17 Jul 2023 04:49:15 -0400 Subject: [PATCH 34/55] chore: http/2 benchmark (#35) Co-authored-by: Carlos Fuentes --- benchmarks/benchmark-http2.js | 306 ++++++++++++++++++++++++++++++++ benchmarks/benchmark-https.js | 319 ++++++++++++++++++++++++++++++++++ benchmarks/benchmark.js | 12 +- benchmarks/server-http2.js | 49 ++++++ benchmarks/server-https.js | 41 +++++ lib/client.js | 5 +- package.json | 2 +- 7 files changed, 727 insertions(+), 7 deletions(-) create mode 100644 benchmarks/benchmark-http2.js create mode 100644 benchmarks/benchmark-https.js create mode 100644 benchmarks/server-http2.js create mode 100644 benchmarks/server-https.js diff --git a/benchmarks/benchmark-http2.js b/benchmarks/benchmark-http2.js new file mode 100644 index 00000000000..d8555de22b5 --- /dev/null +++ b/benchmarks/benchmark-http2.js @@ -0,0 +1,306 @@ +'use strict' + +const { connect } = require('http2') +const { createSecureContext } = require('tls') +const os = require('os') +const path = require('path') +const { readFileSync } = require('fs') +const { table } = require('table') +const { Writable } = require('stream') +const { WritableStream } = require('stream/web') +const { isMainThread } = require('worker_threads') + +const { Pool, Client, fetch, Agent, setGlobalDispatcher } = require('..') + +const ca = readFileSync(path.join(__dirname, '..', 'test', 'fixtures', 'ca.pem'), 'utf8') +const servername = 'agent1' + +const iterations = (parseInt(process.env.SAMPLES, 10) || 10) + 1 +const errorThreshold = parseInt(process.env.ERROR_THRESHOLD, 10) || 3 +const connections = parseInt(process.env.CONNECTIONS, 10) || 50 +const pipelining = parseInt(process.env.PIPELINING, 10) || 10 +const parallelRequests = parseInt(process.env.PARALLEL, 10) || 100 +const headersTimeout = parseInt(process.env.HEADERS_TIMEOUT, 10) || 0 +const bodyTimeout = parseInt(process.env.BODY_TIMEOUT, 10) || 0 +const dest = {} + +if (process.env.PORT) { + dest.port = process.env.PORT + dest.url = `https://localhost:${process.env.PORT}` +} else { + dest.url = 'https://localhost' + dest.socketPath = path.join(os.tmpdir(), 'undici.sock') +} + +const httpsBaseOptions = { + ca, + servername, + protocol: 'https:', + hostname: 'localhost', + method: 'GET', + path: '/', + query: { + frappucino: 'muffin', + goat: 'scone', + pond: 'moose', + foo: ['bar', 'baz', 'bal'], + bool: true, + numberKey: 256 + }, + ...dest +} + +const http2ClientOptions = { + secureContext: createSecureContext({ ca }), + servername +} + +const undiciOptions = { + path: '/', + method: 'GET', + headersTimeout, + bodyTimeout +} + +const Class = connections > 1 ? Pool : Client +const dispatcher = new Class(httpsBaseOptions.url, { + allowH2: true, + pipelining, + connections, + connect: { + rejectUnauthorized: false, + ca, + servername + }, + ...dest +}) + +setGlobalDispatcher(new Agent({ + allowH2: true, + pipelining, + connections, + connect: { + rejectUnauthorized: false, + ca, + servername + } +})) + +class SimpleRequest { + constructor (resolve) { + this.dst = new Writable({ + write (chunk, encoding, callback) { + callback() + } + }).on('finish', resolve) + } + + onConnect (abort) { } + + onHeaders (statusCode, headers, resume) { + this.dst.on('drain', resume) + } + + onData (chunk) { + return this.dst.write(chunk) + } + + onComplete () { + this.dst.end() + } + + onError (err) { + throw err + } +} + +function makeParallelRequests (cb) { + return Promise.all(Array.from(Array(parallelRequests)).map(() => new Promise(cb))) +} + +function printResults (results) { + // Sort results by least performant first, then compare relative performances and also printing padding + let last + + const rows = Object.entries(results) + // If any failed, put on the top of the list, otherwise order by mean, ascending + .sort((a, b) => (!a[1].success ? -1 : b[1].mean - a[1].mean)) + .map(([name, result]) => { + if (!result.success) { + return [name, result.size, 'Errored', 'N/A', 'N/A'] + } + + // Calculate throughput and relative performance + const { size, mean, standardError } = result + const relative = last !== 0 ? (last / mean - 1) * 100 : 0 + + // Save the slowest for relative comparison + if (typeof last === 'undefined') { + last = mean + } + + return [ + name, + size, + `${((connections * 1e9) / mean).toFixed(2)} req/sec`, + `± ${((standardError / mean) * 100).toFixed(2)} %`, + relative > 0 ? `+ ${relative.toFixed(2)} %` : '-' + ] + }) + + console.log(results) + + // Add the header row + rows.unshift(['Tests', 'Samples', 'Result', 'Tolerance', 'Difference with slowest']) + + return table(rows, { + columns: { + 0: { + alignment: 'left' + }, + 1: { + alignment: 'right' + }, + 2: { + alignment: 'right' + }, + 3: { + alignment: 'right' + }, + 4: { + alignment: 'right' + } + }, + drawHorizontalLine: (index, size) => index > 0 && index < size, + border: { + bodyLeft: '│', + bodyRight: '│', + bodyJoin: '│', + joinLeft: '|', + joinRight: '|', + joinJoin: '|' + } + }) +} + +const experiments = { + 'http2 - request' () { + return makeParallelRequests(resolve => { + connect(dest.url, http2ClientOptions, (session) => { + const headers = { + ':path': '/', + ':method': 'GET', + ':scheme': 'https', + ':authority': `localhost:${dest.port}` + } + + const request = session.request(headers) + + request.pipe( + new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + ).on('finish', resolve) + }) + }) + }, + 'undici - pipeline' () { + return makeParallelRequests(resolve => { + dispatcher + .pipeline(undiciOptions, data => { + return data.body + }) + .end() + .pipe( + new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + ) + .on('finish', resolve) + }) + }, + 'undici - request' () { + return makeParallelRequests(resolve => { + try { + dispatcher.request(undiciOptions).then(({ body }) => { + body + .pipe( + new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + ) + .on('error', (err) => { + console.log('undici - request - dispatcher.request - body - error', err) + }) + .on('finish', () => { + resolve() + }) + }) + } catch (err) { + console.error('undici - request - dispatcher.request - requestCount', err) + } + }) + }, + 'undici - stream' () { + return makeParallelRequests(resolve => { + return dispatcher + .stream(undiciOptions, () => { + return new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + }) + .then(resolve) + }) + }, + 'undici - dispatch' () { + return makeParallelRequests(resolve => { + dispatcher.dispatch(undiciOptions, new SimpleRequest(resolve)) + }) + } +} + +if (process.env.PORT) { + // fetch does not support the socket + experiments['undici - fetch'] = () => { + return makeParallelRequests(resolve => { + fetch(dest.url, {}).then(res => { + res.body.pipeTo(new WritableStream({ write () { }, close () { resolve() } })) + }).catch(console.log) + }) + } +} + +async function main () { + const { cronometro } = await import('cronometro') + + cronometro( + experiments, + { + iterations, + errorThreshold, + print: false + }, + (err, results) => { + if (err) { + throw err + } + + console.log(printResults(results)) + dispatcher.destroy() + } + ) +} + +if (isMainThread) { + main() +} else { + module.exports = main +} diff --git a/benchmarks/benchmark-https.js b/benchmarks/benchmark-https.js new file mode 100644 index 00000000000..a364f0a0c43 --- /dev/null +++ b/benchmarks/benchmark-https.js @@ -0,0 +1,319 @@ +'use strict' + +const https = require('https') +const os = require('os') +const path = require('path') +const { readFileSync } = require('fs') +const { table } = require('table') +const { Writable } = require('stream') +const { WritableStream } = require('stream/web') +const { isMainThread } = require('worker_threads') + +const { Pool, Client, fetch, Agent, setGlobalDispatcher } = require('..') + +const ca = readFileSync(path.join(__dirname, '..', 'test', 'fixtures', 'ca.pem'), 'utf8') +const servername = 'agent1' + +const iterations = (parseInt(process.env.SAMPLES, 10) || 10) + 1 +const errorThreshold = parseInt(process.env.ERROR_TRESHOLD, 10) || 3 +const connections = parseInt(process.env.CONNECTIONS, 10) || 50 +const pipelining = parseInt(process.env.PIPELINING, 10) || 10 +const parallelRequests = parseInt(process.env.PARALLEL, 10) || 100 +const headersTimeout = parseInt(process.env.HEADERS_TIMEOUT, 10) || 0 +const bodyTimeout = parseInt(process.env.BODY_TIMEOUT, 10) || 0 +const dest = {} + +if (process.env.PORT) { + dest.port = process.env.PORT + dest.url = `https://localhost:${process.env.PORT}` +} else { + dest.url = 'https://localhost' + dest.socketPath = path.join(os.tmpdir(), 'undici.sock') +} + +const httpsBaseOptions = { + ca, + servername, + protocol: 'https:', + hostname: 'localhost', + method: 'GET', + path: '/', + query: { + frappucino: 'muffin', + goat: 'scone', + pond: 'moose', + foo: ['bar', 'baz', 'bal'], + bool: true, + numberKey: 256 + }, + ...dest +} + +const httpsNoKeepAliveOptions = { + ...httpsBaseOptions, + agent: new https.Agent({ + keepAlive: false, + maxSockets: connections, + // rejectUnauthorized: false, + ca, + servername + }) +} + +const httpsKeepAliveOptions = { + ...httpsBaseOptions, + agent: new https.Agent({ + keepAlive: true, + maxSockets: connections, + // rejectUnauthorized: false, + ca, + servername + }) +} + +const undiciOptions = { + path: '/', + method: 'GET', + headersTimeout, + bodyTimeout +} + +const Class = connections > 1 ? Pool : Client +const dispatcher = new Class(httpsBaseOptions.url, { + pipelining, + connections, + connect: { + // rejectUnauthorized: false, + ca, + servername + }, + ...dest +}) + +setGlobalDispatcher(new Agent({ + pipelining, + connections, + connect: { + // rejectUnauthorized: false, + ca, + servername + } +})) + +class SimpleRequest { + constructor (resolve) { + this.dst = new Writable({ + write (chunk, encoding, callback) { + callback() + } + }).on('finish', resolve) + } + + onConnect (abort) { } + + onHeaders (statusCode, headers, resume) { + this.dst.on('drain', resume) + } + + onData (chunk) { + return this.dst.write(chunk) + } + + onComplete () { + this.dst.end() + } + + onError (err) { + throw err + } +} + +function makeParallelRequests (cb) { + return Promise.all(Array.from(Array(parallelRequests)).map(() => new Promise(cb))) +} + +function printResults (results) { + // Sort results by least performant first, then compare relative performances and also printing padding + let last + + const rows = Object.entries(results) + // If any failed, put on the top of the list, otherwise order by mean, ascending + .sort((a, b) => (!a[1].success ? -1 : b[1].mean - a[1].mean)) + .map(([name, result]) => { + if (!result.success) { + return [name, result.size, 'Errored', 'N/A', 'N/A'] + } + + // Calculate throughput and relative performance + const { size, mean, standardError } = result + const relative = last !== 0 ? (last / mean - 1) * 100 : 0 + + // Save the slowest for relative comparison + if (typeof last === 'undefined') { + last = mean + } + + return [ + name, + size, + `${((connections * 1e9) / mean).toFixed(2)} req/sec`, + `± ${((standardError / mean) * 100).toFixed(2)} %`, + relative > 0 ? `+ ${relative.toFixed(2)} %` : '-' + ] + }) + + console.log(results) + + // Add the header row + rows.unshift(['Tests', 'Samples', 'Result', 'Tolerance', 'Difference with slowest']) + + return table(rows, { + columns: { + 0: { + alignment: 'left' + }, + 1: { + alignment: 'right' + }, + 2: { + alignment: 'right' + }, + 3: { + alignment: 'right' + }, + 4: { + alignment: 'right' + } + }, + drawHorizontalLine: (index, size) => index > 0 && index < size, + border: { + bodyLeft: '│', + bodyRight: '│', + bodyJoin: '│', + joinLeft: '|', + joinRight: '|', + joinJoin: '|' + } + }) +} + +const experiments = { + 'https - no keepalive' () { + return makeParallelRequests(resolve => { + https.get(httpsNoKeepAliveOptions, res => { + res + .pipe( + new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + ) + .on('finish', resolve) + }) + }) + }, + 'https - keepalive' () { + return makeParallelRequests(resolve => { + https.get(httpsKeepAliveOptions, res => { + res + .pipe( + new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + ) + .on('finish', resolve) + }) + }) + }, + 'undici - pipeline' () { + return makeParallelRequests(resolve => { + dispatcher + .pipeline(undiciOptions, data => { + return data.body + }) + .end() + .pipe( + new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + ) + .on('finish', resolve) + }) + }, + 'undici - request' () { + return makeParallelRequests(resolve => { + dispatcher.request(undiciOptions).then(({ body }) => { + body + .pipe( + new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + ) + .on('finish', resolve) + }) + }) + }, + 'undici - stream' () { + return makeParallelRequests(resolve => { + return dispatcher + .stream(undiciOptions, () => { + return new Writable({ + write (chunk, encoding, callback) { + callback() + } + }) + }) + .then(resolve) + }) + }, + 'undici - dispatch' () { + return makeParallelRequests(resolve => { + dispatcher.dispatch(undiciOptions, new SimpleRequest(resolve)) + }) + } +} + +if (process.env.PORT) { + // fetch does not support the socket + experiments['undici - fetch'] = () => { + return makeParallelRequests(resolve => { + fetch(dest.url, {}).then(res => { + res.body.pipeTo(new WritableStream({ write () { }, close () { resolve() } })) + }).catch(console.log) + }) + } +} + +async function main () { + const { cronometro } = await import('cronometro') + + cronometro( + experiments, + { + iterations, + errorThreshold, + print: false + }, + (err, results) => { + if (err) { + throw err + } + + console.log(printResults(results)) + dispatcher.destroy() + } + ) +} + +if (isMainThread) { + main() +} else { + module.exports = main +} diff --git a/benchmarks/benchmark.js b/benchmarks/benchmark.js index 4cf129f7b98..5bf3d2ede4f 100644 --- a/benchmarks/benchmark.js +++ b/benchmarks/benchmark.js @@ -73,7 +73,13 @@ const dispatcher = new Class(httpBaseOptions.url, { ...dest }) -setGlobalDispatcher(new Agent({ pipelining, connections })) +setGlobalDispatcher(new Agent({ + pipelining, + connections, + connect: { + rejectUnauthorized: false + } +})) class SimpleRequest { constructor (resolve) { @@ -84,7 +90,7 @@ class SimpleRequest { }).on('finish', resolve) } - onConnect (abort) {} + onConnect (abort) { } onHeaders (statusCode, headers, resume) { this.dst.on('drain', resume) @@ -260,7 +266,7 @@ if (process.env.PORT) { experiments['undici - fetch'] = () => { return makeParallelRequests(resolve => { fetch(dest.url).then(res => { - res.body.pipeTo(new WritableStream({ write () {}, close () { resolve() } })) + res.body.pipeTo(new WritableStream({ write () { }, close () { resolve() } })) }).catch(console.log) }) } diff --git a/benchmarks/server-http2.js b/benchmarks/server-http2.js new file mode 100644 index 00000000000..0be99cd2fd7 --- /dev/null +++ b/benchmarks/server-http2.js @@ -0,0 +1,49 @@ +'use strict' + +const { unlinkSync, readFileSync } = require('fs') +const { createSecureServer } = require('http2') +const os = require('os') +const path = require('path') +const cluster = require('cluster') + +const key = readFileSync(path.join(__dirname, '..', 'test', 'fixtures', 'key.pem'), 'utf8') +const cert = readFileSync(path.join(__dirname, '..', 'test', 'fixtures', 'cert.pem'), 'utf8') + +const socketPath = path.join(os.tmpdir(), 'undici.sock') + +const port = process.env.PORT || socketPath +const timeout = parseInt(process.env.TIMEOUT, 10) || 1 +const workers = parseInt(process.env.WORKERS) || os.cpus().length + +const sessionTimeout = 600e3 // 10 minutes + +if (cluster.isPrimary) { + try { + unlinkSync(socketPath) + } catch (_) { + // Do nothing if the socket does not exist + } + + for (let i = 0; i < workers; i++) { + cluster.fork() + } +} else { + const buf = Buffer.alloc(64 * 1024, '_') + const server = createSecureServer( + { + key, + cert, + allowHTTP1: true, + sessionTimeout + }, + (req, res) => { + setTimeout(() => { + res.end(buf) + }, timeout) + } + ) + + server.keepAliveTimeout = 600e3 + + server.listen(port) +} diff --git a/benchmarks/server-https.js b/benchmarks/server-https.js new file mode 100644 index 00000000000..f0275d9cbcc --- /dev/null +++ b/benchmarks/server-https.js @@ -0,0 +1,41 @@ +'use strict' + +const { unlinkSync, readFileSync } = require('fs') +const { createServer } = require('https') +const os = require('os') +const path = require('path') +const cluster = require('cluster') + +const key = readFileSync(path.join(__dirname, '..', 'test', 'fixtures', 'key.pem'), 'utf8') +const cert = readFileSync(path.join(__dirname, '..', 'test', 'fixtures', 'cert.pem'), 'utf8') + +const socketPath = path.join(os.tmpdir(), 'undici.sock') + +const port = process.env.PORT || socketPath +const timeout = parseInt(process.env.TIMEOUT, 10) || 1 +const workers = parseInt(process.env.WORKERS) || os.cpus().length + +if (cluster.isPrimary) { + try { + unlinkSync(socketPath) + } catch (_) { + // Do nothing if the socket does not exist + } + + for (let i = 0; i < workers; i++) { + cluster.fork() + } +} else { + const buf = Buffer.alloc(64 * 1024, '_') + const server = createServer({ + key, + cert, + keepAliveTimeout: 600e3 + }, (req, res) => { + setTimeout(() => { + res.end(buf) + }, timeout) + }) + + server.listen(port) +} diff --git a/lib/client.js b/lib/client.js index 543c6925fcb..46d374d9213 100644 --- a/lib/client.js +++ b/lib/client.js @@ -83,8 +83,7 @@ const { HTTP2_HEADER_METHOD, HTTP2_HEADER_PATH, HTTP2_HEADER_CONTENT_LENGTH, - HTTP2_HEADER_EXPECT, - HTTP2_HEADER_STATUS + HTTP2_HEADER_EXPECT } } = http2 @@ -1158,7 +1157,7 @@ async function connect (client) { }) if (client.destroyed) { - util.destroy(socket.on('error', () => {}), new ClientDestroyedError()) + util.destroy(socket.on('error', () => { }), new ClientDestroyedError()) return } diff --git a/package.json b/package.json index 598a78654a9..bb56ffab396 100644 --- a/package.json +++ b/package.json @@ -133,4 +133,4 @@ "dependencies": { "busboy": "^1.6.0" } -} +} \ No newline at end of file From 5b7f6f5d2003fa3bbf46f78061e88a2e528fc450 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 19 Jul 2023 11:00:07 +0200 Subject: [PATCH 35/55] refactor: adjust accordingly to review --- lib/client.js | 9 ++++----- lib/core/request.js | 2 ++ test/http2.js | 13 ++++++++++++- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/client.js b/lib/client.js index 46d374d9213..21cb36ec6cd 100644 --- a/lib/client.js +++ b/lib/client.js @@ -83,7 +83,8 @@ const { HTTP2_HEADER_METHOD, HTTP2_HEADER_PATH, HTTP2_HEADER_CONTENT_LENGTH, - HTTP2_HEADER_EXPECT + HTTP2_HEADER_EXPECT, + HTTP2_HEADER_STATUS } } = http2 @@ -1365,9 +1366,7 @@ function _resume (client, sync) { return } - if ((socket.destroyed || socket[kWriting] || socket[kReset] || socket[kBlocking]) || - (client[kHTTP2Session] && client[kHTTP2Session].destroyed)) { - // TODO(HTTP/2): what if exceeds max concurrent streams or can't accept new + if ((socket.destroyed || socket[kWriting] || socket[kReset] || socket[kBlocking])) { return } @@ -1777,7 +1776,7 @@ function writeH2 (client, session, request) { } stream.on('response', headers => { - if (request.onHeaders(Number(headers[':status']), headers, stream.resume.bind(stream), '') === false) { + if (request.onHeaders(Number(headers[HTTP2_HEADER_STATUS]), headers, stream.resume.bind(stream), '') === false) { stream.pause() } }) diff --git a/lib/core/request.js b/lib/core/request.js index 4f92a08d927..0fb2eb75e57 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -158,6 +158,8 @@ class Request { this.expectContinue = expectContinue != null ? expectContinue : false + this[kRawRequestHeaders] = null + if (Array.isArray(headers)) { if (headers.length % 2 !== 0) { throw new InvalidArgumentError('headers array must be even') diff --git a/test/http2.js b/test/http2.js index 3c9068f2d4d..62f0dd34d29 100644 --- a/test/http2.js +++ b/test/http2.js @@ -464,7 +464,7 @@ test('Dispatcher#destroy', async t => { } }) - t.plan(3) + t.plan(4) t.teardown(server.close.bind(server)) promises.push( @@ -497,6 +497,16 @@ test('Dispatcher#destroy', async t => { }) ) + promises.push( + client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) + ) + await client.destroy() const results = await Promise.allSettled(promises) @@ -504,6 +514,7 @@ test('Dispatcher#destroy', async t => { t.equal(results[0].status, 'rejected') t.equal(results[1].status, 'rejected') t.equal(results[2].status, 'rejected') + t.equal(results[3].status, 'rejected') }) test('Should handle h2 request with body (string or buffer) - dispatch', t => { From a8d26652f494eae28ad4447b528834b21c78c680 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 21 Jul 2023 18:26:12 +0200 Subject: [PATCH 36/55] fix: add missing error handler for socket --- lib/client.js | 30 +++++++++++++++++------------- lib/core/util.js | 7 +------ 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/lib/client.js b/lib/client.js index 21cb36ec6cd..0eadf690119 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1020,7 +1020,7 @@ function onSocketError (err) { // On Mac OS, we get an ECONNRESET even if there is a full body to be forwarded // to the user. - if (err.code === 'ECONNRESET' && parser.statusCode && !parser.shouldKeepAlive) { + if (err.code === 'ECONNRESET' && parser && parser.statusCode && !parser.shouldKeepAlive) { // We treat all incoming data so for as a valid response. parser.onMessageComplete() return @@ -1054,7 +1054,7 @@ function onError (client, err) { function onSocketEnd () { const { [kParser]: parser } = this - if (parser.statusCode && !parser.shouldKeepAlive) { + if (parser && parser.statusCode && !parser.shouldKeepAlive) { // We treat all incoming data so far as a valid response. parser.onMessageComplete() return @@ -1064,12 +1064,12 @@ function onSocketEnd () { } function onSocketClose () { - const { [kClient]: client } = this + const { [kClient]: client, [kHTTP2Session]: session, [kParser]: parser } = this - if (client[kHTTP2Session] == null) { - if (!this[kError] && this[kParser].statusCode && !this[kParser].shouldKeepAlive) { + if (session == null && parser) { + if (!this[kError] && parser.statusCode && !parser.shouldKeepAlive) { // We treat all incoming data so far as a valid response. - this[kParser].onMessageComplete() + parser.onMessageComplete() } this[kParser].destroy() @@ -1175,13 +1175,15 @@ async function connect (client) { session[kError] = null session[kClient] = client + session[kSocket] = socket session.on('error', onHttp2SessionError) session.on('goaway', onHTTP2GoAway.bind(client)) session.on('close', onSocketClose) session.unref() - + client[kHTTP2Session] = session client[kHTTP2SessionState].streams = new FixedQueue() + socket[kHTTP2Session] = session } else { if (!llhttpInstance) { llhttpInstance = await llhttpPromise @@ -1194,16 +1196,18 @@ async function connect (client) { socket[kBlocking] = false socket[kError] = null socket[kParser] = new Parser(client, socket, llhttpInstance) - socket[kClient] = client socket[kCounter] = 0 socket[kMaxRequests] = client[kMaxRequests] - socket - .on('error', onSocketError) - .on('readable', onSocketReadable) - .on('end', onSocketEnd) - .on('close', onSocketClose) } + socket[kClient] = client + + socket + .on('error', onSocketError) + .on('readable', onSocketReadable) + .on('end', onSocketEnd) + .on('close', onSocketClose) + client[kSocket] = socket if (channels.connected.hasSubscribers) { diff --git a/lib/core/util.js b/lib/core/util.js index 8972ff3857f..b1eb6501f78 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -200,12 +200,7 @@ function destroy (stream, err) { stream.socket = null } - // HTTP/2 - It causes to throw uncaught exceptions due to the - // error passed down the socket.destroy - const isHTTP2 = stream.alpnProtocol === 'h2' && - Object.getPrototypeOf(err).constructor === ClientDestroyedError - - stream.destroy(!isHTTP2 ? err : null) + stream.destroy(err) } else if (err) { process.nextTick((stream, err) => { stream.emit('error', err) From 0cadb9f1fa8acda838b1485d329061252abf6538 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 26 Jul 2023 14:03:26 +0200 Subject: [PATCH 37/55] refactor: headers handling --- lib/client.js | 14 ++++++--- lib/core/request.js | 76 +++++++++++++++++++++++++++++++++------------ lib/core/symbols.js | 3 +- test/http2.js | 5 +-- 4 files changed, 71 insertions(+), 27 deletions(-) diff --git a/lib/client.js b/lib/client.js index 0eadf690119..556cdc33a96 100644 --- a/lib/client.js +++ b/lib/client.js @@ -75,7 +75,8 @@ const { kHost, kHTTP2Session, kHTTP2SessionState, - kRawRequestHeaders + kHTTP2BuildRequest, + kHTTP2CopyHeaders } = require('./core/symbols') const { constants: { @@ -332,7 +333,9 @@ class Client extends DispatcherBase { [kDispatch] (opts, handler) { const origin = opts.origin || this[kUrl].origin - const request = new Request(origin, opts, handler) + let request + if (this[kHTTP2Session]) request = Request[kHTTP2BuildRequest](origin, opts, handler) + else request = new Request(origin, opts, handler) this[kQueue].push(request) if (this[kResuming]) { @@ -1587,7 +1590,11 @@ function write (client, request) { } function writeH2 (client, session, request) { - const { body, method, path, host, upgrade, expectContinue, signal } = request + const { body, method, path, host, upgrade, expectContinue, signal, headers: reqHeaders } = request + + let headers + if (typeof reqHeaders === 'string') headers = Request[kHTTP2CopyHeaders](reqHeaders.trim()) + else headers = reqHeaders if (upgrade) { errorRequest(client, request, new Error('Upgrade not supported for H2')) @@ -1612,7 +1619,6 @@ function writeH2 (client, session, request) { } let stream - const headers = { ...request[kRawRequestHeaders] } headers[HTTP2_HEADER_AUTHORITY] = host || client[kHost] headers[HTTP2_HEADER_PATH] = path diff --git a/lib/core/request.js b/lib/core/request.js index 0fb2eb75e57..2cc02a07d0d 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -5,7 +5,7 @@ const { NotSupportedError } = require('./errors') const assert = require('assert') -const { kRawRequestHeaders } = require('./symbols') +const { kHTTP2BuildRequest, kHTTP2CopyHeaders } = require('./symbols') const util = require('./util') // tokenRegExp and headerCharRegex have been lifted from @@ -156,33 +156,22 @@ class Request { this.headers = '' + // Only for H2 this.expectContinue = expectContinue != null ? expectContinue : false - this[kRawRequestHeaders] = null - if (Array.isArray(headers)) { if (headers.length % 2 !== 0) { throw new InvalidArgumentError('headers array must be even') } for (let i = 0; i < headers.length; i += 2) { - const headerKey = headers[i] - const headerValue = headers[i + 1] - processHeader(this, headerKey, headerValue) - this[kRawRequestHeaders] = { - [headerKey]: headerValue - } + processHeader(this, headers[i], headers[i + 1]) } } else if (headers && typeof headers === 'object') { const keys = Object.keys(headers) for (let i = 0; i < keys.length; i++) { const key = keys[i] - const headerValue = headers[key] - processHeader(this, key, headerValue) - this[kRawRequestHeaders] = { - [key]: headerValue - } + processHeader(this, key, headers[key]) } - this[kRawRequestHeaders] = headers } else if (headers != null) { throw new InvalidArgumentError('headers must be an object or an array') } @@ -289,13 +278,54 @@ class Request { return this[kHandler].onError(error) } + // TODO: adjust to support H2 addHeader (key, value) { processHeader(this, key, value) return this } + + static [kHTTP2BuildRequest] (origin, opts, handler) { + const headers = opts.headers + opts = { ...opts, headers: null } + + const request = new Request(origin, opts, handler) + + if (Array.isArray(headers)) { + if (headers.length % 2 !== 0) { + throw new InvalidArgumentError('headers array must be even') + } + for (let i = 0; i < headers.length; i += 2) { + processHeader(request, headers[i], headers[i + 1], true) + } + } else if (headers && typeof headers === 'object') { + const keys = Object.keys(headers) + for (let i = 0; i < keys.length; i++) { + const key = keys[i] + processHeader(request, key, headers[key], true) + } + } else if (headers != null) { + throw new InvalidArgumentError('headers must be an object or an array') + } + + return request + } + + static [kHTTP2CopyHeaders] (raw) { + const rawHeaders = raw.split('\r\n') + + const headers = {} + for (const header of rawHeaders) { + const [key, value] = header.split(': ') + + if (headers[key]) headers[key] += `,${value}` + else headers[key] = value + } + + return headers + } } -function processHeaderValue (key, val) { +function processHeaderValue (key, val, skipAppend) { if (val && typeof val === 'object') { throw new InvalidArgumentError(`invalid ${key} header`) } @@ -306,10 +336,10 @@ function processHeaderValue (key, val) { throw new InvalidArgumentError(`invalid ${key} header`) } - return `${key}: ${val}\r\n` + return skipAppend ? val : `${key}: ${val}\r\n` } -function processHeader (request, key, val) { +function processHeader (request, key, val, skipAppend = false) { if (val && (typeof val === 'object' && !Array.isArray(val))) { throw new InvalidArgumentError(`invalid ${key} header`) } else if (val === undefined) { @@ -377,10 +407,16 @@ function processHeader (request, key, val) { } else { if (Array.isArray(val)) { for (let i = 0; i < val.length; i++) { - request.headers += processHeaderValue(key, val[i]) + if (skipAppend) { + if (request.headers[key]) request.headers[key] += `,${processHeaderValue(key, val[i], skipAppend)}` + else request.headers[key] = processHeaderValue(key, val[i], skipAppend) + } else { + request.headers += processHeaderValue(key, val[i]) + } } } else { - request.headers += processHeaderValue(key, val) + if (skipAppend) request.headers[key] = processHeaderValue(key, val, skipAppend) + else request.headers += processHeaderValue(key, val) } } } diff --git a/lib/core/symbols.js b/lib/core/symbols.js index 5ac7557037f..fc258d8efbb 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -54,5 +54,6 @@ module.exports = { kMaxResponseSize: Symbol('max response size'), kHTTP2Session: Symbol('http2Session'), kHTTP2SessionState: Symbol('http2Session state'), - kRawRequestHeaders: Symbol('request raw headers') + kHTTP2BuildRequest: Symbol('http2 build request'), + kHTTP2CopyHeaders: Symbol('http2 copy headers') } diff --git a/test/http2.js b/test/http2.js index 62f0dd34d29..0b72ebaa1fa 100644 --- a/test/http2.js +++ b/test/http2.js @@ -66,6 +66,7 @@ test('Should support H2 connection (headers as array)', async t => { server.on('stream', (stream, headers) => { t.equal(headers['x-my-header'], 'foo') + t.equal(headers['x-my-drink'], 'coffee,tea') t.equal(headers[':method'], 'GET') stream.respond({ 'content-type': 'text/plain; charset=utf-8', @@ -84,14 +85,14 @@ test('Should support H2 connection (headers as array)', async t => { } }) - t.plan(6) + t.plan(7) t.teardown(server.close.bind(server)) t.teardown(client.close.bind(client)) const response = await client.request({ path: '/', method: 'GET', - headers: ['x-my-header', 'foo'] + headers: ['x-my-header', 'foo', 'x-my-drink', ['coffee', 'tea']] }) response.body.on('data', chunk => { From c89c0035ab99bd1989fa0b7ea2ef5fbd30114aee Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 28 Jul 2023 19:41:23 +0200 Subject: [PATCH 38/55] feat: initial concurrent stream support --- lib/client.js | 72 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 26 deletions(-) diff --git a/lib/client.js b/lib/client.js index 556cdc33a96..492880f552d 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1183,7 +1183,7 @@ async function connect (client) { session.on('goaway', onHTTP2GoAway.bind(client)) session.on('close', onSocketClose) session.unref() - + client[kHTTP2Session] = session client[kHTTP2SessionState].streams = new FixedQueue() socket[kHTTP2Session] = session @@ -1619,10 +1619,19 @@ function writeH2 (client, session, request) { } let stream + const h2State = client[kHTTP2SessionState]; + const shouldCreateNewStream = h2State.streams.isEmpty() || + h2State.openStreams < h2State.maxStreams + headers[HTTP2_HEADER_AUTHORITY] = host || client[kHost] headers[HTTP2_HEADER_PATH] = path if (method === 'CONNECT') { + if (!shouldCreateNewStream) { + errorRequest(client, request, new Error('H2: Max streams reached')) + return false + } + session.ref() // we are already connected, streams are pending, first request // will create a new stream. We trigger a request to create the stream and wait until @@ -1668,6 +1677,7 @@ function writeH2 (client, session, request) { } let contentLength = util.bodyLength(body) + if (contentLength == null) { contentLength = request.contentLength } @@ -1696,8 +1706,6 @@ function writeH2 (client, session, request) { } session.ref() - const shouldCreateNewStream = client[kHTTP2SessionState].streams.isEmpty() || - client[kHTTP2SessionState].openStreams < client[kHTTP2SessionState].maxStreams if (expectContinue) { headers[HTTP2_HEADER_EXPECT] = '100-continue' @@ -1731,7 +1739,7 @@ function writeH2 (client, session, request) { request.onBodySent(body) request.onRequestSent() - client[kHTTP2SessionState].streams.push(stream) + h2State.streams.push(stream) } else if (util.isBlobLike(body)) { if (typeof body.stream === 'function') { writeIterable({ @@ -1785,39 +1793,51 @@ function writeH2 (client, session, request) { } } - stream.on('response', headers => { - if (request.onHeaders(Number(headers[HTTP2_HEADER_STATUS]), headers, stream.resume.bind(stream), '') === false) { - stream.pause() - } - }) + stream.on('readable', onReadable) - stream.on('data', chunk => { - if (request.onData(chunk) === false) { + stream.once('response', headers => { + if (request.onHeaders(Number(headers[HTTP2_HEADER_STATUS]), headers, stream.resume.bind(stream), '') === false) { stream.pause() } }) - stream.on('trailers', headers => { - // TODO(HTTP/2): Suppor trailers - }) - stream.on('end', () => { - request.onComplete([]) + stream.once('close', () => { + h2State.openStreams -= 1 + // TODO(HTTP/2): unref only if current streams count is 0 + if (h2State.openStreams === 0) session.unref() }) - stream.on('aborted', () => { - // TODO(HTTP/2): Support aborted - }) + // stream.on('aborted', () => { + // // TODO(HTTP/2): Support aborted + // }) - stream.on('timeout', () => { - // TODO(HTTP/2): Support timeout - }) + // stream.on('timeout', () => { + // // TODO(HTTP/2): Support timeout + // }) - stream.on('close', () => { - // TODO(HTTP/2): unref only if current streams count is 0 - session.unref() - }) + // stream.on('trailers', headers => { + // // TODO(HTTP/2): Suppor trailers + // }) return true + + function onReadable () { + let chunk + let status = true + + // We read until we receive a backpressure signal or reach the end of the stream + // eslint-disable-next-line + while ((chunk = this.read()) && (status = request.onData(chunk)) !== false) {} + + if (chunk == null && status !== false) { + request.onComplete([]) + this.off('readable', onReadable) + } + + // Destroyed means either ends closed te connection, here we check for server side + // termination + if (!this.destroyed) h2State.streams.push(this) + } } function writeStream ({ h2Stream, body, client, request, socket, contentLength, header, expectsPayload }) { From d72aa31ac5f77902568a621dd8e375a62bfb698e Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 28 Jul 2023 19:42:33 +0200 Subject: [PATCH 39/55] fix: lint --- lib/client.js | 2 +- lib/core/util.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/client.js b/lib/client.js index 492880f552d..104a3870acb 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1619,7 +1619,7 @@ function writeH2 (client, session, request) { } let stream - const h2State = client[kHTTP2SessionState]; + const h2State = client[kHTTP2SessionState] const shouldCreateNewStream = h2State.streams.isEmpty() || h2State.openStreams < h2State.maxStreams diff --git a/lib/core/util.js b/lib/core/util.js index b1eb6501f78..b77db3c59ba 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -5,7 +5,7 @@ const { kDestroyed, kBodyUsed } = require('./symbols') const { IncomingMessage } = require('http') const stream = require('stream') const net = require('net') -const { InvalidArgumentError, ClientDestroyedError } = require('./errors') +const { InvalidArgumentError } = require('./errors') const { Blob } = require('buffer') const nodeUtil = require('util') const { stringify } = require('querystring') From 16e1334ff60ea2f74a35b638047e44c1bc5fa721 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 11 Aug 2023 14:02:45 +0200 Subject: [PATCH 40/55] refactor: adjust several pieces --- lib/client.js | 220 ++++++++++++++++++++++++++++++-------------------- test/http2.js | 6 ++ 2 files changed, 137 insertions(+), 89 deletions(-) diff --git a/lib/client.js b/lib/client.js index 104a3870acb..e14b91838e8 100644 --- a/lib/client.js +++ b/lib/client.js @@ -270,8 +270,8 @@ class Client extends DispatcherBase { // HTTP/2 this[kHTTP2Session] = null this[kHTTP2SessionState] = { - streams: null, // Fixed queue of streams - openStreams: 0, + // streams: null, // Fixed queue of streams - For future support of `push` + openStreams: 0, // Keep track of them to decide wether or not unref the session maxStreams: 100 // Max peerConcurrentStreams for a Node h2 server } this[kHost] = `${this[kUrl].hostname}${this[kUrl].port ? `:${this[kUrl].port}` : ''}` @@ -385,7 +385,15 @@ class Client extends DispatcherBase { } if (this[kHTTP2Session] != null) { - this[kHTTP2Session].destroy() + const { maxStreams } = this[kHTTP2SessionState] + + util.destroy(this[kHTTP2Session], err) + this[kHTTP2Session] = null + this[kHTTP2SessionState] = { + maxStreams, + openStreams: 0 + // streams: null // Fixed queue of streams + } } if (!this[kSocket]) { @@ -402,29 +410,59 @@ class Client extends DispatcherBase { function onHttp2SessionError (err) { assert(err.code !== 'ERR_TLS_CERT_ALTNAME_INVALID') - this[kError] = err + this[kSocket][kError] = err onError(this[kClient], err) } +function onHttp2FrameError (type, code, id) { + const err = new InformationalError(`HTTP/2: "frameError" received - type ${type}, code ${code}`) + + if (id === 0) { + this[kSocket][kError] = err + onError(this[kClient], err) + } +} + +function onHttp2SessionEnd () { + util.destroy(this, new SocketError('other side closed')) + util.destroy(this[kSocket], new SocketError('other side closed')) +} + function onHTTP2GoAway (code) { - // TODO; iterate over streams and - // shutdown each one of them before - // fully closing and destroying the Socket - // NOTE: The last ID of the frame received is the last processed - // stream. Remaining streams should finish its work before - // closing them. New streams cannot be created - // When started on client side, the same logic applies. - // Remaining streams should finish its work before - // closing the connection. No new streams can be opened. - this.emit('disconnect', - this[kUrl], - [this], - new InformationalError(`HTTP/2: "GOAWAY" frame received with code ${code}`) + const client = this[kClient] + const err = new InformationalError(`HTTP/2: "GOAWAY" frame received with code ${code}`) + client[kSocket] = null + client[kHTTP2Session] = null + + if (client.destroyed) { + assert(this[kPending] === 0) + + // Fail entire queue. + const requests = client[kQueue].splice(client[kRunningIdx]) + for (let i = 0; i < requests.length; i++) { + const request = requests[i] + errorRequest(this, request, err) + } + } else if (client[kRunning] > 0) { + // Fail head of pipeline. + const request = client[kQueue][client[kRunningIdx]] + client[kQueue][client[kRunningIdx]++] = null + + errorRequest(client, request, err) + } + + client[kPendingIdx] = client[kRunningIdx] + + assert(client[kRunning] === 0) + + client.emit('disconnect', + client[kUrl], + [client], + err ) - // TODO: Gracefully shutdown each stream - this.close() + resume(client) } const constants = require('./llhttp/constants') @@ -1176,16 +1214,16 @@ async function connect (client) { peerMaxConcurrentStreams: client[kHTTP2SessionState].maxStreams }) - session[kError] = null session[kClient] = client session[kSocket] = socket session.on('error', onHttp2SessionError) - session.on('goaway', onHTTP2GoAway.bind(client)) + session.on('frameError', onHttp2FrameError) + session.on('end', onHttp2SessionEnd) + session.on('goaway', onHTTP2GoAway) session.on('close', onSocketClose) session.unref() client[kHTTP2Session] = session - client[kHTTP2SessionState].streams = new FixedQueue() socket[kHTTP2Session] = session } else { if (!llhttpInstance) { @@ -1197,13 +1235,13 @@ async function connect (client) { socket[kWriting] = false socket[kReset] = false socket[kBlocking] = false - socket[kError] = null socket[kParser] = new Parser(client, socket, llhttpInstance) socket[kCounter] = 0 socket[kMaxRequests] = client[kMaxRequests] } socket[kClient] = client + socket[kError] = null socket .on('error', onSocketError) @@ -1620,8 +1658,7 @@ function writeH2 (client, session, request) { let stream const h2State = client[kHTTP2SessionState] - const shouldCreateNewStream = h2State.streams.isEmpty() || - h2State.openStreams < h2State.maxStreams + const shouldCreateNewStream = h2State.openStreams < h2State.maxStreams headers[HTTP2_HEADER_AUTHORITY] = host || client[kHost] headers[HTTP2_HEADER_PATH] = path @@ -1640,15 +1677,18 @@ function writeH2 (client, session, request) { if (stream.id && !stream.pending) { request.onUpgrade(null, null, stream) + ++h2State.openStreams } else { stream.once('ready', () => { request.onUpgrade(null, null, stream) + ++h2State.openStreams }) } - stream.on('close', () => { + stream.once('close', () => { + h2State.openStreams -= 1 // TODO(HTTP/2): unref only if current streams count is 0 - session.unref() + if (h2State.openStreams === 0) session.unref() }) return true @@ -1727,6 +1767,64 @@ function writeH2 (client, session, request) { writeBodyH2() } + // Increment counter as we have new several streams open + ++h2State.openStreams + + stream.once('response', headers => { + if (request.onHeaders(Number(headers[HTTP2_HEADER_STATUS]), headers, stream.resume.bind(stream), '') === false) { + stream.pause() + } + }) + + stream.once('end', () => { + request.onComplete([]) + }) + + stream.on('data', (chunk) => { + if (request.onData(chunk) === false) stream.pause() + }) + + stream.once('close', () => { + h2State.openStreams -= 1 + // TODO(HTTP/2): unref only if current streams count is 0 + if (h2State.openStreams === 0) session.unref() + }) + + stream.once('error', function (err) { + if (client[kHTTP2Session] && !client[kHTTP2Session].destroyed && !this.closed && !this.destroyed) { + h2State.streams -= 1 + util.destroy(stream, err) + } + }) + + stream.once('frameError', (type, code) => { + const err = new InformationalError(`HTTP/2: "frameError" received - type ${type}, code ${code}`) + errorRequest(client, request, err) + + if (client[kHTTP2Session] && !client[kHTTP2Session].destroyed && !this.closed && !this.destroyed) { + h2State.streams -= 1 + util.destroy(stream, err) + } + }) + + // stream.on('aborted', () => { + // // TODO(HTTP/2): Support aborted + // }) + + // stream.on('timeout', () => { + // // TODO(HTTP/2): Support timeout + // }) + + // stream.on('push', headers => { + // // TODO(HTTP/2): Suppor push + // }) + + // stream.on('trailers', headers => { + // // TODO(HTTP/2): Support trailers + // }) + + return true + function writeBodyH2 () { /* istanbul ignore else: assertion */ if (!body) { @@ -1738,8 +1836,6 @@ function writeH2 (client, session, request) { stream.uncork() request.onBodySent(body) request.onRequestSent() - - h2State.streams.push(stream) } else if (util.isBlobLike(body)) { if (typeof body.stream === 'function') { writeIterable({ @@ -1765,8 +1861,6 @@ function writeH2 (client, session, request) { }) } } else if (util.isStream(body)) { - // TODO: adapt the HTTP2Stream to API compatible with current undici - // expectations from a stream writeStream({ body, client, @@ -1774,7 +1868,7 @@ function writeH2 (client, session, request) { contentLength, expectsPayload, socket: client[kSocket], - h2Stream: stream, + h2stream: stream, header: '' }) } else if (util.isIterable(body)) { @@ -1792,66 +1886,22 @@ function writeH2 (client, session, request) { assert(false) } } - - stream.on('readable', onReadable) - - stream.once('response', headers => { - if (request.onHeaders(Number(headers[HTTP2_HEADER_STATUS]), headers, stream.resume.bind(stream), '') === false) { - stream.pause() - } - }) - - stream.once('close', () => { - h2State.openStreams -= 1 - // TODO(HTTP/2): unref only if current streams count is 0 - if (h2State.openStreams === 0) session.unref() - }) - - // stream.on('aborted', () => { - // // TODO(HTTP/2): Support aborted - // }) - - // stream.on('timeout', () => { - // // TODO(HTTP/2): Support timeout - // }) - - // stream.on('trailers', headers => { - // // TODO(HTTP/2): Suppor trailers - // }) - - return true - - function onReadable () { - let chunk - let status = true - - // We read until we receive a backpressure signal or reach the end of the stream - // eslint-disable-next-line - while ((chunk = this.read()) && (status = request.onData(chunk)) !== false) {} - - if (chunk == null && status !== false) { - request.onComplete([]) - this.off('readable', onReadable) - } - - // Destroyed means either ends closed te connection, here we check for server side - // termination - if (!this.destroyed) h2State.streams.push(this) - } } -function writeStream ({ h2Stream, body, client, request, socket, contentLength, header, expectsPayload }) { +function writeStream ({ h2stream, body, client, request, socket, contentLength, header, expectsPayload }) { assert(contentLength !== 0 || client[kRunning] === 0, 'stream body cannot be pipelined') if (socket.alpnProtocol === 'h2') { // For HTTP/2, is enough to pipe the stream const pipe = pipeline( body, - h2Stream, + h2stream, (err) => { if (err) { util.destroy(body, err) - util.destroy(h2Stream, err) + util.destroy(h2stream, err) + } else { + request.onRequestSent() } } ) @@ -1860,12 +1910,8 @@ function writeStream ({ h2Stream, body, client, request, socket, contentLength, pipe.once('end', () => { pipe.removeListener('data', onPipeData) util.destroy(pipe) - request.onRequestSent() - client[kHTTP2SessionState].streams.push(h2Stream) }) - body.once('end', () => util.destroy(body)) - function onPipeData (chunk) { request.onBodySent(chunk) } @@ -1968,8 +2014,6 @@ async function writeBlob ({ h2stream, body, client, request, socket, contentLeng h2stream.cork() h2stream.write(buffer) h2stream.uncork() - - client[kHTTP2SessionState].streams.push(h2stream) } else { socket.cork() socket.write(`${header}content-length: ${contentLength}\r\n\r\n`, 'latin1') @@ -2028,8 +2072,6 @@ async function writeIterable ({ h2stream, body, client, request, socket, content await waitForDrain() } } - - client[kHTTP2SessionState].streams.push(h2stream) } catch (err) { h2stream.destroy(err) } finally { diff --git a/test/http2.js b/test/http2.js index 0b72ebaa1fa..36b83c5d780 100644 --- a/test/http2.js +++ b/test/http2.js @@ -412,6 +412,12 @@ test('Dispatcher#Connect', t => { t.notOk(socket.closed) }) + // We need to handle the error event although + // is not controlled by Undici, the fact that a session + // is destroyed and destroys subsequent streams, causes + // unhandled errors to surface if not handling this event. + socket.on('error', () => {}) + socket.once('end', () => { t.equal(requestBody, expectedBody) t.equal(result, 'hello h2!') From 92be0000ea61da7411d35add16de371acec0a699 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 16 Aug 2023 11:01:46 +0200 Subject: [PATCH 41/55] fix: support h2 headers for fetch --- lib/api/api-connect.js | 2 +- lib/fetch/index.js | 38 ++++++++++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/lib/api/api-connect.js b/lib/api/api-connect.js index 92f317fb842..fd2b6ad97a5 100644 --- a/lib/api/api-connect.js +++ b/lib/api/api-connect.js @@ -51,7 +51,7 @@ class ConnectHandler extends AsyncResource { this.callback = null - let headers + let headers = rawHeaders // Indicates is an HTTP2Session if (headers != null) { headers = this.responseHeaders === 'raw' ? util.parseRawHeaders(rawHeaders) : util.parseHeaders(rawHeaders) diff --git a/lib/fetch/index.js b/lib/fetch/index.js index d615f07ea27..4168047e803 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -1979,19 +1979,37 @@ async function httpNetworkFetch ( let location = '' const headers = new Headers() - for (let n = 0; n < headersList.length; n += 2) { - const key = headersList[n + 0].toString('latin1') - const val = headersList[n + 1].toString('latin1') - if (key.toLowerCase() === 'content-encoding') { - // https://www.rfc-editor.org/rfc/rfc7231#section-3.1.2.1 - // "All content-coding values are case-insensitive..." - codings = val.toLowerCase().split(',').map((x) => x.trim()).reverse() - } else if (key.toLowerCase() === 'location') { - location = val + // For H2, the headers are a plain JS object + // We distinguish between them and iterate accordingly + if (Array.isArray(headers)) { + for (let n = 0; n < headersList.length; n += 2) { + const key = headersList[n + 0].toString('latin1') + const val = headersList[n + 1].toString('latin1') + if (key.toLowerCase() === 'content-encoding') { + // https://www.rfc-editor.org/rfc/rfc7231#section-3.1.2.1 + // "All content-coding values are case-insensitive..." + codings = val.toLowerCase().split(',').map((x) => x.trim()) + } else if (key.toLowerCase() === 'location') { + location = val + } + + headers.append(key, val) } + } else { + const keys = Object.keys(headersList) + for (const key of keys) { + const val = headersList[key] + if (key.toLowerCase() === 'content-encoding') { + // https://www.rfc-editor.org/rfc/rfc7231#section-3.1.2.1 + // "All content-coding values are case-insensitive..." + codings = val.toLowerCase().split(',').map((x) => x.trim()).reverse() + } else if (key.toLowerCase() === 'location') { + location = val + } - headers.append(key, val) + headers.append(key, val) + } } this.body = new Readable({ read: resume }) From 837e6292712214c13160c7bfa08f380254dfdf0f Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 16 Aug 2023 11:02:52 +0200 Subject: [PATCH 42/55] feat: enhance h2 for fetch --- lib/client.js | 1 - test/http2.js | 267 ++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 257 insertions(+), 11 deletions(-) diff --git a/lib/client.js b/lib/client.js index e14b91838e8..73932274653 100644 --- a/lib/client.js +++ b/lib/client.js @@ -27,7 +27,6 @@ const { ClientDestroyedError } = require('./core/errors') const buildConnector = require('./core/connect') -const FixedQueue = require('./node/fixed-queue') const { kUrl, kReset, diff --git a/test/http2.js b/test/http2.js index 36b83c5d780..d9d25f2dc05 100644 --- a/test/http2.js +++ b/test/http2.js @@ -9,9 +9,9 @@ const { Writable, pipeline, PassThrough, Readable } = require('node:stream') const { test, plan } = require('tap') const pem = require('https-pem') -const { Client } = require('..') +const { Client, fetch } = require('..') -plan(16) +plan(20) test('Should support H2 connection', async t => { const body = [] @@ -121,7 +121,7 @@ test('Should support H2 GOAWAY (server-side)', async t => { stream.end('hello h2!') }) - server.on('session', (session) => { + server.on('session', session => { setTimeout(() => { session.goaway(204) }, 1000) @@ -178,13 +178,16 @@ test('Should throw if bad allowH2 has been pased', async t => { }) test('Request should fail if allowH2 is false and server advertises h2 only', async t => { - const server = createSecureServer({ - ...pem, - allowHTTP1: false, - ALPNProtocols: ['http/1.1'] - }, (req, res) => { - t.fail('Should not create a valid h2 stream') - }) + const server = createSecureServer( + { + ...pem, + allowHTTP1: false, + ALPNProtocols: ['http/1.1'] + }, + (req, res) => { + t.fail('Should not create a valid h2 stream') + } + ) server.listen(0) await once(server, 'listening') @@ -847,3 +850,247 @@ test( t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) } ) + +test('[Fetch] Should handle h2 request with body (string or buffer)', async t => { + const server = createSecureServer(pem) + const expectedBody = 'hello from client!' + const expectedRequestBody = 'hello h2!' + const requestBody = [] + + server.on('stream', async (stream, headers) => { + stream.on('data', chunk => requestBody.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end(expectedRequestBody) + }) + + t.plan(2) + + server.listen() + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + method: 'POST', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' + }, + body: expectedBody + } + ) + + const responseBody = await response.text() + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + t.equal(Buffer.concat(requestBody).toString('utf-8'), expectedBody) + t.equal(responseBody, expectedRequestBody) +}) + +test('[Fetch] Should handle h2 request with body (stream)', async t => { + const server = createSecureServer(pem) + const expectedBody = readFileSync(__filename, 'utf-8') + const stream = createReadStream(__filename) + const requestChunks = [] + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'PUT') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + method: 'PUT', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' + }, + body: stream, + duplex: 'half' + } + ) + + const responseBody = await response.text() + + t.equal(response.status, 200) + t.equal(response.headers.get('content-type'), 'text/plain; charset=utf-8') + t.equal(response.headers.get('x-custom-h2'), 'foo') + t.equal(responseBody, 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) +}) + +test( + 'Should handle h2 request with body (Blob)', + { skip: !Blob }, + async t => { + const server = createSecureServer(pem) + const expectedBody = 'asd' + const requestChunks = [] + const body = new Blob(['asd'], { + type: 'text/plain' + }) + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'POST') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + body, + method: 'POST', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' + } + } + ) + + const responseBody = await response.arrayBuffer() + + t.equal(response.status, 200) + t.equal(response.headers.get('content-type'), 'text/plain; charset=utf-8') + t.equal(response.headers.get('x-custom-h2'), 'foo') + t.same(new TextDecoder().decode(responseBody).toString(), 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) + } +) + +test( + 'Should handle h2 request with body (Blob:ArrayBuffer)', + { skip: !Blob }, + async t => { + const server = createSecureServer(pem) + const expectedBody = 'hello' + const requestChunks = [] + const expectedResponseBody = { hello: 'h2' } + const buf = Buffer.from(expectedBody) + const body = new ArrayBuffer(buf.byteLength) + + buf.copy(new Uint8Array(body)) + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'PUT') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'application/json', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end(JSON.stringify(expectedResponseBody)) + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + body, + method: 'PUT', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' + } + } + ) + + const responseBody = await response.json() + + t.equal(response.status, 200) + t.equal(response.headers.get('content-type'), 'application/json') + t.equal(response.headers.get('x-custom-h2'), 'foo') + t.same(responseBody, expectedResponseBody) + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) + } +) From bf2383f2872df1fd19ffce6946f2a0ce0cfa6d33 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 16 Aug 2023 13:47:01 +0200 Subject: [PATCH 43/55] refactor: apply review suggestions Co-authored-by: Robert Nagy --- lib/client.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/client.js b/lib/client.js index 73932274653..7367cbadbfc 100644 --- a/lib/client.js +++ b/lib/client.js @@ -268,7 +268,7 @@ class Client extends DispatcherBase { // HTTP/2 this[kHTTP2Session] = null - this[kHTTP2SessionState] = { + this[kHTTP2SessionState] = !allowH2 ? null : { // streams: null, // Fixed queue of streams - For future support of `push` openStreams: 0, // Keep track of them to decide wether or not unref the session maxStreams: 100 // Max peerConcurrentStreams for a Node h2 server @@ -388,11 +388,7 @@ class Client extends DispatcherBase { util.destroy(this[kHTTP2Session], err) this[kHTTP2Session] = null - this[kHTTP2SessionState] = { - maxStreams, - openStreams: 0 - // streams: null // Fixed queue of streams - } + this[kHTTP2SessionState] = null } if (!this[kSocket]) { @@ -1198,7 +1194,7 @@ async function connect (client) { }) if (client.destroyed) { - util.destroy(socket.on('error', () => { }), new ClientDestroyedError()) + util.destroy(socket.on('error', () => {}), new ClientDestroyedError()) return } @@ -1410,7 +1406,7 @@ function _resume (client, sync) { return } - if ((socket.destroyed || socket[kWriting] || socket[kReset] || socket[kBlocking])) { + if (socket.destroyed || socket[kWriting] || socket[kReset] || socket[kBlocking]) { return } From 5908efc51544ad4ddc7b25a51d162b1a2e7a5e4b Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 16 Aug 2023 14:00:56 +0200 Subject: [PATCH 44/55] refactor: set allowh2 to false --- lib/core/connect.js | 2 +- test/http2.js | 152 ++++++++++++++++++++++++-------------------- 2 files changed, 84 insertions(+), 70 deletions(-) diff --git a/lib/core/connect.js b/lib/core/connect.js index 8673e747793..bb71085a156 100644 --- a/lib/core/connect.js +++ b/lib/core/connect.js @@ -79,7 +79,7 @@ function buildConnector ({ allowH2, maxCachedSessions, socketPath, timeout, ...o const options = { path: socketPath, ...opts } const sessionCache = new SessionCache(maxCachedSessions == null ? 100 : maxCachedSessions) timeout = timeout == null ? 10e3 : timeout - allowH2 = allowH2 != null ? allowH2 : true + allowH2 = allowH2 != null ? allowH2 : false return function connect ({ hostname, host, protocol, port, servername, localAddress, httpSocket }, callback) { let socket if (protocol === 'https:') { diff --git a/test/http2.js b/test/http2.js index d9d25f2dc05..67422c239f3 100644 --- a/test/http2.js +++ b/test/http2.js @@ -34,7 +34,8 @@ test('Should support H2 connection', async t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.plan(6) @@ -82,7 +83,8 @@ test('Should support H2 connection (headers as array)', async t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.plan(7) @@ -133,7 +135,8 @@ test('Should support H2 GOAWAY (server-side)', async t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.plan(9) @@ -242,7 +245,8 @@ test('Should handle h2 continue', async t => { connect: { rejectUnauthorized: false }, - expectContinue: true + expectContinue: true, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -291,7 +295,8 @@ test('Dispatcher#Stream', t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -339,7 +344,8 @@ test('Dispatcher#Pipeline', t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -397,7 +403,8 @@ test('Dispatcher#Connect', t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -443,7 +450,8 @@ test('Dispatcher#Upgrade', t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -471,7 +479,8 @@ test('Dispatcher#destroy', async t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.plan(4) @@ -551,7 +560,8 @@ test('Should handle h2 request with body (string or buffer) - dispatch', t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -628,7 +638,8 @@ test('Should handle h2 request with body (stream)', async t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -696,7 +707,8 @@ test('Should handle h2 request with body (iterable)', async t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -757,7 +769,8 @@ test('Should handle h2 request with body (Blob)', { skip: !Blob }, async t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -822,7 +835,8 @@ test( const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -877,7 +891,8 @@ test('[Fetch] Should handle h2 request with body (string or buffer)', async t => const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) const response = await fetch( @@ -933,7 +948,8 @@ test('[Fetch] Should handle h2 request with body (stream)', async t => { const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) @@ -963,70 +979,67 @@ test('[Fetch] Should handle h2 request with body (stream)', async t => { t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) }) -test( - 'Should handle h2 request with body (Blob)', - { skip: !Blob }, - async t => { - const server = createSecureServer(pem) - const expectedBody = 'asd' - const requestChunks = [] - const body = new Blob(['asd'], { - type: 'text/plain' - }) - - server.on('stream', async (stream, headers) => { - t.equal(headers[':method'], 'POST') - t.equal(headers[':path'], '/') - t.equal(headers[':scheme'], 'https') +test('Should handle h2 request with body (Blob)', { skip: !Blob }, async t => { + const server = createSecureServer(pem) + const expectedBody = 'asd' + const requestChunks = [] + const body = new Blob(['asd'], { + type: 'text/plain' + }) - stream.on('data', chunk => requestChunks.push(chunk)) + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'POST') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') - stream.respond({ - 'content-type': 'text/plain; charset=utf-8', - 'x-custom-h2': headers['x-my-header'], - ':status': 200 - }) + stream.on('data', chunk => requestChunks.push(chunk)) - stream.end('hello h2!') + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 }) - t.plan(8) + stream.end('hello h2!') + }) - server.listen(0) - await once(server, 'listening') + t.plan(8) - const client = new Client(`https://localhost:${server.address().port}`, { - connect: { - rejectUnauthorized: false - } - }) + server.listen(0) + await once(server, 'listening') - t.teardown(server.close.bind(server)) - t.teardown(client.close.bind(client)) + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + allowH2: true + }) - const response = await fetch( - `https://localhost:${server.address().port}/`, - // Needs to be passed to disable the reject unauthorized - { - body, - method: 'POST', - dispatcher: client, - headers: { - 'x-my-header': 'foo', - 'content-type': 'text-plain' - } + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + body, + method: 'POST', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' } - ) + } + ) - const responseBody = await response.arrayBuffer() + const responseBody = await response.arrayBuffer() - t.equal(response.status, 200) - t.equal(response.headers.get('content-type'), 'text/plain; charset=utf-8') - t.equal(response.headers.get('x-custom-h2'), 'foo') - t.same(new TextDecoder().decode(responseBody).toString(), 'hello h2!') - t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) - } -) + t.equal(response.status, 200) + t.equal(response.headers.get('content-type'), 'text/plain; charset=utf-8') + t.equal(response.headers.get('x-custom-h2'), 'foo') + t.same(new TextDecoder().decode(responseBody).toString(), 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) +}) test( 'Should handle h2 request with body (Blob:ArrayBuffer)', @@ -1065,7 +1078,8 @@ test( const client = new Client(`https://localhost:${server.address().port}`, { connect: { rejectUnauthorized: false - } + }, + allowH2: true }) t.teardown(server.close.bind(server)) From efa11b9489db468f71286e5ab6ef9b74d11cdb63 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 16 Aug 2023 14:03:15 +0200 Subject: [PATCH 45/55] fix: linting --- lib/client.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/client.js b/lib/client.js index 7367cbadbfc..d368278aee1 100644 --- a/lib/client.js +++ b/lib/client.js @@ -268,11 +268,13 @@ class Client extends DispatcherBase { // HTTP/2 this[kHTTP2Session] = null - this[kHTTP2SessionState] = !allowH2 ? null : { - // streams: null, // Fixed queue of streams - For future support of `push` - openStreams: 0, // Keep track of them to decide wether or not unref the session - maxStreams: 100 // Max peerConcurrentStreams for a Node h2 server - } + this[kHTTP2SessionState] = !allowH2 + ? null + : { + // streams: null, // Fixed queue of streams - For future support of `push` + openStreams: 0, // Keep track of them to decide wether or not unref the session + maxStreams: 100 // Max peerConcurrentStreams for a Node h2 server + } this[kHost] = `${this[kUrl].hostname}${this[kUrl].port ? `:${this[kUrl].port}` : ''}` // kQueue is built up of 3 sections separated by @@ -384,8 +386,6 @@ class Client extends DispatcherBase { } if (this[kHTTP2Session] != null) { - const { maxStreams } = this[kHTTP2SessionState] - util.destroy(this[kHTTP2Session], err) this[kHTTP2Session] = null this[kHTTP2SessionState] = null From 4d5b3ca7de945db3542bcd27ff7f636e535f3296 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 18 Aug 2023 10:44:08 +0200 Subject: [PATCH 46/55] refactor: implement kHTTPConnVersion symbol --- lib/client.js | 9 ++++++--- lib/core/symbols.js | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/client.js b/lib/client.js index d368278aee1..83818fabb3c 100644 --- a/lib/client.js +++ b/lib/client.js @@ -70,6 +70,7 @@ const { kInterceptors, kLocalAddress, kMaxResponseSize, + kHTTPConnVersion, // HTTP2 kHost, kHTTP2Session, @@ -265,6 +266,7 @@ class Client extends DispatcherBase { this[kMaxRequests] = maxRequestsPerClient this[kClosedResolve] = null this[kMaxResponseSize] = maxResponseSize > -1 ? maxResponseSize : -1 + this[kHTTPConnVersion] = 'h1' // HTTP/2 this[kHTTP2Session] = null @@ -1100,9 +1102,9 @@ function onSocketEnd () { } function onSocketClose () { - const { [kClient]: client, [kHTTP2Session]: session, [kParser]: parser } = this + const { [kClient]: client, [kParser]: parser } = this - if (session == null && parser) { + if (client[kHTTPConnVersion] === 'h1' && parser) { if (!this[kError] && parser.statusCode && !parser.shouldKeepAlive) { // We treat all incoming data so far as a valid response. parser.onMessageComplete() @@ -1209,6 +1211,7 @@ async function connect (client) { peerMaxConcurrentStreams: client[kHTTP2SessionState].maxStreams }) + client[kHTTPConnVersion] = 'h2' session[kClient] = client session[kSocket] = socket session.on('error', onHttp2SessionError) @@ -1462,7 +1465,7 @@ function _resume (client, sync) { } function write (client, request) { - if (client[kHTTP2Session]) { + if (client[kHTTPConnVersion] === 'h2') { writeH2(client, client[kHTTP2Session], request) return } diff --git a/lib/core/symbols.js b/lib/core/symbols.js index fc258d8efbb..12e24d328e7 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -55,5 +55,6 @@ module.exports = { kHTTP2Session: Symbol('http2Session'), kHTTP2SessionState: Symbol('http2Session state'), kHTTP2BuildRequest: Symbol('http2 build request'), - kHTTP2CopyHeaders: Symbol('http2 copy headers') + kHTTP2CopyHeaders: Symbol('http2 copy headers'), + kHTTPConnVersion: Symbol('http connection version') } From e1b794c36df059128d00749153a5bfc41e756855 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 18 Aug 2023 10:54:33 +0200 Subject: [PATCH 47/55] test: adjust testing --- test/http2-alpn.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/http2-alpn.js b/test/http2-alpn.js index 712a493cb3c..04b8cb6abd8 100644 --- a/test/http2-alpn.js +++ b/test/http2-alpn.js @@ -57,7 +57,8 @@ test('Should upgrade to HTTP/2 when HTTPS/1 is available for GET', async (t) => connect: { ca, servername: 'agent1' - } + }, + allowH2: true }) // close the client on teardown @@ -203,7 +204,8 @@ test('Should upgrade to HTTP/2 when HTTPS/1 is available for POST', async (t) => connect: { ca, servername: 'agent1' - } + }, + allowH2: true }) // close the client on teardown From 4bbd684a6c5f7802372b19eae1c3461f928c7b57 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 18 Aug 2023 13:31:10 +0200 Subject: [PATCH 48/55] feat: buil factory --- lib/client.js | 11 ++++++----- lib/core/request.js | 8 +++++++- lib/core/symbols.js | 1 + 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/client.js b/lib/client.js index 83818fabb3c..83e08a2ce30 100644 --- a/lib/client.js +++ b/lib/client.js @@ -76,7 +76,8 @@ const { kHTTP2Session, kHTTP2SessionState, kHTTP2BuildRequest, - kHTTP2CopyHeaders + kHTTP2CopyHeaders, + kHTTP1BuildRequest } = require('./core/symbols') const { constants: { @@ -337,8 +338,8 @@ class Client extends DispatcherBase { const origin = opts.origin || this[kUrl].origin let request - if (this[kHTTP2Session]) request = Request[kHTTP2BuildRequest](origin, opts, handler) - else request = new Request(origin, opts, handler) + if (this[kHTTPConnVersion] === 'h2') request = Request[kHTTP2BuildRequest](origin, opts, handler) + else request = Request[kHTTP1BuildRequest](origin, opts, handler) this[kQueue].push(request) if (this[kResuming]) { @@ -1234,10 +1235,10 @@ async function connect (client) { socket[kReset] = false socket[kBlocking] = false socket[kParser] = new Parser(client, socket, llhttpInstance) - socket[kCounter] = 0 - socket[kMaxRequests] = client[kMaxRequests] } + socket[kCounter] = 0 + socket[kMaxRequests] = client[kMaxRequests] socket[kClient] = client socket[kError] = null diff --git a/lib/core/request.js b/lib/core/request.js index 2cc02a07d0d..60e170bb527 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -5,7 +5,7 @@ const { NotSupportedError } = require('./errors') const assert = require('assert') -const { kHTTP2BuildRequest, kHTTP2CopyHeaders } = require('./symbols') +const { kHTTP2BuildRequest, kHTTP2CopyHeaders, kHTTP1BuildRequest } = require('./symbols') const util = require('./util') // tokenRegExp and headerCharRegex have been lifted from @@ -284,6 +284,12 @@ class Request { return this } + static [kHTTP1BuildRequest] (origin, opts, handler) { + // TODO: Migrate header parsing here, to make Requests + // HTTP agnostic + return new Request(origin, opts, handler) + } + static [kHTTP2BuildRequest] (origin, opts, handler) { const headers = opts.headers opts = { ...opts, headers: null } diff --git a/lib/core/symbols.js b/lib/core/symbols.js index 12e24d328e7..c2492f4355f 100644 --- a/lib/core/symbols.js +++ b/lib/core/symbols.js @@ -55,6 +55,7 @@ module.exports = { kHTTP2Session: Symbol('http2Session'), kHTTP2SessionState: Symbol('http2Session state'), kHTTP2BuildRequest: Symbol('http2 build request'), + kHTTP1BuildRequest: Symbol('http1 build request'), kHTTP2CopyHeaders: Symbol('http2 copy headers'), kHTTPConnVersion: Symbol('http connection version') } From d27be8aa2e130a83739bc050e53c4b6a5436dcec Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Sun, 20 Aug 2023 13:31:02 +0200 Subject: [PATCH 49/55] fix: rebase --- lib/fetch/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fetch/index.js b/lib/fetch/index.js index 4168047e803..516f7e5c56b 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -1982,7 +1982,7 @@ async function httpNetworkFetch ( // For H2, the headers are a plain JS object // We distinguish between them and iterate accordingly - if (Array.isArray(headers)) { + if (Array.isArray(headersList)) { for (let n = 0; n < headersList.length; n += 2) { const key = headersList[n + 0].toString('latin1') const val = headersList[n + 1].toString('latin1') From 37c73bad58bdc7b46679c2ef97c8db823f79ff93 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Sun, 20 Aug 2023 14:06:55 +0200 Subject: [PATCH 50/55] feat: enhance TS types for maxConcurrent streams --- docs/api/Client.md | 2 ++ docs/api/Dispatcher.md | 1 + lib/client.js | 33 ++++++++++++++------------------- lib/core/request.js | 2 ++ test/http2.js | 26 +++++++++++++++++++++++++- types/client.d.ts | 10 +++++++++- types/dispatcher.d.ts | 2 ++ 7 files changed, 55 insertions(+), 21 deletions(-) diff --git a/docs/api/Client.md b/docs/api/Client.md index fc7c5d26e8f..f19c4081dd0 100644 --- a/docs/api/Client.md +++ b/docs/api/Client.md @@ -30,6 +30,8 @@ Returns: `Client` * **interceptors** `{ Client: DispatchInterceptor[] }` - Default: `[RedirectInterceptor]` - A list of interceptors that are applied to the dispatch method. Additional logic can be applied (such as, but not limited to: 302 status code handling, authentication, cookies, compression and caching). Note that the behavior of interceptors is Experimental and might change at any given time. * **autoSelectFamily**: `boolean` (optional) - Default: depends on local Node version, on Node 18.13.0 and above is `false`. Enables a family autodetection algorithm that loosely implements section 5 of [RFC 8305](https://tools.ietf.org/html/rfc8305#section-5). See [here](https://nodejs.org/api/net.html#socketconnectoptions-connectlistener) for more details. This option is ignored if not supported by the current Node version. * **autoSelectFamilyAttemptTimeout**: `number` - Default: depends on local Node version, on Node 18.13.0 and above is `250`. The amount of time in milliseconds to wait for a connection attempt to finish before trying the next address when using the `autoSelectFamily` option. See [here](https://nodejs.org/api/net.html#socketconnectoptions-connectlistener) for more details. +* **allowH2**: `boolean` - Default: `false`. Enables support for H2 if the server has assigned bigger priority to it through ALPN negotiation. +* **maxConcurrentStreams**: `number` - Default: `100`. Dictates the maximum number of concurrent streams for a single H2 session. It can be overriden by a SETTINGS remote frame. #### Parameter: `ConnectOptions` diff --git a/docs/api/Dispatcher.md b/docs/api/Dispatcher.md index a50642948aa..a04be8cff1e 100644 --- a/docs/api/Dispatcher.md +++ b/docs/api/Dispatcher.md @@ -202,6 +202,7 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo * **bodyTimeout** `number | null` (optional) - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 300 seconds. * **headersTimeout** `number | null` (optional) - The amount of time the parser will wait to receive the complete HTTP headers while not sending the request. Defaults to 300 seconds. * **throwOnError** `boolean` (optional) - Default: `false` - Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server. +* **expectContinue** `boolean` (optional) - Default: `false` - For H2, it appends the expect: 100-continue header, and halts the request body until a 100-continue is received from the remote server #### Parameter: `DispatchHandler` diff --git a/lib/client.js b/lib/client.js index 83e08a2ce30..bc717c36013 100644 --- a/lib/client.js +++ b/lib/client.js @@ -144,7 +144,9 @@ class Client extends DispatcherBase { maxResponseSize, autoSelectFamily, autoSelectFamilyAttemptTimeout, - allowH2 + // h2 + allowH2, + maxConcurrentStreams } = {}) { super() @@ -216,10 +218,6 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('localAddress must be valid string IP address') } - if (allowH2 != null && typeof allowH2 !== 'boolean') { - throw new InvalidArgumentError('allowH2 must be a valid boolean value') - } - if (maxResponseSize != null && (!Number.isInteger(maxResponseSize) || maxResponseSize < -1)) { throw new InvalidArgumentError('maxResponseSize must be a positive number') } @@ -231,6 +229,15 @@ class Client extends DispatcherBase { throw new InvalidArgumentError('autoSelectFamilyAttemptTimeout must be a positive number') } + // h2 + if (allowH2 != null && typeof allowH2 !== 'boolean') { + throw new InvalidArgumentError('allowH2 must be a valid boolean value') + } + + if (maxConcurrentStreams != null && (typeof maxConcurrentStreams !== 'number' || maxConcurrentStreams < 1)) { + throw new InvalidArgumentError('maxConcurrentStreams must be a possitive integer, greater than 0') + } + if (typeof connect !== 'function') { connect = buildConnector({ ...tls, @@ -276,7 +283,7 @@ class Client extends DispatcherBase { : { // streams: null, // Fixed queue of streams - For future support of `push` openStreams: 0, // Keep track of them to decide wether or not unref the session - maxStreams: 100 // Max peerConcurrentStreams for a Node h2 server + maxConcurrentStreams: maxConcurrentStreams != null ? maxConcurrentStreams : 100 // Max peerConcurrentStreams for a Node h2 server } this[kHost] = `${this[kUrl].hostname}${this[kUrl].port ? `:${this[kUrl].port}` : ''}` @@ -1209,7 +1216,7 @@ async function connect (client) { if (isH2) { const session = http2.connect(client[kUrl], { createConnection: () => socket, - peerMaxConcurrentStreams: client[kHTTP2SessionState].maxStreams + peerMaxConcurrentStreams: client[kHTTP2SessionState].maxConcurrentStreams }) client[kHTTPConnVersion] = 'h2' @@ -1657,17 +1664,11 @@ function writeH2 (client, session, request) { let stream const h2State = client[kHTTP2SessionState] - const shouldCreateNewStream = h2State.openStreams < h2State.maxStreams headers[HTTP2_HEADER_AUTHORITY] = host || client[kHost] headers[HTTP2_HEADER_PATH] = path if (method === 'CONNECT') { - if (!shouldCreateNewStream) { - errorRequest(client, request, new Error('H2: Max streams reached')) - return false - } - session.ref() // we are already connected, streams are pending, first request // will create a new stream. We trigger a request to create the stream and wait until @@ -1748,12 +1749,6 @@ function writeH2 (client, session, request) { if (expectContinue) { headers[HTTP2_HEADER_EXPECT] = '100-continue' - - if (!shouldCreateNewStream) { - errorRequest(client, request, new Error('H2: Max streams reached')) - return false - } - /** * @type {import('node:http2').ClientHttp2Stream} */ diff --git a/lib/core/request.js b/lib/core/request.js index 60e170bb527..3ddc7fdbe32 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -296,6 +296,8 @@ class Request { const request = new Request(origin, opts, handler) + request.headers = {} + if (Array.isArray(headers)) { if (headers.length % 2 !== 0) { throw new InvalidArgumentError('headers array must be even') diff --git a/test/http2.js b/test/http2.js index 67422c239f3..4f7ac90092b 100644 --- a/test/http2.js +++ b/test/http2.js @@ -11,7 +11,7 @@ const pem = require('https-pem') const { Client, fetch } = require('..') -plan(20) +plan(21) test('Should support H2 connection', async t => { const body = [] @@ -180,6 +180,30 @@ test('Should throw if bad allowH2 has been pased', async t => { } }) +test('Should throw if bad maxConcurrentStreams has been pased', async t => { + try { + // eslint-disable-next-line + new Client('https://localhost:1000', { + allowH2: true, + maxConcurrentStreams: {} + }) + t.fail() + } catch (error) { + t.equal(error.message, 'maxConcurrentStreams must be a possitive integer, greater than 0') + } + + try { + // eslint-disable-next-line + new Client('https://localhost:1000', { + allowH2: true, + maxConcurrentStreams: -1 + }) + t.fail() + } catch (error) { + t.equal(error.message, 'maxConcurrentStreams must be a possitive integer, greater than 0') + } +}) + test('Request should fail if allowH2 is false and server advertises h2 only', async t => { const server = createSecureServer( { diff --git a/types/client.d.ts b/types/client.d.ts index cfdbf977e55..3d348f9427b 100644 --- a/types/client.d.ts +++ b/types/client.d.ts @@ -72,8 +72,16 @@ export declare namespace Client { autoSelectFamily?: boolean; /** The amount of time in milliseconds to wait for a connection attempt to finish before trying the next address when using the `autoSelectFamily` option. */ autoSelectFamilyAttemptTimeout?: number; - // TODO + /** + * @description Enables support for H2 if the server has assigned bigger priority to it through ALPN negotiation. + * @default false + */ allowH2?: boolean; + /** + * @description Dictates the maximum number of concurrent streams for a single H2 session. It can be overriden by a SETTINGS remote frame. + * @default 100 + */ + maxConcurrentStreams?: number } export interface SocketInfo { localAddress?: string diff --git a/types/dispatcher.d.ts b/types/dispatcher.d.ts index 7f621371f86..42a78ba0834 100644 --- a/types/dispatcher.d.ts +++ b/types/dispatcher.d.ts @@ -117,6 +117,8 @@ declare namespace Dispatcher { reset?: boolean; /** Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server. Defaults to false */ throwOnError?: boolean; + /** For H2, it appends the expect: 100-continue header, and halts the request body until a 100-continue is received from the remote server*/ + expectContinue?: boolean; } export interface ConnectOptions { path: string; From 224c26e84ad28021d7d348ecea31a086ad878276 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 23 Aug 2023 10:17:10 +0200 Subject: [PATCH 51/55] test: move fetch tests to fetch folder --- test/fetch/http2.js | 256 ++++++++++++++++++++++++++++++++++++++++++++ test/http2.js | 248 +----------------------------------------- 2 files changed, 258 insertions(+), 246 deletions(-) create mode 100644 test/fetch/http2.js diff --git a/test/fetch/http2.js b/test/fetch/http2.js new file mode 100644 index 00000000000..246c14e1eef --- /dev/null +++ b/test/fetch/http2.js @@ -0,0 +1,256 @@ +'use strict' + +const { createSecureServer } = require('node:http2') +const { createReadStream, readFileSync } = require('node:fs') +const { once } = require('node:events') +const { Blob } = require('node:buffer') + +const { test, plan } = require('tap') +const pem = require('https-pem') + +const { Client, fetch } = require('../..') + +plan(4) + +test('[Fetch] Should handle h2 request with body (string or buffer)', async t => { + const server = createSecureServer(pem) + const expectedBody = 'hello from client!' + const expectedRequestBody = 'hello h2!' + const requestBody = [] + + server.on('stream', async (stream, headers) => { + stream.on('data', chunk => requestBody.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end(expectedRequestBody) + }) + + t.plan(2) + + server.listen() + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + allowH2: true + }) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + method: 'POST', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' + }, + body: expectedBody + } + ) + + const responseBody = await response.text() + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + t.equal(Buffer.concat(requestBody).toString('utf-8'), expectedBody) + t.equal(responseBody, expectedRequestBody) +}) + +test('[Fetch] Should handle h2 request with body (stream)', async t => { + const server = createSecureServer(pem) + const expectedBody = readFileSync(__filename, 'utf-8') + const stream = createReadStream(__filename) + const requestChunks = [] + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'PUT') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + allowH2: true + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + method: 'PUT', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' + }, + body: stream, + duplex: 'half' + } + ) + + const responseBody = await response.text() + + t.equal(response.status, 200) + t.equal(response.headers.get('content-type'), 'text/plain; charset=utf-8') + t.equal(response.headers.get('x-custom-h2'), 'foo') + t.equal(responseBody, 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) +}) +test('Should handle h2 request with body (Blob)', { skip: !Blob }, async t => { + const server = createSecureServer(pem) + const expectedBody = 'asd' + const requestChunks = [] + const body = new Blob(['asd'], { + type: 'text/plain' + }) + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'POST') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'text/plain; charset=utf-8', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end('hello h2!') + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + allowH2: true + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + body, + method: 'POST', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' + } + } + ) + + const responseBody = await response.arrayBuffer() + + t.equal(response.status, 200) + t.equal(response.headers.get('content-type'), 'text/plain; charset=utf-8') + t.equal(response.headers.get('x-custom-h2'), 'foo') + t.same(new TextDecoder().decode(responseBody).toString(), 'hello h2!') + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) +}) + +test( + 'Should handle h2 request with body (Blob:ArrayBuffer)', + { skip: !Blob }, + async t => { + const server = createSecureServer(pem) + const expectedBody = 'hello' + const requestChunks = [] + const expectedResponseBody = { hello: 'h2' } + const buf = Buffer.from(expectedBody) + const body = new ArrayBuffer(buf.byteLength) + + buf.copy(new Uint8Array(body)) + + server.on('stream', async (stream, headers) => { + t.equal(headers[':method'], 'PUT') + t.equal(headers[':path'], '/') + t.equal(headers[':scheme'], 'https') + + stream.on('data', chunk => requestChunks.push(chunk)) + + stream.respond({ + 'content-type': 'application/json', + 'x-custom-h2': headers['x-my-header'], + ':status': 200 + }) + + stream.end(JSON.stringify(expectedResponseBody)) + }) + + t.plan(8) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + connect: { + rejectUnauthorized: false + }, + allowH2: true + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + + const response = await fetch( + `https://localhost:${server.address().port}/`, + // Needs to be passed to disable the reject unauthorized + { + body, + method: 'PUT', + dispatcher: client, + headers: { + 'x-my-header': 'foo', + 'content-type': 'text-plain' + } + } + ) + + const responseBody = await response.json() + + t.equal(response.status, 200) + t.equal(response.headers.get('content-type'), 'application/json') + t.equal(response.headers.get('x-custom-h2'), 'foo') + t.same(responseBody, expectedResponseBody) + t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) + } +) diff --git a/test/http2.js b/test/http2.js index 4f7ac90092b..d6e980ef886 100644 --- a/test/http2.js +++ b/test/http2.js @@ -9,9 +9,9 @@ const { Writable, pipeline, PassThrough, Readable } = require('node:stream') const { test, plan } = require('tap') const pem = require('https-pem') -const { Client, fetch } = require('..') +const { Client } = require('..') -plan(21) +plan(17) test('Should support H2 connection', async t => { const body = [] @@ -888,247 +888,3 @@ test( t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) } ) - -test('[Fetch] Should handle h2 request with body (string or buffer)', async t => { - const server = createSecureServer(pem) - const expectedBody = 'hello from client!' - const expectedRequestBody = 'hello h2!' - const requestBody = [] - - server.on('stream', async (stream, headers) => { - stream.on('data', chunk => requestBody.push(chunk)) - - stream.respond({ - 'content-type': 'text/plain; charset=utf-8', - 'x-custom-h2': headers['x-my-header'], - ':status': 200 - }) - - stream.end(expectedRequestBody) - }) - - t.plan(2) - - server.listen() - await once(server, 'listening') - - const client = new Client(`https://localhost:${server.address().port}`, { - connect: { - rejectUnauthorized: false - }, - allowH2: true - }) - - const response = await fetch( - `https://localhost:${server.address().port}/`, - // Needs to be passed to disable the reject unauthorized - { - method: 'POST', - dispatcher: client, - headers: { - 'x-my-header': 'foo', - 'content-type': 'text-plain' - }, - body: expectedBody - } - ) - - const responseBody = await response.text() - - t.teardown(server.close.bind(server)) - t.teardown(client.close.bind(client)) - - t.equal(Buffer.concat(requestBody).toString('utf-8'), expectedBody) - t.equal(responseBody, expectedRequestBody) -}) - -test('[Fetch] Should handle h2 request with body (stream)', async t => { - const server = createSecureServer(pem) - const expectedBody = readFileSync(__filename, 'utf-8') - const stream = createReadStream(__filename) - const requestChunks = [] - - server.on('stream', async (stream, headers) => { - t.equal(headers[':method'], 'PUT') - t.equal(headers[':path'], '/') - t.equal(headers[':scheme'], 'https') - - stream.on('data', chunk => requestChunks.push(chunk)) - - stream.respond({ - 'content-type': 'text/plain; charset=utf-8', - 'x-custom-h2': headers['x-my-header'], - ':status': 200 - }) - - stream.end('hello h2!') - }) - - t.plan(8) - - server.listen(0) - await once(server, 'listening') - - const client = new Client(`https://localhost:${server.address().port}`, { - connect: { - rejectUnauthorized: false - }, - allowH2: true - }) - - t.teardown(server.close.bind(server)) - t.teardown(client.close.bind(client)) - - const response = await fetch( - `https://localhost:${server.address().port}/`, - // Needs to be passed to disable the reject unauthorized - { - method: 'PUT', - dispatcher: client, - headers: { - 'x-my-header': 'foo', - 'content-type': 'text-plain' - }, - body: stream, - duplex: 'half' - } - ) - - const responseBody = await response.text() - - t.equal(response.status, 200) - t.equal(response.headers.get('content-type'), 'text/plain; charset=utf-8') - t.equal(response.headers.get('x-custom-h2'), 'foo') - t.equal(responseBody, 'hello h2!') - t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) -}) - -test('Should handle h2 request with body (Blob)', { skip: !Blob }, async t => { - const server = createSecureServer(pem) - const expectedBody = 'asd' - const requestChunks = [] - const body = new Blob(['asd'], { - type: 'text/plain' - }) - - server.on('stream', async (stream, headers) => { - t.equal(headers[':method'], 'POST') - t.equal(headers[':path'], '/') - t.equal(headers[':scheme'], 'https') - - stream.on('data', chunk => requestChunks.push(chunk)) - - stream.respond({ - 'content-type': 'text/plain; charset=utf-8', - 'x-custom-h2': headers['x-my-header'], - ':status': 200 - }) - - stream.end('hello h2!') - }) - - t.plan(8) - - server.listen(0) - await once(server, 'listening') - - const client = new Client(`https://localhost:${server.address().port}`, { - connect: { - rejectUnauthorized: false - }, - allowH2: true - }) - - t.teardown(server.close.bind(server)) - t.teardown(client.close.bind(client)) - - const response = await fetch( - `https://localhost:${server.address().port}/`, - // Needs to be passed to disable the reject unauthorized - { - body, - method: 'POST', - dispatcher: client, - headers: { - 'x-my-header': 'foo', - 'content-type': 'text-plain' - } - } - ) - - const responseBody = await response.arrayBuffer() - - t.equal(response.status, 200) - t.equal(response.headers.get('content-type'), 'text/plain; charset=utf-8') - t.equal(response.headers.get('x-custom-h2'), 'foo') - t.same(new TextDecoder().decode(responseBody).toString(), 'hello h2!') - t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) -}) - -test( - 'Should handle h2 request with body (Blob:ArrayBuffer)', - { skip: !Blob }, - async t => { - const server = createSecureServer(pem) - const expectedBody = 'hello' - const requestChunks = [] - const expectedResponseBody = { hello: 'h2' } - const buf = Buffer.from(expectedBody) - const body = new ArrayBuffer(buf.byteLength) - - buf.copy(new Uint8Array(body)) - - server.on('stream', async (stream, headers) => { - t.equal(headers[':method'], 'PUT') - t.equal(headers[':path'], '/') - t.equal(headers[':scheme'], 'https') - - stream.on('data', chunk => requestChunks.push(chunk)) - - stream.respond({ - 'content-type': 'application/json', - 'x-custom-h2': headers['x-my-header'], - ':status': 200 - }) - - stream.end(JSON.stringify(expectedResponseBody)) - }) - - t.plan(8) - - server.listen(0) - await once(server, 'listening') - - const client = new Client(`https://localhost:${server.address().port}`, { - connect: { - rejectUnauthorized: false - }, - allowH2: true - }) - - t.teardown(server.close.bind(server)) - t.teardown(client.close.bind(client)) - - const response = await fetch( - `https://localhost:${server.address().port}/`, - // Needs to be passed to disable the reject unauthorized - { - body, - method: 'PUT', - dispatcher: client, - headers: { - 'x-my-header': 'foo', - 'content-type': 'text-plain' - } - } - ) - - const responseBody = await response.json() - - t.equal(response.status, 200) - t.equal(response.headers.get('content-type'), 'application/json') - t.equal(response.headers.get('x-custom-h2'), 'foo') - t.same(responseBody, expectedResponseBody) - t.equal(Buffer.concat(requestChunks).toString('utf-8'), expectedBody) - } -) From 904f334feac74df55efb690d6d38fa868d046ec1 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 23 Aug 2023 10:25:21 +0200 Subject: [PATCH 52/55] feat: add experimental warning --- docs/api/Client.md | 2 ++ lib/client.js | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/docs/api/Client.md b/docs/api/Client.md index f19c4081dd0..591fb97c44b 100644 --- a/docs/api/Client.md +++ b/docs/api/Client.md @@ -17,6 +17,8 @@ Returns: `Client` ### Parameter: `ClientOptions` +> ⚠️ Warning: The `H2` support is experimental. + * **bodyTimeout** `number | null` (optional) - Default: `300e3` - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 300 seconds. * **headersTimeout** `number | null` (optional) - Default: `300e3` - The amount of time the parser will wait to receive the complete HTTP headers while not sending the request. Defaults to 300 seconds. * **keepAliveMaxTimeout** `number | null` (optional) - Default: `600e3` - The maximum allowed `keepAliveTimeout` when overridden by *keep-alive* hints from the server. Defaults to 10 minutes. diff --git a/lib/client.js b/lib/client.js index bc717c36013..1c0f2d71494 100644 --- a/lib/client.js +++ b/lib/client.js @@ -90,6 +90,9 @@ const { } } = http2 +// Experimental +let h2ExperimentalWarned = false + const FastBuffer = Buffer[Symbol.species] const kClosedResolve = Symbol('kClosedResolve') @@ -344,9 +347,9 @@ class Client extends DispatcherBase { [kDispatch] (opts, handler) { const origin = opts.origin || this[kUrl].origin - let request - if (this[kHTTPConnVersion] === 'h2') request = Request[kHTTP2BuildRequest](origin, opts, handler) - else request = Request[kHTTP1BuildRequest](origin, opts, handler) + const request = this[kHTTPConnVersion] === 'h2' + ? Request[kHTTP2BuildRequest](origin, opts, handler) + : Request[kHTTP1BuildRequest](origin, opts, handler) this[kQueue].push(request) if (this[kResuming]) { @@ -1214,6 +1217,13 @@ async function connect (client) { const isH2 = socket.alpnProtocol === 'h2' if (isH2) { + if (!h2ExperimentalWarned) { + h2ExperimentalWarned = true + process.emitWarning('H2 support is experimental, expect them to change at any time.', { + code: 'UNDICI-H2' + }) + } + const session = http2.connect(client[kUrl], { createConnection: () => socket, peerMaxConcurrentStreams: client[kHTTP2SessionState].maxConcurrentStreams From bbd1ffe237eb3788004b845a29d6beb0446fa711 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 23 Aug 2023 10:39:52 +0200 Subject: [PATCH 53/55] test: refactor suite --- test/http2.js | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/test/http2.js b/test/http2.js index d6e980ef886..47219efe575 100644 --- a/test/http2.js +++ b/test/http2.js @@ -189,7 +189,10 @@ test('Should throw if bad maxConcurrentStreams has been pased', async t => { }) t.fail() } catch (error) { - t.equal(error.message, 'maxConcurrentStreams must be a possitive integer, greater than 0') + t.equal( + error.message, + 'maxConcurrentStreams must be a possitive integer, greater than 0' + ) } try { @@ -200,7 +203,10 @@ test('Should throw if bad maxConcurrentStreams has been pased', async t => { }) t.fail() } catch (error) { - t.equal(error.message, 'maxConcurrentStreams must be a possitive integer, greater than 0') + t.equal( + error.message, + 'maxConcurrentStreams must be a possitive integer, greater than 0' + ) } }) @@ -643,8 +649,6 @@ test('Should handle h2 request with body (stream)', async t => { t.equal(headers[':path'], '/') t.equal(headers[':scheme'], 'https') - stream.on('data', chunk => requestChunks.push(chunk)) - stream.respond({ 'content-type': 'text/plain; charset=utf-8', 'x-custom-h2': headers['x-my-header'], @@ -652,6 +656,10 @@ test('Should handle h2 request with body (stream)', async t => { }) stream.end('hello h2!') + + for await (const chunk of stream) { + requestChunks.push(chunk) + } }) t.plan(8) @@ -678,11 +686,9 @@ test('Should handle h2 request with body (stream)', async t => { body: stream }) - response.body.on('data', chunk => { + for await (const chunk of response.body) { responseBody.push(chunk) - }) - - await once(response.body, 'end') + } t.equal(response.statusCode, 200) t.equal(response.headers['content-type'], 'text/plain; charset=utf-8') From e44b0784b544676eb1b0c18fa4a00a6a4d7fe3e7 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 25 Aug 2023 11:07:59 +0200 Subject: [PATCH 54/55] refactor: apply several changes --- lib/client.js | 34 +++++++++++++++++++--------------- test/fetch/encoding.js | 12 ++++++------ test/http2.js | 4 ++-- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/lib/client.js b/lib/client.js index 1c0f2d71494..ebc9a6afc11 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1063,16 +1063,18 @@ function onSocketReadable () { } function onSocketError (err) { - const { [kParser]: parser } = this + const { [kClient]: client, [kParser]: parser } = this assert(err.code !== 'ERR_TLS_CERT_ALTNAME_INVALID') - // On Mac OS, we get an ECONNRESET even if there is a full body to be forwarded - // to the user. - if (err.code === 'ECONNRESET' && parser && parser.statusCode && !parser.shouldKeepAlive) { - // We treat all incoming data so for as a valid response. - parser.onMessageComplete() - return + if (client[kHTTPConnVersion] !== 'h2') { + // On Mac OS, we get an ECONNRESET even if there is a full body to be forwarded + // to the user. + if (err.code === 'ECONNRESET' && parser.statusCode && !parser.shouldKeepAlive) { + // We treat all incoming data so for as a valid response. + parser.onMessageComplete() + return + } } this[kError] = err @@ -1101,12 +1103,14 @@ function onError (client, err) { } function onSocketEnd () { - const { [kParser]: parser } = this + const { [kParser]: parser, [kClient]: client } = this - if (parser && parser.statusCode && !parser.shouldKeepAlive) { - // We treat all incoming data so far as a valid response. - parser.onMessageComplete() - return + if (client[kHTTPConnVersion] !== 'h2') { + if (parser.statusCode && !parser.shouldKeepAlive) { + // We treat all incoming data so far as a valid response. + parser.onMessageComplete() + return + } } util.destroy(this, new SocketError('other side closed', util.getSocketInfo(this))) @@ -1895,7 +1899,7 @@ function writeH2 (client, session, request) { function writeStream ({ h2stream, body, client, request, socket, contentLength, header, expectsPayload }) { assert(contentLength !== 0 || client[kRunning] === 0, 'stream body cannot be pipelined') - if (socket.alpnProtocol === 'h2') { + if (client[kHTTPConnVersion] === 'h2') { // For HTTP/2, is enough to pipe the stream const pipe = pipeline( body, @@ -2006,7 +2010,7 @@ function writeStream ({ h2stream, body, client, request, socket, contentLength, async function writeBlob ({ h2stream, body, client, request, socket, contentLength, header, expectsPayload }) { assert(contentLength === body.size, 'blob body must have content length') - const isH2 = socket.alpnProtocol === 'h2' + const isH2 = client[kHTTPConnVersion] === 'h2' try { if (contentLength != null && contentLength !== body.size) { throw new RequestContentLengthMismatchError() @@ -2060,7 +2064,7 @@ async function writeIterable ({ h2stream, body, client, request, socket, content } }) - if (socket.alpnProtocol === 'h2') { + if (client[kHTTPConnVersion] === 'h2') { h2stream .on('close', onDrain) .on('drain', onDrain) diff --git a/test/fetch/encoding.js b/test/fetch/encoding.js index 5817924521a..75d8fc37d5b 100644 --- a/test/fetch/encoding.js +++ b/test/fetch/encoding.js @@ -17,10 +17,10 @@ test('content-encoding header is case-iNsENsITIve', async (t) => { res.setHeader('Content-Encoding', contentCodings) res.setHeader('Content-Type', 'text/plain') - gzip.pipe(brotli).pipe(res) + brotli.pipe(gzip).pipe(res) - gzip.write(text) - gzip.end() + brotli.write(text) + brotli.end() }).listen(0) t.teardown(server.close.bind(server)) @@ -43,10 +43,10 @@ test('response decompression according to content-encoding should be handled in res.setHeader('Content-Encoding', contentCodings) res.setHeader('Content-Type', 'text/plain') - deflate.pipe(gzip).pipe(res) + gzip.pipe(deflate).pipe(res) - deflate.write(text) - deflate.end() + gzip.write(text) + gzip.end() }).listen(0) t.teardown(server.close.bind(server)) diff --git a/test/http2.js b/test/http2.js index 47219efe575..f05f1d867e2 100644 --- a/test/http2.js +++ b/test/http2.js @@ -655,11 +655,11 @@ test('Should handle h2 request with body (stream)', async t => { ':status': 200 }) - stream.end('hello h2!') - for await (const chunk of stream) { requestChunks.push(chunk) } + + stream.end('hello h2!') }) t.plan(8) From 75f6a2c108abbb017f11aea4b0bc5844e5340a9f Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 30 Aug 2023 10:41:56 +0200 Subject: [PATCH 55/55] test: split tests between v20 and lower --- test/http2.js | 115 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 84 insertions(+), 31 deletions(-) diff --git a/test/http2.js b/test/http2.js index f05f1d867e2..8fd9c616bc3 100644 --- a/test/http2.js +++ b/test/http2.js @@ -11,7 +11,9 @@ const pem = require('https-pem') const { Client } = require('..') -plan(17) +const isGreaterThanv20 = Number(process.version.slice(1).split('.')[0]) >= 20 + +plan(18) test('Should support H2 connection', async t => { const body = [] @@ -210,41 +212,92 @@ test('Should throw if bad maxConcurrentStreams has been pased', async t => { } }) -test('Request should fail if allowH2 is false and server advertises h2 only', async t => { - const server = createSecureServer( - { - ...pem, - allowHTTP1: false, - ALPNProtocols: ['http/1.1'] - }, - (req, res) => { - t.fail('Should not create a valid h2 stream') - } - ) +test( + 'Request should fail if allowH2 is false and server advertises h1 only', + { skip: isGreaterThanv20 }, + async t => { + const server = createSecureServer( + { + ...pem, + allowHTTP1: false, + ALPNProtocols: ['http/1.1'] + }, + (req, res) => { + t.fail('Should not create a valid h2 stream') + } + ) - server.listen(0) - await once(server, 'listening') + server.listen(0) + await once(server, 'listening') - const client = new Client(`https://localhost:${server.address().port}`, { - allowH2: false, - connect: { - rejectUnauthorized: false - } - }) + const client = new Client(`https://localhost:${server.address().port}`, { + allowH2: false, + connect: { + rejectUnauthorized: false + } + }) - t.teardown(server.close.bind(server)) - t.teardown(client.close.bind(client)) + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) - const response = await client.request({ - path: '/', - method: 'GET', - headers: { - 'x-my-header': 'foo' - } - }) + const response = await client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) - t.equal(response.statusCode, 403) -}) + t.equal(response.statusCode, 403) + } +) + +test( + '[v20] Request should fail if allowH2 is false and server advertises h1 only', + { skip: !isGreaterThanv20 }, + async t => { + const server = createSecureServer( + { + ...pem, + allowHTTP1: false, + ALPNProtocols: ['http/1.1'] + }, + (req, res) => { + t.fail('Should not create a valid h2 stream') + } + ) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`https://localhost:${server.address().port}`, { + allowH2: false, + connect: { + rejectUnauthorized: false + } + }) + + t.teardown(server.close.bind(server)) + t.teardown(client.close.bind(client)) + t.plan(2) + + try { + await client.request({ + path: '/', + method: 'GET', + headers: { + 'x-my-header': 'foo' + } + }) + } catch (error) { + t.equal( + error.message, + 'Client network socket disconnected before secure TLS connection was established' + ) + t.equal(error.code, 'ECONNRESET') + } + } +) test('Should handle h2 continue', async t => { const requestBody = []