diff --git a/lib/core/request.js b/lib/core/request.js index 8e2072a46c7..3fcbab79c74 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -173,7 +173,7 @@ class Request { this.method = method - this.typeOfService = typeOfService ?? 0 + this.typeOfService = typeOfService this.abort = null diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index 6d89dd09b5e..63d4fc78500 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -60,6 +60,7 @@ const removeAllListeners = util.removeAllListeners const kIdleSocketValidation = Symbol('kIdleSocketValidation') const kIdleSocketValidationTimeout = Symbol('kIdleSocketValidationTimeout') const kSocketUsed = Symbol('kSocketUsed') +const kTypeOfService = Symbol('kTypeOfService') let extractBody @@ -1134,6 +1135,32 @@ function shouldSendContentLength (method) { return method !== 'GET' && method !== 'HEAD' && method !== 'OPTIONS' && method !== 'TRACE' && method !== 'CONNECT' } +function setTypeOfService (socket, request) { + if (typeof socket.setTypeOfService !== 'function') { + return + } + + const typeOfService = request.typeOfService + + if (typeOfService === undefined) { + return + } + + const currentTypeOfService = socket[kTypeOfService] + + if (currentTypeOfService === typeOfService) { + return + } + + try { + socket.setTypeOfService(typeOfService) + socket[kTypeOfService] = typeOfService + } catch { + // QoS marking is best-effort. setTypeOfService() can throw synchronously on + // some platforms depending on socket state, but that must not abort the request. + } +} + /** * @param {import('./client.js')} client * @param {import('../core/request.js')} request @@ -1265,9 +1292,7 @@ function writeH1 (client, request) { socket[kBlocking] = true } - if (socket.setTypeOfService) { - socket.setTypeOfService(request.typeOfService) - } + setTypeOfService(socket, request) let header = `${method} ${path} HTTP/1.1\r\n` diff --git a/test/ip-prioritization.js b/test/ip-prioritization.js index 8dc0412fb25..251cba93bec 100644 --- a/test/ip-prioritization.js +++ b/test/ip-prioritization.js @@ -1,12 +1,14 @@ 'use strict' +const assert = require('node:assert') const { test, after } = require('node:test') const { Client } = require('..') const { createServer } = require('node:http') const { once } = require('node:events') +const net = require('node:net') -test('HTTP/1.1 Request Prioritization', async (t) => { - let priority = null +test('HTTP/1.1 Request Prioritization', async () => { + const priorities = [] const server = createServer((req, res) => { res.end('ok') @@ -17,7 +19,7 @@ test('HTTP/1.1 Request Prioritization', async (t) => { const client = new Client(`http://localhost:${server.address().port}`, { connect: (opts, cb) => { - const socket = require('node:net').connect({ + const socket = net.connect({ ...opts, host: opts.hostname, port: opts.port @@ -25,7 +27,7 @@ test('HTTP/1.1 Request Prioritization', async (t) => { cb(null, socket) }) socket.setTypeOfService = (p) => { - priority = p + priorities.push(p) } return socket } @@ -33,20 +35,104 @@ test('HTTP/1.1 Request Prioritization', async (t) => { after(() => client.close()) after(() => server.close()) - await client.request({ + const response = await client.request({ path: '/', method: 'GET', typeOfService: 42 }) + await response.body.text() - // Check if priority was set - if (priority !== 42) { - throw new Error(`Expected priority 42, got ${priority}`) - } + const response2 = await client.request({ + path: '/', + method: 'GET' + }) + await response2.body.text() + + assert.deepStrictEqual(priorities, [42]) +}) + +// https://github.com/nodejs/undici/issues/5544 +// The default request path should not touch setsockopt on a fresh socket. +test('HTTP/1.1 Request Prioritization skips default ToS on fresh socket', async () => { + let calls = 0 + + const server = createServer((req, res) => { + res.end('ok') + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`, { + connect: (opts, cb) => { + const socket = net.connect({ + ...opts, + host: opts.hostname, + port: opts.port + }, () => { + cb(null, socket) + }) + socket.setTypeOfService = () => { + calls++ + throw new Error('setTypeOfService EINVAL') + } + return socket + } + }) + after(() => client.close()) + after(() => server.close()) + + const response = await client.request({ + path: '/', + method: 'GET' + }) + await response.body.text() + + assert.strictEqual(calls, 0) +}) + +// https://github.com/nodejs/undici/issues/5544 +// setTypeOfService() is best-effort and must not make the request fail. +test('HTTP/1.1 Request Prioritization ignores setTypeOfService errors', async () => { + const priorities = [] + + const server = createServer((req, res) => { + res.end('ok') + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`, { + connect: (opts, cb) => { + const socket = net.connect({ + ...opts, + host: opts.hostname, + port: opts.port + }, () => { + cb(null, socket) + }) + socket.setTypeOfService = (p) => { + priorities.push(p) + throw new Error('setTypeOfService EINVAL') + } + return socket + } + }) + after(() => client.close()) + after(() => server.close()) + + const response = await client.request({ + path: '/', + method: 'GET', + typeOfService: 42 + }) + + assert.strictEqual(await response.body.text(), 'ok') + assert.deepStrictEqual(priorities, [42]) }) test('HTTP/2 Connection Prioritization', async (t) => { - const net = require('node:net') const buildConnector = require('../lib/core/connect') let receivedHints = null diff --git a/types/dispatcher.d.ts b/types/dispatcher.d.ts index f732ec45cc9..001d14ede16 100644 --- a/types/dispatcher.d.ts +++ b/types/dispatcher.d.ts @@ -111,7 +111,7 @@ declare namespace Dispatcher { idempotent?: boolean; /** Whether the response is expected to take a long time and would end up blocking the pipeline. When this is set to `true` further pipelining will be avoided on the same connection until headers have been received. Defaults to `method !== 'HEAD'`. */ blocking?: boolean; - /** The IP Type of Service (ToS) value for the request socket. Must be an integer between 0 and 255. Default: `0` */ + /** The IP Type of Service (ToS) value for the request socket. Must be an integer between 0 and 255. */ typeOfService?: number | null; /** Upgrade the request. Should be used to specify the kind of upgrade i.e. `'Websocket'`. Default: `method === 'CONNECT' || null`. */ upgrade?: boolean | string | null;