Skip to content

poc: original request is not aborted on bodytimeout (only retry handler?) #4470

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
Uzlopak wants to merge 2 commits into
base: main
Choose a base branch
from
Open

poc: original request is not aborted on bodytimeout (only retry handler?) #4470

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ce580c
test: deflake test/iisue-3356.js
Uzlopak Aug 26, 2025
4fc4ee2
Merge branch 'main' into deflake-3356
Uzlopak Sep 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions lib/handler/retry-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,8 @@ class RetryHandler {
return
}

function shouldRetry (passedErr) {
const shouldRetryCb = (passedErr) => {
if (passedErr) {
this.headersSent = true

this.headersSent = true
this.handler.onResponseStart?.(controller, statusCode, headers, statusMessage)
controller.resume()
Expand All @@ -108,10 +106,11 @@ class RetryHandler {
this.retryOpts.retry(
err,
{
controller,
state: { counter: this.retryCount },
opts: { retryOptions: this.retryOpts, ...this.opts }
},
shouldRetry.bind(this)
shouldRetryCb
)
}

Expand All @@ -125,7 +124,7 @@ class RetryHandler {
this.handler.onRequestUpgrade?.(controller, statusCode, headers, socket)
}

static [kRetryHandlerDefaultRetry] (err, { state, opts }, cb) {
static [kRetryHandlerDefaultRetry] (err, { controller, state, opts }, shouldRetryCb) {

@Uzlopak Uzlopak Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

renamed it to shouldRetryCb so that it is easier to grok what it does.
controller is passed to potentially abort the request.

const { statusCode, code, headers } = err
const { method, retryOptions } = opts
const {
Expand All @@ -137,17 +136,18 @@ class RetryHandler {
errorCodes,
methods
} = retryOptions

const { counter } = state

// Any code that is not a Undici's originated and allowed to retry
if (code && code !== 'UND_ERR_REQ_RETRY' && !errorCodes.includes(code)) {
cb(err)
shouldRetryCb(err)
return
}

// If a set of method are provided and the current method is not in the list
if (Array.isArray(methods) && !methods.includes(method)) {
cb(err)
shouldRetryCb(err)
return
}

Expand All @@ -157,13 +157,24 @@ class RetryHandler {
Array.isArray(statusCodes) &&
!statusCodes.includes(statusCode)
) {
cb(err)
shouldRetryCb(err)
return
}

// If the error is a body timeout we want to abort the request
// as the server could be still sending data and we want to avoid
// to have multiple ongoing requests.
if (code === 'UND_ERR_BODY_TIMEOUT') {

@mcollina mcollina Aug 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why?

@Uzlopak Uzlopak Aug 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Use the modified test on main. The process hangs unrecoverable.

if (controller && !controller.aborted) {
controller.abort()
}
shouldRetryCb(err)
Comment on lines +164 to +171

@Uzlopak Uzlopak Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this feels so hacky.

return
}

// If we reached the max number of retries
if (counter > maxRetries) {
cb(err)
shouldRetryCb(err)
return
}

Expand All @@ -180,7 +191,9 @@ class RetryHandler {
? Math.min(retryAfterHeader, maxTimeout)
: Math.min(minTimeout * timeoutFactor ** (counter - 1), maxTimeout)

setTimeout(() => cb(null), retryTimeout)
setTimeout(() => {
shouldRetryCb(null)
}, retryTimeout)?.unref()
Comment on lines +194 to +196

@Uzlopak Uzlopak Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why I unref it? tbh... where do we actually clear the timer?

@metcoder95 metcoder95 Aug 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is not cleared as is not common request gets aborted on retry, tho having it as safe net when the controller.abort is called seems a good approach

}

onResponseStart (controller, statusCode, headers, statusMessage) {
Expand Down Expand Up @@ -362,7 +375,7 @@ class RetryHandler {
return
}

function shouldRetry (returnedErr) {
const shouldRetryCb = (returnedErr) => {
if (!returnedErr) {
this.retry(controller)
return
Expand All @@ -385,10 +398,11 @@ class RetryHandler {
this.retryOpts.retry(
err,
{
controller,
state: { counter: this.retryCount },
opts: { retryOptions: this.retryOpts, ...this.opts }
},
shouldRetry.bind(this)
shouldRetryCb

@Uzlopak Uzlopak Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because shouldRetryCb is now an arrow function, this points to the retry handler anyway.

)
}
}
Expand Down
83 changes: 59 additions & 24 deletions test/issue-3356.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,39 +2,31 @@

const { tspl } = require('@matteo.collina/tspl')
const { test, after } = require('node:test')
const { setTimeout: sleep } = require('node:timers/promises')
const { createServer } = require('node:http')
const { once } = require('node:events')
const { tick: fastTimersTick } = require('../lib/util/timers')
const { fetch, Agent, RetryAgent } = require('..')

test('https://github.com/nodejs/undici/issues/3356', { skip: process.env.CITGM }, async (t) => {
test('https://github.com/nodejs/undici/issues/3356', async (t) => {
t = tspl(t, { plan: 3 })

let shouldRetry = true
let callCount = 0
const server = createServer({ joinDuplicateHeaders: true })
server.on('request', (req, res) => {
res.writeHead(200, { 'content-type': 'text/plain' })
if (shouldRetry) {
shouldRetry = false
res.flushHeaders()
res.write('h')

res.flushHeaders()
res.write('h')
setTimeout(() => { res.end('ello world!') }, 100)
if (callCount++ === 0) {
res.write('ahahaha')
// never end the response

@Uzlopak Uzlopak Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

First i thought, to increase the timeouts, but then i thought: What happens if we never timeout?!

Original issue 3356 was, that we should ensure, that we dont concat the responses. solution was that non-206 responses should throw.

} else {
res.end('hello world!')
t.fail('should not be called twice')

@Uzlopak Uzlopak Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you run the code on main, you will see that we will call the routehandler twice! This means, that the retry handler makes the request twice. That doesnt seem right if we say, that responses with status 200 will not be able to process responses with content-range

@metcoder95 metcoder95 Aug 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Responses with 200 can mean that no more data is available (consumed all request) or the server just don't support range-request and will send the whole body instead.

The handler already covered that, but will need to check what was possibly wrong with it

@Uzlopak Uzlopak Aug 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

According to the issue, we had the problem that if we passed the response stream to the target stream, there is no way to "revert" that downstreamed data. E.g. we stream to a file stream, and partial data is written, response stream has issues, now retry, so we begin from the beginning to stream. Bam, double data.

The consensus of that issue was to handle it as an error and define the state of the request/response as non recoverable

Maybe my understanding is wrong. But this means that status 200 means that we dont retry. Of course we could consider that even if status 200 is thrown we retry and see if the response is a partial response with corresponding range headers set.
But i dont see it in my tests?!

We could have of course tried other approaches too. Like make a request and track transferred content on bytes, on error do retry sent range headers in hope it will accept it, and if there are no content-range headers dump bytes till we get new bytes and push them finally to the real stream. Such a behaviour should be configurable.

}
})

server.listen(0)

await once(server, 'listening')

after(async () => {
server.close()
await once(server.listen(0), 'listening')

await once(server, 'close')
})
after(() => once(server.close(), 'close'))

const agent = new RetryAgent(new Agent({ bodyTimeout: 50 }), {
errorCodes: ['UND_ERR_BODY_TIMEOUT']

@Uzlopak Uzlopak Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This means that UND_ERR_BODY_TIMEOUT should retry. But actually we decided, that it shuold not retry?

Expand All @@ -44,18 +36,61 @@ test('https://github.com/nodejs/undici/issues/3356', { skip: process.env.CITGM }
dispatcher: agent
})

fastTimersTick()

await sleep(500)
t.equal(response.status, 200)

try {
t.equal(response.status, 200)
// consume response
await response.text()
t.fail('should have thrown')
} catch (err) {
t.equal(err.name, 'TypeError')
t.equal(err.cause.code, 'UND_ERR_REQ_RETRY')
t.equal(err.cause.code, 'UND_ERR_BODY_TIMEOUT')
}

await t.completed
})

test('https://github.com/nodejs/undici/issues/3356', { skip: true }, async (t) => {

@Uzlopak Uzlopak Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I skipped this test, because it is not working. Maybe the logic for 206 with content-range is wrong.

@metcoder95 metcoder95 Aug 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since when is not working, or only not working with the new changes?

@Uzlopak Uzlopak Aug 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IIRC it also fails on main. But maybe the test setup is bad.

t = tspl(t, { plan: 2 })

let callCount = 0
const server = createServer({ joinDuplicateHeaders: true })
server.on('request', (req, res) => {
if (callCount++ === 0) {
res.writeHead(206, {
'content-type': 'text/plain',
'content-range': 'bytes 0-12/12'
})
res.flushHeaders()
res.write('h')

// never end the response
} else if (callCount === 1) {
console.log(req.headers['accept-ranges'])
res.writeHead(206, {
'content-type': 'text/plain',
'content-range': 'bytes 1-12/12'
})
res.flushHeaders()
res.write('ello world!')
res.end()
}
})

await once(server.listen(0), 'listening')

after(() => once(server.close(), 'close'))

const agent = new RetryAgent(new Agent({ bodyTimeout: 50 }), {
errorCodes: ['UND_ERR_BODY_TIMEOUT']
})

const response = await fetch(`http://localhost:${server.address().port}`, {
dispatcher: agent
})

t.equal(response.status, 206)

t.equal(await response.text(), 'hello world!')

await t.completed
})
4 changes: 2 additions & 2 deletions types/retry-handler.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ declare namespace RetryHandler {
*
* @type {Dispatcher.HttpMethod[]}
* @memberof RetryOptions
* @default ['GET', 'HEAD', 'OPTIONS', 'PUT', 'DELETE', 'TRACE'],
* @default Array<'GET','HEAD','OPTIONS','PUT','DELETE','TRACE'>,
*/
methods?: Dispatcher.HttpMethod[];
/**
Expand All @@ -113,7 +113,7 @@ declare namespace RetryHandler {
*
* @type {number[]}
* @memberof RetryOptions
* @default [500, 502, 503, 504, 429],
* @default Array<500,502,503,504,429>,
*/
statusCodes?: number[];
}
Expand Down
Loading