diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml new file mode 100644 index 00000000000..deabbc6dc78 --- /dev/null +++ b/.github/workflows/nightly.yml @@ -0,0 +1,62 @@ +name: Nightly tests + +on: + workflow_dispatch: + schedule: + - cron: "0 10 * * *" + +jobs: + test: + if: github.repository == 'nodejs/undici' + strategy: + fail-fast: false + max-parallel: 0 + matrix: + runs-on: + - ubuntu-latest + - windows-latest + - macos-latest + uses: ./.github/workflows/test.yml + with: + node-version: 22-nightly + runs-on: ${{ matrix.runs-on }} + secrets: inherit + + report-failure: + if: failure() + needs: test + runs-on: ubuntu-latest + permissions: + issues: write + steps: + - name: Create or update issue + uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 + with: + script: | + const ISSUE_TITLE = "Nightly tests are failing" + + const actionRunUrl = "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" + + const issueContext = { + owner: context.repo.owner, + repo: context.repo.repo + } + + let issue = (await github.rest.issues.listForRepo({ + state: "open", + creator: "github-actions[bot]", + ...issueContext + })).data.find((issue) => issue.title === ISSUE_TITLE) + + if(!issue) { + issue = (await github.rest.issues.create({ + title: ISSUE_TITLE, + ...issueContext + })).data + } + + await github.rest.issues.createComment({ + issue_number: issue.number, + body: `Tests against nightly failed, see: ${actionRunUrl}`, + ...issueContext + }); diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index fa1d3539a5a..6373d4ac7e7 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -51,8 +51,6 @@ jobs: run: npm run lint test: - name: Test with Node.js ${{ matrix.node-version }} on ${{ matrix.runs-on }} - timeout-minutes: 15 strategy: fail-fast: false max-parallel: 0 @@ -61,48 +59,15 @@ jobs: - 18 - 20 - 21 - runs-on: + runs-on: - ubuntu-latest - windows-latest - macos-latest - - runs-on: ${{ matrix.runs-on }} - steps: - - name: Checkout - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 - with: - persist-credentials: false - - - name: Setup Node.js@${{ matrix.node-version }} - uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 - with: - node-version: ${{ matrix.node-version }} - - - name: Print version information - run: | - echo OS: $(node -p "os.version()") - echo Node.js: $(node --version) - echo npm: $(npm --version) - echo git: $(git --version) - - - name: Install dependencies - run: npm install - - - name: Print installed dependencies - run: npm ls --all - continue-on-error: true - - - name: Run tests - run: npm run coverage:ci - env: - CI: true - NODE_V8_COVERAGE: ./coverage/tmp - - - name: Coverage Report - if: matrix.runs-on == 'ubuntu-latest' && matrix.node-version == 20 - uses: codecov/codecov-action@54bcd8715eee62d40e33596ef5e8f0f48dbbccab # v4.1.0 - with: - token: ${{ secrets.CODECOV_TOKEN }} + uses: ./.github/workflows/test.yml + with: + node-version: ${{ matrix.node-version }} + runs-on: ${{ matrix.runs-on }} + secrets: inherit test-types: name: Test TypeScript types diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 00000000000..9cebbb20bef --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,53 @@ +name: Run tests + +on: + workflow_call: + inputs: + node-version: + required: true + type: string + runs-on: + required: true + type: string + +jobs: + test: + name: Test with Node.js ${{ inputs.node-version }} on ${{ inputs.runs-on }} + timeout-minutes: 15 + runs-on: ${{ inputs.runs-on }} + steps: + - name: Checkout + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + with: + persist-credentials: false + + - name: Setup Node.js@${{ inputs.node-version }} + uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 + with: + node-version: ${{ inputs.node-version }} + + - name: Print version information + run: | + echo OS: $(node -p "os.version()") + echo Node.js: $(node --version) + echo npm: $(npm --version) + echo git: $(git --version) + + - name: Install dependencies + run: npm install + + - name: Print installed dependencies + run: npm ls --all + continue-on-error: true + + - name: Run tests + run: npm run coverage:ci + env: + CI: true + NODE_V8_COVERAGE: ./coverage/tmp + + - name: Coverage Report + if: inputs.runs-on == 'ubuntu-latest' && inputs.node-version == 20 + uses: codecov/codecov-action@54bcd8715eee62d40e33596ef5e8f0f48dbbccab # v4.1.0 + with: + token: ${{ secrets.CODECOV_TOKEN }} diff --git a/build/Dockerfile b/build/Dockerfile index d74cd6d44f1..180c39fa6f6 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -1,4 +1,4 @@ -FROM node:21-alpine@sha256:4999fa1391e09259e71845d3d0e9ddfe5f51ab30253c8b490c633f710c7446a0 +FROM node:21-alpine@sha256:577f8eb599858005100d84ef3fb6bd6582c1b6b17877a393cdae4bfc9935f068 ARG UID=1000 ARG GID=1000 diff --git a/docs/docs/api/RetryHandler.md b/docs/docs/api/RetryHandler.md index 6a932da8bdc..6dbc5077d02 100644 --- a/docs/docs/api/RetryHandler.md +++ b/docs/docs/api/RetryHandler.md @@ -35,6 +35,12 @@ Extends: [`Dispatch.DispatchOptions`](Dispatcher.md#parameter-dispatchoptions). - `state`: `RetryState` - Current retry state. It can be mutated. - `opts`: `Dispatch.DispatchOptions & RetryOptions` - Options passed to the retry handler. +**`RetryState`** + +It represents the retry state for a given request. + +- `counter`: `number` - Current retry attempt. + ### Parameter `RetryHandlers` - **dispatch** `(options: Dispatch.DispatchOptions, handlers: Dispatch.DispatchHandlers) => Promise` (required) - Dispatch function to be called after every retry. diff --git a/lib/handler/retry-handler.js b/lib/handler/retry-handler.js index f2e22f6d4c0..204025c769a 100644 --- a/lib/handler/retry-handler.js +++ b/lib/handler/retry-handler.js @@ -1,3 +1,4 @@ +'use strict' const assert = require('node:assert') const { kRetryHandlerDefaultRetry } = require('../core/symbols') @@ -37,7 +38,7 @@ class RetryHandler { retry: retryFn ?? RetryHandler[kRetryHandlerDefaultRetry], retryAfter: retryAfter ?? true, maxTimeout: maxTimeout ?? 30 * 1000, // 30s, - timeout: minTimeout ?? 500, // .5s + minTimeout: minTimeout ?? 500, // .5s timeoutFactor: timeoutFactor ?? 2, maxRetries: maxRetries ?? 5, // What errors we should retry @@ -59,6 +60,7 @@ class RetryHandler { } this.retryCount = 0 + this.retryCountCheckpoint = 0 this.start = 0 this.end = null this.etag = null @@ -104,17 +106,14 @@ class RetryHandler { const { method, retryOptions } = opts const { maxRetries, - timeout, + minTimeout, maxTimeout, timeoutFactor, statusCodes, errorCodes, methods } = retryOptions - let { counter, currentTimeout } = state - - currentTimeout = - currentTimeout != null && currentTimeout > 0 ? currentTimeout : timeout + const { counter } = state // Any code that is not a Undici's originated and allowed to retry if ( @@ -159,9 +158,7 @@ class RetryHandler { const retryTimeout = retryAfterHeader > 0 ? Math.min(retryAfterHeader, maxTimeout) - : Math.min(currentTimeout * timeoutFactor ** counter, maxTimeout) - - state.currentTimeout = retryTimeout + : Math.min(minTimeout * timeoutFactor ** (counter - 1), maxTimeout) setTimeout(() => cb(null), retryTimeout) } @@ -309,10 +306,19 @@ class RetryHandler { return this.handler.onError(err) } + // We reconcile in case of a mix between network errors + // and server error response + if (this.retryCount - this.retryCountCheckpoint > 0) { + // We count the difference between the last checkpoint and the current retry count + this.retryCount = this.retryCountCheckpoint + (this.retryCount - this.retryCountCheckpoint) + } else { + this.retryCount += 1 + } + this.retryOpts.retry( err, { - state: { counter: this.retryCount++, currentTimeout: this.retryAfter }, + state: { counter: this.retryCount }, opts: { retryOptions: this.retryOpts, ...this.opts } }, onRetry.bind(this) @@ -334,6 +340,7 @@ class RetryHandler { } try { + this.retryCountCheckpoint = this.retryCount this.dispatch(this.opts, this) } catch (err) { this.handler.onError(err) diff --git a/lib/web/fetch/formdata.js b/lib/web/fetch/formdata.js index 0e95a534f11..9e7d6d0e4ad 100644 --- a/lib/web/fetch/formdata.js +++ b/lib/web/fetch/formdata.js @@ -157,12 +157,27 @@ class FormData { } [nodeUtil.inspect.custom] (depth, options) { - let output = 'FormData:\n' - this[kState].forEach(entry => { - output += `${entry.name}: ${entry.value}\n` - }) + const state = this[kState].reduce((a, b) => { + if (a[b.name]) { + if (Array.isArray(a[b.name])) { + a[b.name].push(b.value) + } else { + a[b.name] = [a[b.name], b.value] + } + } else { + a[b.name] = b.value + } + + return a + }, { __proto__: null }) + + options.depth ??= depth + options.colors ??= true + + const output = nodeUtil.formatWithOptions(options, state) - return output + // remove [Object null prototype] + return `FormData ${output.slice(output.indexOf(']') + 2)}` } } diff --git a/lib/web/fetch/headers.js b/lib/web/fetch/headers.js index b2580e90e04..b849b0e356c 100644 --- a/lib/web/fetch/headers.js +++ b/lib/web/fetch/headers.js @@ -572,16 +572,10 @@ class Headers { return (this[kHeadersList][kHeadersSortedMap] = headers) } - [Symbol.for('nodejs.util.inspect.custom')] () { - webidl.brandCheck(this, Headers) - - return this[kHeadersList] - } - [util.inspect.custom] (depth, options) { - const inspected = util.inspect(this[kHeadersList].entries) + options.depth ??= depth - return `Headers ${inspected}` + return `Headers ${util.formatWithOptions(options, this[kHeadersList].entries)}` } } diff --git a/lib/web/fetch/request.js b/lib/web/fetch/request.js index 82de276060b..a35b90bf9f3 100644 --- a/lib/web/fetch/request.js +++ b/lib/web/fetch/request.js @@ -778,6 +778,8 @@ class Request { options.depth = 2 } + options.colors ??= true + const properties = { method: this.method, url: this.url, @@ -796,7 +798,7 @@ class Request { signal: this.signal } - return nodeUtil.formatWithOptions(options, { ...properties }) + return `Request ${nodeUtil.formatWithOptions(options, properties)}` } } diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index bcb54237fcc..182e3dc0199 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -259,6 +259,8 @@ class Response { options.depth = 2 } + options.colors ??= true + const properties = { status: this.status, statusText: this.statusText, @@ -271,7 +273,7 @@ class Response { url: this.url } - return nodeUtil.formatWithOptions(options, `Response ${nodeUtil.inspect(properties)}`) + return `Response ${nodeUtil.formatWithOptions(options, properties)}` } } diff --git a/package.json b/package.json index 91fec318a7b..4dd682aaa7f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "undici", - "version": "6.9.0", + "version": "6.10.0", "description": "An HTTP/1.1 client, written from scratch for Node.js", "homepage": "https://undici.nodejs.org", "bugs": { diff --git a/test/fetch/formdata-inspect-custom.js b/test/fetch/formdata-inspect-custom.js index e52ccdf3fdd..4fb70069439 100644 --- a/test/fetch/formdata-inspect-custom.js +++ b/test/fetch/formdata-inspect-custom.js @@ -10,7 +10,7 @@ test('FormData class custom inspection', () => { formData.append('username', 'john_doe') formData.append('email', 'john@example.com') - const expectedOutput = 'FormData:\nusername: john_doe\nemail: john@example.com\n' + const expectedOutput = "FormData {\n username: 'john_doe',\n email: 'john@example.com'\n}" assert.deepStrictEqual(inspect(formData), expectedOutput) }) diff --git a/test/fetch/request-inspect-custom.js b/test/fetch/request-inspect-custom.js index 7f4290d1d4e..7cb7fc73152 100644 --- a/test/fetch/request-inspect-custom.js +++ b/test/fetch/request-inspect-custom.js @@ -16,7 +16,7 @@ describe('Request custom inspection', () => { const inspectedOutput = util.inspect(request) - const expectedOutput = '{\n method: \'POST\',\n url: \'https://example.com/api\',\n headers: Headers { \'Content-Type\': \'application/json\' },\n destination: \'\',\n referrer: \'about:client\',\n referrerPolicy: \'\',\n mode: \'cors\',\n credentials: \'same-origin\',\n cache: \'default\',\n redirect: \'follow\',\n integrity: \'\',\n keepalive: false,\n isReloadNavigation: false,\n isHistoryNavigation: false,\n signal: AbortSignal { aborted: false }\n}' + const expectedOutput = "Request {\n method: 'POST',\n url: 'https://example.com/api',\n headers: Headers { 'Content-Type': 'application/json' },\n destination: '',\n referrer: 'about:client',\n referrerPolicy: '',\n mode: 'cors',\n credentials: 'same-origin',\n cache: 'default',\n redirect: 'follow',\n integrity: '',\n keepalive: false,\n isReloadNavigation: false,\n isHistoryNavigation: false,\n signal: AbortSignal { aborted: false }\n}" assert.strictEqual(inspectedOutput, expectedOutput) }) }) diff --git a/test/issue-803.js b/test/issue-803.js index 849bc1aba47..0ab94cd1d37 100644 --- a/test/issue-803.js +++ b/test/issue-803.js @@ -5,22 +5,26 @@ const { test, after } = require('node:test') const { once } = require('node:events') const { Client } = require('..') const { createServer } = require('node:http') -const EE = require('node:events') -test('https://github.com/nodejs/undici/issues/803', async (t) => { +test('https://github.com/nodejs/undici/issues/803', { timeout: 60000 }, async (t) => { t = tspl(t, { plan: 2 }) const SIZE = 5900373096 + const chunkSize = 65536 + const parts = (SIZE / chunkSize) | 0 + const lastPartSize = SIZE % chunkSize + const chunk = Buffer.allocUnsafe(chunkSize) const server = createServer(async (req, res) => { res.setHeader('content-length', SIZE) - let pos = 0 - while (pos < SIZE) { - const len = Math.min(SIZE - pos, 65536) - if (!res.write(Buffer.allocUnsafe(len))) { - await EE.once(res, 'drain') + let i = 0 + while (i++ < parts) { + if (res.write(chunk) === false) { + await once(res, 'drain') } - pos += len + } + if (res.write(chunk.subarray(0, lastPartSize)) === false) { + await once(res, 'drain') } res.end() diff --git a/test/retry-handler.js b/test/retry-handler.js index f6b83cd34dd..1cbeed56ef8 100644 --- a/test/retry-handler.js +++ b/test/retry-handler.js @@ -17,7 +17,105 @@ test('Should retry status code', async t => { const dispatchOptions = { retryOptions: { retry: (err, { state, opts }, done) => { - counter++ + ++counter + + if ( + err.statusCode === 500 || + err.message.includes('other side closed') + ) { + setTimeout(done, 500) + return + } + + return done(err) + } + }, + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + } + + server.on('request', (req, res) => { + switch (counter) { + case 0: + req.destroy() + return + case 1: + res.writeHead(500) + res.end('failed') + return + case 2: + res.writeHead(200) + res.end('hello world!') + return + default: + t.fail() + } + }) + + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`) + const handler = new RetryHandler(dispatchOptions, { + dispatch: client.dispatch.bind(client), + handler: { + onConnect () { + t.ok(true, 'pass') + }, + onBodySent () { + t.ok(true, 'pass') + }, + onHeaders (status, _rawHeaders, resume, _statusMessage) { + t.strictEqual(status, 200) + return true + }, + onData (chunk) { + chunks.push(chunk) + return true + }, + onComplete () { + t.strictEqual(Buffer.concat(chunks).toString('utf-8'), 'hello world!') + t.strictEqual(counter, 2) + }, + onError () { + t.fail() + } + } + }) + + after(async () => { + await client.close() + server.close() + + await once(server, 'close') + }) + + client.dispatch( + { + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + }, + handler + ) + }) + + await t.completed +}) + +test('Should account for network and response errors', async t => { + t = tspl(t, { plan: 4 }) + + let counter = 0 + const chunks = [] + const server = createServer() + const dispatchOptions = { + retryOptions: { + retry: (err, { state, opts }, done) => { + counter = state.counter if ( err.statusCode === 500 || @@ -706,3 +804,75 @@ test('should not error if request is not meant to be retried', async t => { await t.completed }) + +test('Should be able to properly pass the minTimeout to the RetryContext when constructing a RetryCallback function', async t => { + t = tspl(t, { plan: 2 }) + + let counter = 0 + const server = createServer() + server.on('request', (req, res) => { + switch (counter) { + case 0: + res.writeHead(500) + res.end('failed') + return + case 1: + res.writeHead(200) + res.end('hello world!') + return + default: + t.fail() + } + }) + + const dispatchOptions = { + retryOptions: { + retry: (err, { state, opts }, done) => { + counter++ + t.strictEqual(opts.retryOptions.minTimeout, 100) + + if (err.statusCode === 500) { + return done() + } + + return done(err) + }, + minTimeout: 100 + }, + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + } + + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`) + const handler = new RetryHandler(dispatchOptions, { + dispatch: client.dispatch.bind(client), + handler: new RequestHandler(dispatchOptions, (err, data) => { + t.ifError(err) + }) + }) + + after(async () => { + await client.close() + server.close() + + await once(server, 'close') + }) + + client.dispatch( + { + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + }, + handler + ) + }) + + await t.completed +}) diff --git a/test/types/dispatcher.test-d.ts b/test/types/dispatcher.test-d.ts index 275d88c4af9..245ee218d59 100644 --- a/test/types/dispatcher.test-d.ts +++ b/test/types/dispatcher.test-d.ts @@ -127,6 +127,12 @@ expectAssignable(new Dispatcher()) declare const { body }: Dispatcher.ResponseData; +// compose +{ + expectAssignable(new Dispatcher().compose(new Dispatcher().dispatch, new Dispatcher().dispatch)) + expectAssignable(new Dispatcher().compose([new Dispatcher().dispatch, new Dispatcher().dispatch])) +} + { // body mixin tests expectType(body.body) diff --git a/types/dispatcher.d.ts b/types/dispatcher.d.ts index 08c59ca0718..e632e0e921a 100644 --- a/types/dispatcher.d.ts +++ b/types/dispatcher.d.ts @@ -18,6 +18,9 @@ declare class Dispatcher extends EventEmitter { /** Starts two-way communications with the requested resource. */ connect(options: Dispatcher.ConnectOptions): Promise; connect(options: Dispatcher.ConnectOptions, callback: (err: Error | null, data: Dispatcher.ConnectData) => void): void; + /** Compose a chain of dispatchers */ + compose(dispatchers: Dispatcher['dispatch'][]): Dispatcher.ComposedDispatcher; + compose(...dispatchers: Dispatcher['dispatch'][]): Dispatcher.ComposedDispatcher; /** Performs an HTTP request. */ request(options: Dispatcher.RequestOptions): Promise; request(options: Dispatcher.RequestOptions, callback: (err: Error | null, data: Dispatcher.ResponseData) => void): void; @@ -93,6 +96,8 @@ declare class Dispatcher extends EventEmitter { } declare namespace Dispatcher { + export interface ComposedDispatcher extends Dispatcher {} + export type DispatcherInterceptor = (dispatch: Dispatcher['dispatch']) => Dispatcher['dispatch']; export interface DispatchOptions { origin?: string | URL; path: string; diff --git a/types/retry-handler.d.ts b/types/retry-handler.d.ts index 0528eb44279..e44b207c221 100644 --- a/types/retry-handler.d.ts +++ b/types/retry-handler.d.ts @@ -12,7 +12,7 @@ declare class RetryHandler implements Dispatcher.DispatchHandlers { } declare namespace RetryHandler { - export type RetryState = { counter: number; currentTimeout: number }; + export type RetryState = { counter: number; }; export type RetryContext = { state: RetryState;