Skip to content

Commit

Permalink
Remove connectTimer from Connection.
Browse files Browse the repository at this point in the history
  • Loading branch information
@arthurschreiber
arthurschreiber committed Sep 12, 2024
1 parent c035b78 commit 2e4cf8b
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 163 deletions.
127 changes: 26 additions & 101 deletions src/connection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1026,10 +1026,6 @@ class Connection extends EventEmitter {
*/
declare messageBuffer: Buffer;

/**
* @private
*/
declare connectTimer: undefined | NodeJS.Timeout;
/**
* @private
*/
Expand Down Expand Up @@ -1969,7 +1965,23 @@ class Connection extends EventEmitter {
* @private
*/
initialiseConnection() {
const signal = this.createConnectTimer();
const controller = new AbortController();

setTimeout(() => {
const hostPostfix = this.config.options.port ? `:${this.config.options.port}` : `\\${this.config.options.instanceName}`;
// If we have routing data stored, this connection has been redirected
const server = this.routingData ? this.routingData.server : this.config.server;
const port = this.routingData ? `:${this.routingData.port}` : hostPostfix;
// Grab the target host from the connection configuration, and from a redirect message
// otherwise, leave the message empty.
const routingMessage = this.routingData ? ` (redirected from ${this.config.server}${hostPostfix})` : '';
const message = `Failed to connect to ${server}${port}${routingMessage} in ${this.config.options.connectTimeout}ms`;
this.debug.log(message);

controller.abort(new ConnectionError(message, 'ETIMEOUT'));
}, this.config.options.connectTimeout).unref();

const signal = controller.signal;

(async () => {
let port = this.config.options.port;
Expand All @@ -1983,9 +1995,8 @@ class Connection extends EventEmitter {
signal: signal
});
} catch (err: any) {
// Ignore the AbortError for now, this is still handled by the connectTimer firing
if (signal.aborted) {
return;
throw signal.reason;
}

throw new ConnectionError(err.message, 'EINSTLOOKUP', { cause: err });
Expand All @@ -1996,28 +2007,18 @@ class Connection extends EventEmitter {
try {
socket = await this.connectOnPort(port, this.config.options.multiSubnetFailover, signal, this.config.options.connector);
} catch (err: any) {
// Ignore the AbortError for now, this is still handled by the connectTimer firing
if (signal.aborted) {
return;
throw signal.reason;
}

throw this.wrapSocketError(err);
}

this.socketHandlingForSendPreLogin(socket);
this.sendPreLogin();
this.transitionTo(this.STATE.SENT_PRELOGIN);

try {
await this.performSentPrelogin(signal);
} catch (err: any) {
// Ignore the AbortError for now, this is still handled by the connectTimer firing
if (signal.aborted) {
return;
}

throw err;
}
this.transitionTo(this.STATE.SENT_PRELOGIN);
await this.performSentPrelogin(signal);

this.sendLogin7Packet();

Expand All @@ -2031,48 +2032,19 @@ class Connection extends EventEmitter {
case 'azure-active-directory-service-principal-secret':
case 'azure-active-directory-default':
this.transitionTo(this.STATE.SENT_LOGIN7_WITH_FEDAUTH);
try {
this.routingData = await this.performSentLogin7WithFedAuth(signal);
} catch (err) {
// Ignore the AbortError for now, this is still handled by the connectTimer firing
if (signal.aborted) {
return;
}

throw err;
}
this.routingData = await this.performSentLogin7WithFedAuth(signal);
break;
case 'ntlm':
this.transitionTo(this.STATE.SENT_LOGIN7_WITH_NTLM);
try {
this.routingData = await this.performSentLogin7WithNTLMLogin(signal);
} catch (err) {
// Ignore the AbortError for now, this is still handled by the connectTimer firing
if (signal.aborted) {
return;
}

throw err;
}
this.routingData = await this.performSentLogin7WithNTLMLogin(signal);
break;
default:
this.transitionTo(this.STATE.SENT_LOGIN7_WITH_STANDARD_LOGIN);
try {
this.routingData = await this.performSentLogin7WithStandardLogin(signal);
} catch (err) {
// Ignore the AbortError for now, this is still handled by the connectTimer firing
if (signal.aborted) {
return;
}

throw err;
}
this.routingData = await this.performSentLogin7WithStandardLogin(signal);
break;
}
} catch (err: any) {
if (isTransientError(err)) {
this.clearConnectTimer();

this.debug.log('Initiating retry on transient error');
this.transitionTo(this.STATE.TRANSIENT_FAILURE_RETRY);
this.performTransientFailureRetry();
Expand All @@ -2084,33 +2056,20 @@ class Connection extends EventEmitter {

// If routing data is present, we need to re-route the connection
if (this.routingData) {
this.clearConnectTimer();

this.transitionTo(this.STATE.REROUTING);
this.performReRouting();
return;
}

this.transitionTo(this.STATE.LOGGED_IN_SENDING_INITIAL_SQL);
try {
await this.performLoggedInSendingInitialSql(signal);
} catch (err) {
// Ignore the AbortError for now, this is still handled by the connectTimer firing
if (signal.aborted) {
return;
}

throw err;
}
await this.performLoggedInSendingInitialSql(signal);

this.transitionTo(this.STATE.LOGGED_IN);
this.clearConnectTimer();

process.nextTick(() => {
this.emit('connect');
});
})().catch((err) => {
this.clearConnectTimer();
this.transitionTo(this.STATE.FINAL);

process.nextTick(() => {
Expand All @@ -2124,7 +2083,6 @@ class Connection extends EventEmitter {
*/
cleanupConnection() {
if (!this.closed) {
this.clearConnectTimer();
this.clearRequestTimer();
this.closeConnection();

Expand Down Expand Up @@ -2266,18 +2224,6 @@ class Connection extends EventEmitter {
}
}

/**
* @private
*/
createConnectTimer() {
const controller = new AbortController();
this.connectTimer = setTimeout(() => {
controller.abort();
this.connectTimeout();
}, this.config.options.connectTimeout);
return controller.signal;
}

/**
* @private
*/
Expand Down Expand Up @@ -2309,18 +2255,7 @@ class Connection extends EventEmitter {
* @private
*/
connectTimeout() {
const hostPostfix = this.config.options.port ? `:${this.config.options.port}` : `\\${this.config.options.instanceName}`;
// If we have routing data stored, this connection has been redirected
const server = this.routingData ? this.routingData.server : this.config.server;
const port = this.routingData ? `:${this.routingData.port}` : hostPostfix;
// Grab the target host from the connection configuration, and from a redirect message
// otherwise, leave the message empty.
const routingMessage = this.routingData ? ` (redirected from ${this.config.server}${hostPostfix})` : '';
const message = `Failed to connect to ${server}${port}${routingMessage} in ${this.config.options.connectTimeout}ms`;
this.debug.log(message);
this.emit('connect', new ConnectionError(message, 'ETIMEOUT'));
this.connectTimer = undefined;
this.dispatchEvent('connectTimeout');

}

/**
Expand All @@ -2344,16 +2279,6 @@ class Connection extends EventEmitter {
request.error = new RequestError(message, 'ETIMEOUT');
}

/**
* @private
*/
clearConnectTimer() {
if (this.connectTimer) {
clearTimeout(this.connectTimer);
this.connectTimer = undefined;
}
}

/**
* @private
*/
Expand Down
62 changes: 0 additions & 62 deletions test/integration/connection-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,68 +232,6 @@ describe('Initiate Connect Test', function() {
connection.connect();
});

it('should clear timeouts when failing to connect', function(done) {
const connection = new Connection({
server: 'something.invalid',
options: { connectTimeout: 30000 },
});

if (process.env.TEDIOUS_DEBUG) {
connection.on('debug', console.log);
}

connection.on('connect', (err) => {
try {
assert.instanceOf(err, ConnectionError);
assert.strictEqual(/** @type {ConnectionError} */(err).code, 'ESOCKET');

assert.instanceOf(/** @type {ConnectionError} */(err).cause, Error);
assert.strictEqual(/** @type {Error} */(/** @type {ConnectionError} */(err).cause).message, 'getaddrinfo ENOTFOUND something.invalid');

assert.strictEqual(connection.connectTimer, undefined);
done();
} catch (e) {
done(e);
}
});

connection.on('error', done);
connection.connect();

assert.isOk(connection.connectTimer);
});

it('should clear timeouts when failing to connect to named instance', function(done) {
const connection = new Connection({
server: 'something.invalid',
options: {
instanceName: 'inst',
connectTimeout: 30000,
},
});

if (process.env.TEDIOUS_DEBUG) {
connection.on('debug', console.log);
}

connection.on('connect', (err) => {
assert.instanceOf(err, ConnectionError);
assert.strictEqual(/** @type {ConnectionError} */(err).code, 'EINSTLOOKUP');

assert.instanceOf(/** @type {ConnectionError} */(err).cause, Error);
assert.strictEqual(/** @type {Error} */(/** @type {ConnectionError} */(err).cause).message, 'getaddrinfo ENOTFOUND something.invalid');

assert.strictEqual(connection.connectTimer, undefined);

done();
});

connection.on('error', done);
connection.connect();

assert.isOk(connection.connectTimer);
});

it('should fail if no cipher can be negotiated', function(done) {
const config = getConfig();

Expand Down

0 comments on commit 2e4cf8b

Please sign in to comment.