From 9d41b627f620e46ee2c29464457735ba1b277f0c Mon Sep 17 00:00:00 2001 From: KaKa Date: Wed, 3 Jul 2024 18:31:58 +0800 Subject: [PATCH 1/3] fix: throw on retry when payload is consumed --- lib/handler/retry-handler.js | 15 ++++++++++ test/issue-3356.js | 57 ++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 test/issue-3356.js diff --git a/lib/handler/retry-handler.js b/lib/handler/retry-handler.js index f7dedfa4bac..d7ee04c50f3 100644 --- a/lib/handler/retry-handler.js +++ b/lib/handler/retry-handler.js @@ -68,6 +68,7 @@ class RetryHandler { this.end = null this.etag = null this.resume = null + this.consumed = false // Handle possible onConnect duplication this.handler.onConnect(reason => { @@ -193,6 +194,19 @@ class RetryHandler { this.resume = null if (statusCode !== 206) { + // Abort when status code is not partial content + // and payload is already consumed because downstream + // will concatenate response from two request wrongly. + if (this.consumed) { + this.abort( + new RequestRetryError('unsupported retry when payload is consumed', statusCode, { + headers, + data: { count: this.retryCount } + }) + ) + return false + } + return true } @@ -294,6 +308,7 @@ class RetryHandler { } onData (chunk) { + this.consumed = true this.start += chunk.length return this.handler.onData(chunk) diff --git a/test/issue-3356.js b/test/issue-3356.js new file mode 100644 index 00000000000..927208583a9 --- /dev/null +++ b/test/issue-3356.js @@ -0,0 +1,57 @@ +'use strict' + +const { tspl } = require('@matteo.collina/tspl') +const { test, after } = require('node:test') +const { createServer } = require('node:http') +const { once } = require('node:events') + +const { fetch, Agent, RetryAgent } = require('..') + +test('https://github.com/nodejs/undici/issues/3356', async (t) => { + t = tspl(t, { plan: 3 }) + + let shouldRetry = true + const server = createServer() + server.on('request', (req, res) => { + res.writeHead(200, { 'content-type': 'text/plain' }) + if (shouldRetry) { + shouldRetry = false + + res.flushHeaders() + res.write('h') + setTimeout(() => { res.end('ello world!') }, 100) + } else { + res.end('hello world!') + } + }) + + server.listen(0) + + await once(server, 'listening') + + after(async () => { + server.close() + + await once(server, '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 + }) + setTimeout(async () => { + try { + t.equal(response.status, 200) + // consume response + await response.text() + } catch (err) { + t.equal(err.name, 'TypeError') + t.equal(err.cause.code, 'UND_ERR_REQ_RETRY') + } + }, 200) + + await t.completed +}) From c1a61d7a237ba773779f164bcb0d4f26e5e36b74 Mon Sep 17 00:00:00 2001 From: KaKa Date: Wed, 3 Jul 2024 20:03:53 +0800 Subject: [PATCH 2/3] fixup --- lib/handler/retry-handler.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/handler/retry-handler.js b/lib/handler/retry-handler.js index d7ee04c50f3..f2d3fc7ba76 100644 --- a/lib/handler/retry-handler.js +++ b/lib/handler/retry-handler.js @@ -68,7 +68,6 @@ class RetryHandler { this.end = null this.etag = null this.resume = null - this.consumed = false // Handle possible onConnect duplication this.handler.onConnect(reason => { @@ -194,12 +193,13 @@ class RetryHandler { this.resume = null if (statusCode !== 206) { - // Abort when status code is not partial content - // and payload is already consumed because downstream - // will concatenate response from two request wrongly. - if (this.consumed) { + // Only Partial Content 206 supposed to provide Content-Range, + // any other status code that partially consumed the payload + // should not be retry because it would result in downstream + // wrongly concatanete multiple responses. + if (this.start > 0) { this.abort( - new RequestRetryError('unsupported retry when payload is consumed', statusCode, { + new RequestRetryError('server does not support the range header and the payload was partially consumed', statusCode, { headers, data: { count: this.retryCount } }) @@ -308,7 +308,6 @@ class RetryHandler { } onData (chunk) { - this.consumed = true this.start += chunk.length return this.handler.onData(chunk) From 3c310ea4d2a857d1c41952b7d5bbf522a61b3264 Mon Sep 17 00:00:00 2001 From: KaKa Date: Wed, 3 Jul 2024 20:12:21 +0800 Subject: [PATCH 3/3] fixup --- lib/handler/retry-handler.js | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/lib/handler/retry-handler.js b/lib/handler/retry-handler.js index f2d3fc7ba76..e00e82faf08 100644 --- a/lib/handler/retry-handler.js +++ b/lib/handler/retry-handler.js @@ -192,22 +192,18 @@ class RetryHandler { if (this.resume != null) { this.resume = null - if (statusCode !== 206) { - // Only Partial Content 206 supposed to provide Content-Range, - // any other status code that partially consumed the payload - // should not be retry because it would result in downstream - // wrongly concatanete multiple responses. - if (this.start > 0) { - this.abort( - new RequestRetryError('server does not support the range header and the payload was partially consumed', statusCode, { - headers, - data: { count: this.retryCount } - }) - ) - return false - } - - return true + // Only Partial Content 206 supposed to provide Content-Range, + // any other status code that partially consumed the payload + // should not be retry because it would result in downstream + // wrongly concatanete multiple responses. + if (statusCode !== 206 && (this.start > 0 || statusCode !== 200)) { + this.abort( + new RequestRetryError('server does not support the range header and the payload was partially consumed', statusCode, { + headers, + data: { count: this.retryCount } + }) + ) + return false } const contentRange = parseRangeHeader(headers['content-range'])