Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prioritise error events over timeouts #1551

Merged
merged 3 commits into from Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 28 additions & 5 deletions lib/core/connect.js
Expand Up @@ -75,14 +75,12 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) {
})
}

const timeoutId = timeout
? setTimeout(onConnectTimeout, timeout, socket)
: null
const cancelTimeout = setupTimeout(() => onConnectTimeout(socket), timeout)

socket
.setNoDelay(true)
.once(protocol === 'https:' ? 'secureConnect' : 'connect', function () {
clearTimeout(timeoutId)
cancelTimeout()

if (callback) {
const cb = callback
Expand All @@ -91,7 +89,7 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) {
}
})
.on('error', function (err) {
clearTimeout(timeoutId)
cancelTimeout()

if (callback) {
const cb = callback
Expand All @@ -104,6 +102,31 @@ function buildConnector ({ maxCachedSessions, socketPath, timeout, ...opts }) {
}
}

function setupTimeout (onConnectTimeout, timeout) {
if (!timeout) {
return () => {}
}

let s1 = null
let s2 = null
const timeoutId = setTimeout(() => {
// setImmediate is added to make sure that we priotorise socket error events over timeouts
s1 = setImmediate(() => {
if (process.platform === 'win32') {
// Windows needs an extra setImmediate probably due to implementation differences in the socket logic
s2 = setImmediate(() => onConnectTimeout())
} else {
onConnectTimeout()
}
})
}, timeout)
return () => {
clearTimeout(timeoutId)
clearImmediate(s1)
clearImmediate(s2)
}
}

function onConnectTimeout (socket) {
util.destroy(socket, new ConnectTimeoutError())
}
Expand Down
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -67,6 +67,7 @@
"@sinonjs/fake-timers": "^9.1.2",
"@types/node": "^17.0.29",
"abort-controller": "^3.0.0",
"atomic-sleep": "^1.0.0",
"busboy": "^1.6.0",
"chai": "^4.3.4",
"chai-as-promised": "^7.1.1",
Expand Down
18 changes: 17 additions & 1 deletion test/connect-timeout.js
Expand Up @@ -3,8 +3,24 @@
const { test } = require('tap')
const { Client, Pool, errors } = require('..')
const net = require('net')
const sleep = require('atomic-sleep')

// Never connect
test('priotorise socket errors over timeouts', (t) => {
t.plan(1)
const connectTimeout = 1000
const client = new Pool('http://foobar.bar:1234', { connectTimeout })

client.request({ method: 'GET', path: '/foobar' })
.then(() => t.fail())
.catch((err) => {
t.equal(err.code, 'ENOTFOUND')
})

// block for 1001ms which is enough for the dns lookup to complete and TO to fire
sleep(connectTimeout + 1)
})

// never connect
net.connect = function (options) {
return new net.Socket(options)
}
Expand Down