I am developing a HTTP proxy service and I have observed presence of an odd error message in my logs:
unhandledRejection RequestError: HTTP request error.
at /dev/rayroute/raygun/src/factories/createRequest.js:107:13
at processTicksAndRejections (internal/process/task_queues.js:93:5) {
code: 'RAYGUN_REQUEST_ERROR',
originalError: Error: test
at /dev/rayroute/raygun/src/factories/createRequest.js:73:29
at processTicksAndRejections (internal/process/task_queues.js:93:5)
It is odd because there are plethora of tests to ensure that all errors are handled. It is also odd because I have never never seen unhandled rejection while developing the service (only ever saw it in production logs).
The relevant code looks like this:
const activeRequestHandler = createRequest(requestDefinition);
if (incomingMessage.socket) {
incomingMessage.socket.on('close', () => {
if (responseIsReceived) {
log.trace('client disconnected after response');
} else {
log.debug('client disconnected');
activeRequestHandler.abort(new Error('CLIENT_DISCONNECTED'));
}
});
}
try {
await actions.afterRequestActions(
context,
requestDefinition,
activeRequestHandler
);
} catch (error) {
log.error({
error: serializeError(error),
}, 'afterRequest threw an error');
}
try {
responseDefinition = await activeRequestHandler.response;
} catch (error) {
log.warn({
error: serializeError(error),
}, 'an error occurred while waiting for a HTTP response');
// [..]
}
It is pretty straightforward:
-
createRequest
initiates a HTTP request and returns a request handler - the request handler can be used to abort the ongoing request (
afterRequestActions
aborts request after a hard-timeout); and - it is used to resolve the response or eventual rejection of the promise
I have written tests to ensure that errors are handled when:
- request rejected
- request aborted
-
afterRequestActions
throws an error
, but all tests are passing.
🤔
It turns out that the problem was that in all my test cases actions.afterRequestActions
was resolving/ being rejected before activeRequestHandler.response
is resolved. Meanwhile, in production afterRequestActions
contains logic that can take substantially longer to execute. I have also learned that even if you declare a try..catch
block for your async function, if it resolves before it is await
-ted, then you will get an unhandled rejection, i.e.
This code will not warn about unhandled rejection:
const delay = require('delay');
const main = async () => {
const promise = new Promise((resolve, reject) => {
setTimeout(() => {
reject(new Error('Expected rejection.'));
}, 100);
});
await delay(90);
try {
await promise;
} catch (error) {
console.error(error)
}
};
main();
But this code will always produce a warning about an unhandled rejection:
const delay = require('delay');
const main = async () => {
const promise = new Promise((resolve, reject) => {
setTimeout(() => {
reject(new Error('Expected rejection.'));
}, 100);
});
await delay(110);
try {
await promise;
} catch (error) {
console.error(error)
}
};
main();
The best solution is to add an auxiliary catch block, e.g. This is how I refactored my original code:
const activeRequestHandler = createRequest(requestDefinition);
// Without this we were getting occasional unhandledRejection errors.
// @see https://dev.to/gajus/handling-unhandled-promise-rejections-in-async-functions-5b2b
activeRequestHandler.response.catch((error) => {
log.warn({
error: serializeError(error),
}, 'an error occurred while waiting for a HTTP response (early warning)');
});
if (incomingMessage.socket) {
incomingMessage.socket.on('close', () => {
if (responseIsReceived) {
log.trace('client disconnected after response');
} else {
log.debug('client disconnected');
activeRequestHandler.abort(new Error('CLIENT_DISCONNECTED'));
}
});
}
try {
await actions.afterRequestActions(
context,
requestDefinition,
activeRequestHandler
);
} catch (error) {
log.error({
error: serializeError(error),
}, 'afterRequest threw an error');
}
try {
responseDefinition = await activeRequestHandler.response;
} catch (error) {
log.warn({
error: serializeError(error),
}, 'an error occurred while waiting for a HTTP response');
// [..]
}
Top comments (3)
Wow, it took me a bit to spot the problem before reading the rest of the post! I think there's a deeper problem, which is that
createRequest()
exposes a dangerous API to users by creating a reference to a promise and storing it implicitly.Anyone else using the API will have to be cognizant and use the same workaround. As an alternative, I like how the fetch API does things.
Maybe
createRequest()
should change its API?For what it is worth,
createRequest
is just a light abstraction around got. It is got that uses a cancellable promise to implement cancel (/abort) functionality.I do like your suggestion, though and I think it would be an improvement to
got
API. Please raise an issue with got. I am sure Sindre will appreciate the suggestion.The requirement to process interceptors in series is very much intentional.