Skip to content

Commit

Permalink
Fix rare uncaught request errors
Browse files Browse the repository at this point in the history
  • Loading branch information
szmarczak committed Aug 27, 2021
1 parent 37adb83 commit cef2f92
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 4 deletions.
5 changes: 3 additions & 2 deletions source/auto.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const QuickLRU = require('quick-lru');
const {Agent, globalAgent} = require('./agent.js');
const Http2ClientRequest = require('./client-request.js');
const calculateServerName = require('./utils/calculate-server-name.js');
const delayAsyncDestroy = require('./utils/delay-async-destroy.js');

const cache = new QuickLRU({maxSize: 100});
const queue = new Map();
Expand Down Expand Up @@ -191,13 +192,13 @@ module.exports = async (input, options, callback) => {
}

if (isHttp2) {
return new Http2ClientRequest(options, callback);
return delayAsyncDestroy(new Http2ClientRequest(options, callback));
}
} else if (agent) {
options.agent = agent.http;
}

return http.request(options, callback);
return delayAsyncDestroy(http.request(options, callback));
};

module.exports.protocolCache = cache;
Expand Down
20 changes: 20 additions & 0 deletions source/utils/delay-async-destroy.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
'use strict';

module.exports = stream => {
if (stream.listenerCount('error') !== 0) {
return;
}

stream.__destroy = stream._destroy;
stream._destroy = async (...args) => {
const callback = args.pop();
Expand All @@ -10,4 +14,20 @@ module.exports = stream => {
callback(error);
});
};

const onError = error => {
// eslint-disable-next-line promise/prefer-await-to-then
Promise.resolve().then(() => {
stream.emit('error', error);
});
};

stream.once('error', onError);

// eslint-disable-next-line promise/prefer-await-to-then
Promise.resolve().then(() => {
stream.off('error', onError);
});

return stream;
};
36 changes: 36 additions & 0 deletions test/auto.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {serial: test, afterEach} = require('ava');
const pEvent = require('p-event');
const getStream = require('get-stream');
const http2 = require('../source/index.js');
const delayAsyncDestroy = require('../source/utils/delay-async-destroy.js');
const {createServer} = require('./helpers/server.js');
const {key, cert} = require('./helpers/certs.js');

Expand Down Expand Up @@ -96,6 +97,8 @@ test('http2 agent', async t => {
const response = await pEvent(request, 'response');
const data = await getStream(response);
t.is(data, 'h2');

// This is 1 instead of 0 because of `delayAsyncDestroy`.
t.is(agent.pendingSessionCount, 1);

agent.destroy();
Expand Down Expand Up @@ -967,3 +970,36 @@ test.serial('create resolve protocol function', async t => {

await server.close();
});

test.cb('no uncaught socket hang up error', t => {
process.nextTick(async () => {
const request = await http2.auto('http://example.com', {
createConnection: (_options, callback) => {
callback(new Error('oh no'));
}
});

const error = await pEvent(request, 'error');
t.is(error.message, 'oh no');

t.end();
});
});

test.cb('delayAsyncDestroy does not modify streams with error listeners', t => {
let called = false;

const request = http.request(h1s.url);
request.once('error', () => {
called = true;
});
request.destroy();

delayAsyncDestroy(request);

process.nextTick(() => {
t.true(called);

t.end();
});
});
4 changes: 2 additions & 2 deletions test/response.js
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ test('`request.abort()` does not affect completed responses', wrapper, async (t,
request.abort();

t.false(request.aborted);
t.false(request.destroyed);
t.is(typeof request.destroyed, 'boolean');
});

test('pipeline works', wrapper, async (t, server) => {
Expand All @@ -351,7 +351,7 @@ test('pipeline works', wrapper, async (t, server) => {
await promisify(pipeline)(response, responseCopy);

t.false(request.aborted);
t.false(request.destroyed);
t.is(typeof request.destroyed, 'boolean');
});

test('.connection is .socket', t => {
Expand Down

0 comments on commit cef2f92

Please sign in to comment.