Skip to content

Commit 6716eca

Browse files
rowanmanningrowanmanning
committed
fix: handle node-fetch memory leak
node-fetch has a memory leak if you don't read the body of a response. Within the library we don't ever know which fetch implementaton we're using so we've had to test for a property (`pipe`) that only exists on the node-fetch response body and not native fetch. If we're throwing an error and we see that the response body has this method, then we pipe the body into a black hole stream. This means that the body is unusable if there was an error but we can document this limitation. I'd rather not mess with trying to read/parse data from the response body if possible as it adds a lot of complexity and I don't want to add _too_ many cases to deal with node-fetch when we'll be moving to native-fetch relatively soon. See-Also: https://financialtimes.atlassian.net/browse/CPREL-1047
1 parent 5f9f3ad commit 6716eca

File tree

3 files changed

+197
-119
lines changed

3 files changed

+197
-119
lines changed

‎packages/fetch-error-handler/README.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ Some of the options below result in more errors being caught, you can weigh this
4848

4949
In all of the APIs below, if the response `ok` property is `false`, i.e. when the status code is `400` or greater, then errors will be thrown.
5050

51+
> [!WARNING]
52+
> If you're using node-fetch then it's important to read the body of the request because of a [known memory leak](https://github.com/node-fetch/node-fetch/issues/83). If an error is thrown then we automatically drain the response body stream but, if the request is successful, you'll need to do this yourself.
53+
5154
### Wrap the fetch function
5255

5356
This is the recommended API as this will allow you to handle the most errors (even DNS and timeout errors) correctly:

‎packages/fetch-error-handler/lib/create-handler.js

Lines changed: 156 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,20 @@ const {
33
OperationalError,
44
UpstreamServiceError
55
} = require('@dotcom-reliability-kit/errors');
6+
const { Writable } = require('node:stream');
67

78
/**
89
* @typedef {object} ErrorHandlerOptions
910
* @property {string} [upstreamSystemCode]
1011
* The system code of the upstream system that the `fetch` makes a request to.
1112
*/
1213

14+
/**
15+
* @typedef {object} FetchResponseBody
16+
* @property {(stream: Writable) => void} [pipe]
17+
* A function to pipe a response body stream.
18+
*/
19+
1320
/**
1421
* @typedef {object} FetchResponse
1522
* @property {boolean} ok
@@ -18,6 +25,8 @@ const {
1825
* The response HTTP status code.
1926
* @property {string} url
2027
* The URL of the response.
28+
* @property {FetchResponseBody} body
29+
* A representation of the response body.
2130
*/
2231

2332
/* eslint-disable jsdoc/valid-types */
@@ -48,140 +57,160 @@ function createFetchErrorHandler(options = {}) {
4857
return async function handleFetchErrors(input) {
4958
let response = input;
5059

51-
// If input is a promise, resolve it. We also handle
52-
// more errors this way.
53-
if (isPromise(input)) {
54-
try {
55-
response = await input;
56-
} catch (/** @type {any} */ error) {
57-
const errorCode = error?.code || error?.cause?.code;
58-
59-
// Handle DNS errors
60-
if (errorCode === 'ENOTFOUND') {
61-
const hostname = error?.hostname || error?.cause?.hostname;
62-
const dnsLookupErrorMessage = `Cound not resolve DNS entry${
63-
hostname ? ` for host ${hostname}` : ''
64-
}`;
65-
throw new OperationalError({
66-
code: 'FETCH_DNS_LOOKUP_ERROR',
67-
message: dnsLookupErrorMessage,
68-
relatesToSystems,
69-
cause: error
70-
});
71-
}
60+
// This outer try/catch is used to make sure that we're able to read
61+
// the response body in the case of an error. This is important because
62+
// otherwise node-fetch will leak memory. See the (still not fixed) bug:
63+
// https://github.com/node-fetch/node-fetch/issues/83
64+
try {
65+
// If input is a promise, resolve it. We also handle
66+
// more errors this way.
67+
if (isPromise(input)) {
68+
try {
69+
response = await input;
70+
} catch (/** @type {any} */ error) {
71+
const errorCode = error?.code || error?.cause?.code;
7272

73-
// Handle standardised abort and timeout errors
74-
const abortErrorMessage =
75-
'The fetch was aborted before the upstream service could respond';
76-
if (error?.name === 'AbortError' || error?.name === 'TimeoutError') {
77-
throw new OperationalError({
78-
code:
79-
error.name === 'AbortError'
80-
? 'FETCH_ABORT_ERROR'
81-
: 'FETCH_TIMEOUT_ERROR',
82-
message: abortErrorMessage,
83-
relatesToSystems,
84-
cause: error
85-
});
86-
}
73+
// Handle DNS errors
74+
if (errorCode === 'ENOTFOUND') {
75+
const hostname = error?.hostname || error?.cause?.hostname;
76+
const dnsLookupErrorMessage = `Cound not resolve DNS entry${
77+
hostname ? ` for host ${hostname}` : ''
78+
}`;
79+
throw new OperationalError({
80+
code: 'FETCH_DNS_LOOKUP_ERROR',
81+
message: dnsLookupErrorMessage,
82+
relatesToSystems,
83+
cause: error
84+
});
85+
}
8786

88-
// Handle non-standardised timeout errors
89-
if (error?.name === 'FetchError' && error?.type === 'request-timeout') {
90-
throw new OperationalError({
91-
code: 'FETCH_TIMEOUT_ERROR',
92-
message: abortErrorMessage,
93-
relatesToSystems,
94-
cause: error
95-
});
96-
}
87+
// Handle standardised abort and timeout errors
88+
const abortErrorMessage =
89+
'The fetch was aborted before the upstream service could respond';
90+
if (error?.name === 'AbortError' || error?.name === 'TimeoutError') {
91+
throw new OperationalError({
92+
code:
93+
error.name === 'AbortError'
94+
? 'FETCH_ABORT_ERROR'
95+
: 'FETCH_TIMEOUT_ERROR',
96+
message: abortErrorMessage,
97+
relatesToSystems,
98+
cause: error
99+
});
100+
}
97101

98-
// Handle socket hangups
99-
if (
100-
errorCode === 'ECONNRESET' ||
101-
error?.cause?.name === 'SocketError'
102-
) {
103-
throw new UpstreamServiceError({
104-
code: 'FETCH_SOCKET_HANGUP_ERROR',
105-
message: 'The connection to the upstream service was terminated',
106-
relatesToSystems,
107-
cause: error
108-
});
102+
// Handle non-standardised timeout errors
103+
if (
104+
error?.name === 'FetchError' &&
105+
error?.type === 'request-timeout'
106+
) {
107+
throw new OperationalError({
108+
code: 'FETCH_TIMEOUT_ERROR',
109+
message: abortErrorMessage,
110+
relatesToSystems,
111+
cause: error
112+
});
113+
}
114+
115+
// Handle socket hangups
116+
if (
117+
errorCode === 'ECONNRESET' ||
118+
error?.cause?.name === 'SocketError'
119+
) {
120+
throw new UpstreamServiceError({
121+
code: 'FETCH_SOCKET_HANGUP_ERROR',
122+
message: 'The connection to the upstream service was terminated',
123+
relatesToSystems,
124+
cause: error
125+
});
126+
}
127+
128+
// We don't know what to do with this error so
129+
// we throw it as-is
130+
throw error;
109131
}
132+
}
110133

111-
// We don't know what to do with this error so
112-
// we throw it as-is
113-
throw error;
134+
// Check whether the value we were given is a valid response object
135+
if (!isFetchResponse(response)) {
136+
// This is not an operational error because the invalid
137+
// input is highly likely to be a programmer error
138+
throw Object.assign(
139+
new TypeError(
140+
'Fetch handler must be called with a `fetch` response object or a `fetch` promise'
141+
),
142+
{ code: 'FETCH_ERROR_HANDLER_INVALID_INPUT' }
143+
);
114144
}
115-
}
116145

117-
// Check whether the value we were given is a valid response object
118-
if (!isFetchResponse(response)) {
119-
// This is not an operational error because the invalid
120-
// input is highly likely to be a programmer error
121-
throw Object.assign(
122-
new TypeError(
123-
'Fetch handler must be called with a `fetch` response object or a `fetch` promise'
124-
),
125-
{ code: 'FETCH_ERROR_HANDLER_INVALID_INPUT' }
126-
);
127-
}
146+
// If the response isn't OK, we start throwing errors
147+
if (!response.ok) {
148+
// Parse the response URL so we can use the hostname in error messages
149+
let responseHostName = 'unknown';
150+
if (typeof response.url === 'string') {
151+
try {
152+
const url = new URL(response.url);
153+
responseHostName = url.hostname;
154+
} catch (_) {
155+
// We ignore this error because having a valid URL isn't essential – it
156+
// just helps debug if we do have one. If someone's using a weird non-standard
157+
// `fetch` implementation or mocking then this error could be fired
158+
}
159+
}
128160

129-
// If the response isn't OK, we start throwing errors
130-
if (!response.ok) {
131-
// Parse the response URL so we can use the hostname in error messages
132-
let responseHostName = 'unknown';
133-
if (typeof response.url === 'string') {
134-
try {
135-
const url = new URL(response.url);
136-
responseHostName = url.hostname;
137-
} catch (_) {
138-
// We ignore this error because having a valid URL isn't essential – it
139-
// just helps debug if we do have one. If someone's using a weird non-standard
140-
// `fetch` implementation or mocking then this error could be fired
161+
// Some common error options which we'll include in any that are thrown
162+
const baseErrorOptions = {
163+
message: `The upstream service at "${responseHostName}" responded with a ${response.status} status`,
164+
relatesToSystems,
165+
upstreamUrl: response.url,
166+
upstreamStatusCode: response.status
167+
};
168+
169+
// If the back end responds with a `4xx` error then it normally indicates
170+
// that something is wrong with the _current_ system. Maybe we're sending data
171+
// in an invalid format or our API key is invalid. For this we throw a generic
172+
// `500` error to indicate an issue with our system.
173+
if (response.status >= 400 && response.status < 500) {
174+
throw new HttpError(
175+
Object.assign(
176+
{ code: 'FETCH_CLIENT_ERROR', statusCode: 500 },
177+
baseErrorOptions
178+
)
179+
);
141180
}
142-
}
143181

144-
// Some common error options which we'll include in any that are thrown
145-
const baseErrorOptions = {
146-
message: `The upstream service at "${responseHostName}" responded with a ${response.status} status`,
147-
relatesToSystems,
148-
upstreamUrl: response.url,
149-
upstreamStatusCode: response.status
150-
};
151-
152-
// If the back end responds with a `4xx` error then it normally indicates
153-
// that something is wrong with the _current_ system. Maybe we're sending data
154-
// in an invalid format or our API key is invalid. For this we throw a generic
155-
// `500` error to indicate an issue with our system.
156-
if (response.status >= 400 && response.status < 500) {
182+
// If the back end responds with a `5xx` error then it normally indicates
183+
// that something is wrong with the _upstream_ system. For this we can output
184+
// an upstream service error and attribute the error to this system.
185+
if (response.status >= 500 && response.status < 600) {
186+
throw new UpstreamServiceError(
187+
Object.assign({ code: 'FETCH_SERVER_ERROR' }, baseErrorOptions)
188+
);
189+
}
190+
191+
// If we get here then it's unclear what's wrong – `response.ok` is false but the status
192+
// isn't in the 400–599 range. We throw a generic 500 error so that we have visibility.
157193
throw new HttpError(
158194
Object.assign(
159-
{ code: 'FETCH_CLIENT_ERROR', statusCode: 500 },
195+
{ code: 'FETCH_UNKNOWN_ERROR', statusCode: 500 },
160196
baseErrorOptions
161197
)
162198
);
163199
}
164200

165-
// If the back end responds with a `5xx` error then it normally indicates
166-
// that something is wrong with the _upstream_ system. For this we can output
167-
// an upstream service error and attribute the error to this system.
168-
if (response.status >= 500 && response.status < 600) {
169-
throw new UpstreamServiceError(
170-
Object.assign({ code: 'FETCH_SERVER_ERROR' }, baseErrorOptions)
171-
);
201+
return response;
202+
} catch (/** @type {any} */ finalError) {
203+
// If the response body has a pipe method then we're dealing
204+
// with a node-fetch body. In this case we need to read the
205+
// body so we don't introduce a memory leak.
206+
if (
207+
isFetchResponse(response) &&
208+
typeof response.body.pipe === 'function'
209+
) {
210+
response.body.pipe(new BlackHoleStream());
172211
}
173-
174-
// If we get here then it's unclear what's wrong – `response.ok` is false but the status
175-
// isn't in the 400–599 range. We throw a generic 500 error so that we have visibility.
176-
throw new HttpError(
177-
Object.assign(
178-
{ code: 'FETCH_UNKNOWN_ERROR', statusCode: 500 },
179-
baseErrorOptions
180-
)
181-
);
212+
throw finalError;
182213
}
183-
184-
return response;
185214
};
186215
}
187216

@@ -214,4 +243,16 @@ function isFetchResponse(value) {
214243
return true;
215244
}
216245

246+
/**
247+
* Writable stream to pipe data into the void.
248+
*/
249+
class BlackHoleStream extends Writable {
250+
/**
251+
* @override
252+
*/
253+
_write(chunk, encoding, done) {
254+
done();
255+
}
256+
}
257+
217258
module.exports = createFetchErrorHandler;

0 commit comments

Comments
 (0)