Skip to content

Commit bf84d36

Browse files
sindresorhussindresorhus
committed
Fix body reassignment in beforeRetry hooks
Fixes #2073
1 parent 9725fbd commit bf84d36

File tree

2 files changed

+345
-1
lines changed

2 files changed

+345
-1
lines changed

‎source/core/index.ts‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,9 @@ export default class Request extends Duplex implements RequestEvents<Request> {
446446
return;
447447
}
448448

449+
// Capture body BEFORE hooks run to detect reassignment
450+
const bodyBeforeHooks = this.options.body;
451+
449452
try {
450453
for (const hook of this.options.hooks.beforeRetry) {
451454
// eslint-disable-next-line no-await-in-loop
@@ -461,7 +464,48 @@ export default class Request extends Duplex implements RequestEvents<Request> {
461464
return;
462465
}
463466

464-
this.destroy();
467+
// Preserve stream body reassigned in beforeRetry hooks.
468+
const bodyAfterHooks = this.options.body;
469+
const bodyWasReassigned = bodyBeforeHooks !== bodyAfterHooks;
470+
471+
// Resource cleanup and preservation logic for retry with body reassignment.
472+
// The Promise wrapper (as-promise/index.ts) compares body identity to detect consumed streams,
473+
// so we must preserve the body reference across destroy(). However, destroy() calls _destroy()
474+
// which destroys this.options.body, creating a complex dance of clear/restore operations.
475+
//
476+
// Key constraints:
477+
// 1. If body was reassigned, we must NOT destroy the NEW stream (it will be used for retry)
478+
// 2. If body was reassigned, we MUST destroy the OLD stream to prevent memory leaks
479+
// 3. We must restore the body reference after destroy() for identity checks in promise wrapper
480+
// 4. We cannot use the normal setter after destroy() because it validates stream readability
481+
if (bodyWasReassigned) {
482+
const oldBody = bodyBeforeHooks;
483+
// Temporarily clear body to prevent destroy() from destroying the new stream
484+
this.options.body = undefined;
485+
this.destroy();
486+
487+
// Clean up the old stream resource if it's a stream and different from new body
488+
// (edge case: if old and new are same stream object, don't destroy it)
489+
if (is.nodeStream(oldBody) && oldBody !== bodyAfterHooks) {
490+
oldBody.destroy();
491+
}
492+
493+
// Restore new body for promise wrapper's identity check
494+
// We bypass the setter because it validates stream.readable (which fails for destroyed request)
495+
// Type assertion is necessary here to access private _internals without exposing internal API
496+
if (is.nodeStream(bodyAfterHooks) && (bodyAfterHooks.readableEnded || bodyAfterHooks.destroyed)) {
497+
throw new TypeError('The reassigned stream body must be readable. Ensure you provide a fresh, readable stream in the beforeRetry hook.');
498+
}
499+
500+
(this.options as any)._internals.body = bodyAfterHooks;
501+
} else {
502+
// Body wasn't reassigned - use normal destroy flow which handles body cleanup
503+
this.destroy();
504+
// Note: We do NOT restore the body reference here. The stream was destroyed by _destroy()
505+
// and should not be accessed. The promise wrapper will see that body identity hasn't changed
506+
// and will detect it's a consumed stream, which is the correct behavior.
507+
}
508+
465509
this.emit('retry', this.retryCount + 1, error, (updatedOptions?: OptionsInit) => {
466510
const request = new Request(options.url, updatedOptions, options);
467511
request.retryCount = this.retryCount + 1;

‎test/hooks.ts‎

Lines changed: 300 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1828,3 +1828,303 @@ test('beforeError hook can redact sensitive headers for ERR_UNSUPPORTED_PROTOCOL
18281828

18291829
t.is(error?.options.headers.authorization, '<redacted>');
18301830
});
1831+
1832+
test('beforeRetry can reassign plain stream body', withServer, async (t, server, got) => {
1833+
const {Readable: readable} = await import('node:stream');
1834+
let requestCount = 0;
1835+
const testData = 'Hello, Got!';
1836+
1837+
server.post('/retry', async (request, response) => {
1838+
requestCount++;
1839+
let body = '';
1840+
for await (const chunk of request) {
1841+
body += String(chunk);
1842+
}
1843+
1844+
// First request fails, second succeeds
1845+
if (requestCount === 1) {
1846+
response.statusCode = 500;
1847+
response.end('Server Error');
1848+
} else {
1849+
response.statusCode = 200;
1850+
response.end(`Received: ${body}`);
1851+
}
1852+
});
1853+
1854+
// Factory function to create fresh streams
1855+
const createStream = () => readable.from([testData]);
1856+
1857+
const response = await got.post('retry', {
1858+
body: createStream(),
1859+
retry: {
1860+
limit: 1,
1861+
methods: ['POST'],
1862+
},
1863+
hooks: {
1864+
beforeRetry: [
1865+
({options}) => {
1866+
// Reassign with a fresh stream - this previously failed
1867+
options.body = createStream();
1868+
},
1869+
],
1870+
},
1871+
});
1872+
1873+
t.is(response.statusCode, 200);
1874+
t.is(response.body, `Received: ${testData}`);
1875+
t.is(requestCount, 2);
1876+
});
1877+
1878+
test('beforeRetry destroys old stream when reassigning body', withServer, async (t, server, got) => {
1879+
const {Readable: readable} = await import('node:stream');
1880+
let requestCount = 0;
1881+
const testData = 'Stream data';
1882+
1883+
server.post('/retry', async (request, response) => {
1884+
requestCount++;
1885+
let body = '';
1886+
for await (const chunk of request) {
1887+
body += String(chunk);
1888+
}
1889+
1890+
// First request fails, second succeeds
1891+
if (requestCount === 1) {
1892+
response.statusCode = 500;
1893+
response.end('Server Error');
1894+
} else {
1895+
response.statusCode = 200;
1896+
response.end(`Received: ${body}`);
1897+
}
1898+
});
1899+
1900+
const createStream = () => readable.from([testData]);
1901+
const firstStream = createStream();
1902+
let oldStreamDestroyed = false;
1903+
1904+
// Monitor when the old stream is destroyed
1905+
firstStream.on('close', () => {
1906+
oldStreamDestroyed = true;
1907+
});
1908+
1909+
await got.post('retry', {
1910+
body: firstStream,
1911+
retry: {
1912+
limit: 1,
1913+
methods: ['POST'],
1914+
},
1915+
hooks: {
1916+
beforeRetry: [
1917+
({options}) => {
1918+
// Reassign with a fresh stream
1919+
options.body = createStream();
1920+
},
1921+
],
1922+
},
1923+
});
1924+
1925+
t.true(oldStreamDestroyed, 'Old stream should be destroyed to prevent memory leak');
1926+
t.is(requestCount, 2);
1927+
});
1928+
1929+
test('beforeRetry handles multiple retries with stream reassignment', withServer, async (t, server, got) => {
1930+
const {Readable: readable} = await import('node:stream');
1931+
let requestCount = 0;
1932+
const testData = 'Multi-retry data';
1933+
1934+
server.post('/retry', async (request, response) => {
1935+
requestCount++;
1936+
let body = '';
1937+
for await (const chunk of request) {
1938+
body += String(chunk);
1939+
}
1940+
1941+
// First two requests fail, third succeeds
1942+
if (requestCount < 3) {
1943+
response.statusCode = 500;
1944+
response.end('Server Error');
1945+
} else {
1946+
response.statusCode = 200;
1947+
response.end(`Received: ${body}`);
1948+
}
1949+
});
1950+
1951+
const createStream = () => readable.from([testData]);
1952+
const destroyedStreams: number[] = [];
1953+
let streamId = 0;
1954+
1955+
const response = await got.post('retry', {
1956+
body: createStream(),
1957+
retry: {
1958+
limit: 2,
1959+
methods: ['POST'],
1960+
},
1961+
hooks: {
1962+
beforeRetry: [
1963+
({options}) => {
1964+
const currentStreamId = ++streamId;
1965+
const newStream = createStream();
1966+
newStream.on('close', () => {
1967+
destroyedStreams.push(currentStreamId);
1968+
});
1969+
options.body = newStream;
1970+
},
1971+
],
1972+
},
1973+
});
1974+
1975+
t.is(response.statusCode, 200);
1976+
t.is(response.body, `Received: ${testData}`);
1977+
t.is(requestCount, 3);
1978+
t.is(destroyedStreams.length, 2, 'All old streams should be destroyed');
1979+
});
1980+
1981+
test('beforeRetry handles non-stream body reassignment', withServer, async (t, server, got) => {
1982+
let requestCount = 0;
1983+
1984+
server.post('/retry', async (request, response) => {
1985+
requestCount++;
1986+
let body = '';
1987+
for await (const chunk of request) {
1988+
body += String(chunk);
1989+
}
1990+
1991+
if (requestCount === 1) {
1992+
response.statusCode = 500;
1993+
response.end('Server Error');
1994+
} else {
1995+
response.statusCode = 200;
1996+
response.end(`Received: ${body}`);
1997+
}
1998+
});
1999+
2000+
const response = await got.post('retry', {
2001+
body: 'initial body',
2002+
retry: {
2003+
limit: 1,
2004+
methods: ['POST'],
2005+
},
2006+
hooks: {
2007+
beforeRetry: [
2008+
({options}) => {
2009+
// Reassign with a different string body
2010+
options.body = 'retried body';
2011+
},
2012+
],
2013+
},
2014+
});
2015+
2016+
t.is(response.statusCode, 200);
2017+
t.is(response.body, 'Received: retried body');
2018+
t.is(requestCount, 2);
2019+
});
2020+
2021+
test('beforeRetry handles body set to undefined', withServer, async (t, server, got) => {
2022+
const {Readable: readable} = await import('node:stream');
2023+
let requestCount = 0;
2024+
2025+
server.post('/retry', async (_request, response) => {
2026+
requestCount++;
2027+
// First request fails, second succeeds (with no body)
2028+
if (requestCount === 1) {
2029+
response.statusCode = 500;
2030+
response.end('Error');
2031+
} else {
2032+
response.statusCode = 200;
2033+
response.end('Success');
2034+
}
2035+
});
2036+
2037+
await got.post('retry', {
2038+
body: readable.from(['initial']),
2039+
retry: {
2040+
limit: 1,
2041+
methods: ['POST'],
2042+
},
2043+
hooks: {
2044+
beforeRetry: [
2045+
({options}) => {
2046+
options.body = undefined;
2047+
},
2048+
],
2049+
},
2050+
});
2051+
2052+
t.is(requestCount, 2);
2053+
});
2054+
2055+
test('beforeRetry handles body from undefined to stream', withServer, async (t, server, got) => {
2056+
const {Readable: readable} = await import('node:stream');
2057+
let requestCount = 0;
2058+
2059+
server.post('/retry', async (request, response) => {
2060+
requestCount++;
2061+
let body = '';
2062+
for await (const chunk of request) {
2063+
body += String(chunk);
2064+
}
2065+
2066+
if (requestCount === 1) {
2067+
response.statusCode = 500;
2068+
response.end('Error');
2069+
} else {
2070+
response.statusCode = 200;
2071+
response.end(`Got: ${body}`);
2072+
}
2073+
});
2074+
2075+
const response = await got.post('retry', {
2076+
retry: {
2077+
limit: 1,
2078+
methods: ['POST'],
2079+
},
2080+
hooks: {
2081+
beforeRetry: [
2082+
({options}) => {
2083+
options.body = readable.from(['stream-data']);
2084+
},
2085+
],
2086+
},
2087+
});
2088+
2089+
t.is(response.body, 'Got: stream-data');
2090+
t.is(requestCount, 2);
2091+
});
2092+
2093+
test('beforeRetry handles stream to Buffer conversion', withServer, async (t, server, got) => {
2094+
const {Readable: readable} = await import('node:stream');
2095+
let requestCount = 0;
2096+
2097+
server.post('/retry', async (request, response) => {
2098+
requestCount++;
2099+
let body = '';
2100+
for await (const chunk of request) {
2101+
body += String(chunk);
2102+
}
2103+
2104+
if (requestCount === 1) {
2105+
response.statusCode = 500;
2106+
response.end('Error');
2107+
} else {
2108+
response.statusCode = 200;
2109+
response.end(`Got: ${body}`);
2110+
}
2111+
});
2112+
2113+
const response = await got.post('retry', {
2114+
body: readable.from(['initial']),
2115+
retry: {
2116+
limit: 1,
2117+
methods: ['POST'],
2118+
},
2119+
hooks: {
2120+
beforeRetry: [
2121+
({options}) => {
2122+
options.body = Buffer.from('buffer-data');
2123+
},
2124+
],
2125+
},
2126+
});
2127+
2128+
t.is(response.body, 'Got: buffer-data');
2129+
t.is(requestCount, 2);
2130+
});

0 commit comments

Comments
 (0)