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

replace Bluebird.timeout #3634

Merged
merged 5 commits into from Feb 12, 2020
Merged
Show file tree
Hide file tree
Changes from 3 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
15 changes: 10 additions & 5 deletions lib/client.js
Expand Up @@ -25,6 +25,7 @@ const { makeEscape } = require('./query/string');
const { uniqueId, cloneDeep, defaults } = require('lodash');

const Logger = require('./logger');
const { KnexTimeoutError } = require('./util/timeout');

const debug = require('debug')('knex:client');
const _debugQuery = require('debug')('knex:query');
Expand Down Expand Up @@ -344,11 +345,15 @@ Object.assign(Client.prototype, {
debug('acquired connection from pool: %s', connection.__knexUid);
return connection;
})
.catch(TimeoutError, () => {
throw new Bluebird.TimeoutError(
'Knex: Timeout acquiring a connection. The pool is probably full. ' +
'Are you missing a .transacting(trx) call?'
);
.catch((error) => {
let convertedError = error;
if (error instanceof TimeoutError) {
convertedError = new KnexTimeoutError(
'Knex: Timeout acquiring a connection. The pool is probably full. ' +
'Are you missing a .transacting(trx) call?'
);
}
throw convertedError;
});
} catch (e) {
return Bluebird.reject(e);
Expand Down
4 changes: 2 additions & 2 deletions lib/dialects/mysql/index.js
Expand Up @@ -4,6 +4,7 @@ const inherits = require('inherits');
const { map, defer } = require('lodash');
const { promisify } = require('util');
const Client = require('../../client');
const { timeout } = require('../../util/timeout');
const Bluebird = require('bluebird');

const Transaction = require('./transaction');
Expand Down Expand Up @@ -177,8 +178,7 @@ Object.assign(Client_MySQL.prototype, {
// Purposely not putting timeout on `KILL QUERY` execution because erroring
// early there would release the `connectionToKill` back to the pool with
// a `KILL QUERY` command yet to finish.
return acquiringConn
.timeout(100)
return timeout(acquiringConn, 100)
.then((conn) =>
this.query(conn, {
method: 'raw',
Expand Down
21 changes: 11 additions & 10 deletions lib/dialects/oracledb/transaction.js
Expand Up @@ -2,6 +2,7 @@ const { isUndefined } = require('lodash');

const Bluebird = require('bluebird');
const Transaction = require('../../transaction');
const { timeout, KnexTimeoutError } = require('../../util/timeout');
const debugTx = require('debug')('knex:tx');

module.exports = class Oracle_Transaction extends Transaction {
Expand All @@ -23,24 +24,24 @@ module.exports = class Oracle_Transaction extends Transaction {
}

rollback(conn, err) {
const self = this;
this._completed = true;
debugTx('%s: rolling back', this.txid);
return conn
.rollbackAsync()
.timeout(5000)
.catch(Bluebird.TimeoutError, function(e) {
self._rejecter(e);
return timeout(conn.rollbackAsync(), 5000)
.catch((e) => {
if (!(e instanceof KnexTimeoutError)) {
return Promise.reject(e);
}
this._rejecter(e);
})
.then(function() {
.then(() => {
if (isUndefined(err)) {
if (self.doNotRejectOnRollback) {
self._resolver();
if (this.doNotRejectOnRollback) {
this._resolver();
return;
}
err = new Error(`Transaction rejected with non-error: ${err}`);
}
self._rejecter(err);
this._rejecter(err);
});
}

Expand Down
4 changes: 4 additions & 0 deletions lib/knex.js
Expand Up @@ -5,6 +5,7 @@ const QueryInterface = require('./query/methods');

const makeKnex = require('./util/make-knex');
const parseConnection = require('./util/parse-connection');
const { KnexTimeoutError } = require('./util/timeout');
const fakeClient = require('./util/fake-client');
const { SUPPORTED_CLIENTS } = require('./constants');
const { resolveClientNameWithAliases } = require('./helpers');
Expand Down Expand Up @@ -58,6 +59,9 @@ function Knex(config) {

// Expose Client on the main Knex namespace.
Knex.Client = Client;

Knex.KnexTimeoutError = KnexTimeoutError;

Knex.QueryBuilder = {
extend: function(methodName, fn) {
QueryBuilder.extend(methodName, fn);
Expand Down
14 changes: 11 additions & 3 deletions lib/runner.js
@@ -1,4 +1,6 @@
const Bluebird = require('bluebird');
const { KnexTimeoutError } = require('./util/timeout');
const { timeout } = require('./util/timeout');

let PassThrough;

Expand Down Expand Up @@ -133,7 +135,7 @@ Object.assign(Runner.prototype, {
let queryPromise = this.client.query(this.connection, obj);

if (obj.timeout) {
queryPromise = queryPromise.timeout(obj.timeout);
queryPromise = timeout(queryPromise, obj.timeout);
}

// Await the return value of client.processResponse; in the case of sqlite3's
Expand Down Expand Up @@ -164,7 +166,10 @@ Object.assign(Runner.prototype, {

return postProcessedResponse;
})
.catch(Bluebird.TimeoutError, (error) => {
.catch((error) => {
if (!(error instanceof KnexTimeoutError)) {
return Promise.reject(error);
}
const { timeout, sql, bindings } = obj;

let cancelQuery;
Expand Down Expand Up @@ -241,7 +246,10 @@ Object.assign(Runner.prototype, {
}
return this.client
.acquireConnection()
.catch(Bluebird.TimeoutError, (error) => {
.catch((error) => {
if (!(error instanceof KnexTimeoutError)) {
return Promise.reject(error);
}
if (this.builder) {
error.sql = this.builder.sql;
error.bindings = this.builder.bindings;
Expand Down
26 changes: 17 additions & 9 deletions lib/transaction.js
Expand Up @@ -5,6 +5,7 @@ const { EventEmitter } = require('events');
const Debug = require('debug');

const makeKnex = require('./util/make-knex');
const { timeout, KnexTimeoutError } = require('./util/timeout');

const debug = Debug('knex:tx');

Expand Down Expand Up @@ -150,19 +151,26 @@ class Transaction extends EventEmitter {
}

rollback(conn, error) {
return this.query(conn, 'ROLLBACK', 2, error)
.timeout(5000)
.catch(Bluebird.TimeoutError, () => {
return timeout(this.query(conn, 'ROLLBACK', 2, error), 5000).catch(
(err) => {
if (!(err instanceof KnexTimeoutError)) {
return Promise.reject(err);
}
this._rejecter(error);
});
}
);
}

rollbackTo(conn, error) {
return this.query(conn, `ROLLBACK TO SAVEPOINT ${this.txid}`, 2, error)
.timeout(5000)
.catch(Bluebird.TimeoutError, () => {
this._rejecter(error);
});
return timeout(
this.query(conn, `ROLLBACK TO SAVEPOINT ${this.txid}`, 2, error),
5000
).catch((err) => {
if (!(err instanceof KnexTimeoutError)) {
return Promise.reject(err);
}
this._rejecter(error);
});
}

query(conn, sql, status, value) {
Expand Down
12 changes: 12 additions & 0 deletions lib/util/timeout.js
@@ -0,0 +1,12 @@
const pTimeout = require('p-timeout');

class KnexTimeoutError extends pTimeout.TimeoutError {
constructor(message) {
super(message);
this.name = 'KnexTimeoutError';
}
}

module.exports.KnexTimeoutError = KnexTimeoutError;
module.exports.timeout = (promise, ms) =>
pTimeout(promise, ms, new KnexTimeoutError('operation timed out'));
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -39,6 +39,7 @@
"liftoff": "3.1.0",
"lodash": "^4.17.15",
"mkdirp": "^0.5.1",
"p-timeout": "^3.2.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering how small p-yimeout is, maybe it would make sense to inline implementation and use native finally instead of p-finally?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no strong opinion on that, though, sindresorhus libs are legit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"pg-connection-string": "2.1.0",
"tarn": "^2.0.0",
"tildify": "2.0.0",
Expand Down
8 changes: 4 additions & 4 deletions test/integration/builder/additional.js
Expand Up @@ -739,7 +739,7 @@ module.exports = function(knex) {
.catch(function(error) {
expect(_.pick(error, 'timeout', 'name', 'message')).to.deep.equal({
timeout: 200,
name: 'TimeoutError',
name: 'KnexTimeoutError',
message:
'Defined query timeout of 200ms exceeded when running query.',
});
Expand Down Expand Up @@ -826,7 +826,7 @@ module.exports = function(knex) {
.catch(function(error) {
expect(_.pick(error, 'timeout', 'name', 'message')).to.deep.equal({
timeout: 200,
name: 'TimeoutError',
name: 'KnexTimeoutError',
message:
'Defined query timeout of 200ms exceeded when running query.',
});
Expand Down Expand Up @@ -911,7 +911,7 @@ module.exports = function(knex) {
.catch(function(error) {
expect(_.pick(error, 'timeout', 'name', 'message')).to.deep.equal({
timeout: 1,
name: 'TimeoutError',
name: 'KnexTimeoutError',
message:
'After query timeout of 1ms exceeded, cancelling of query failed.',
});
Expand Down Expand Up @@ -980,7 +980,7 @@ module.exports = function(knex) {
.catch(function(error) {
expect(_.pick(error, 'timeout', 'name', 'message')).to.deep.equal({
timeout: secondQueryTimeout,
name: 'TimeoutError',
name: 'KnexTimeoutError',
message: `Defined query timeout of ${secondQueryTimeout}ms exceeded when running query.`,
});
})
Expand Down
3 changes: 2 additions & 1 deletion test/integration/builder/transaction.js
Expand Up @@ -6,6 +6,7 @@ const Bluebird = require('bluebird');
const Knex = require('../../../knex');
const _ = require('lodash');
const sinon = require('sinon');
const { KnexTimeoutError } = require('../../../lib/util/timeout');

module.exports = function(knex) {
// Certain dialects do not have proper insert with returning, so if this is true
Expand Down Expand Up @@ -505,7 +506,7 @@ module.exports = function(knex) {
.then(function() {
throw new Error('should not get here');
})
.catch(Bluebird.TimeoutError, function(error) {});
.catch(KnexTimeoutError, function(error) {});
});

/**
Expand Down
8 changes: 4 additions & 4 deletions test/tape/invalid-db-setup.js
Expand Up @@ -3,7 +3,7 @@
const tape = require('tape');
const _ = require('lodash');
const makeKnex = require('../../knex');
const Bluebird = require('bluebird');
const { KnexTimeoutError } = require('../../lib/util/timeout');

module.exports = (knexfile) => {
Object.keys(knexfile).forEach((key) => {
Expand All @@ -29,7 +29,7 @@ module.exports = (knexfile) => {
.then((res) => {
t.fail(`Query should have failed, got: ${JSON.stringify(res)}`);
})
.catch(Bluebird.TimeoutError, (e) => {
.catch(KnexTimeoutError, (e) => {
t.fail(`Query should have failed with non timeout error`);
})
.catch((e) => {
Expand All @@ -54,7 +54,7 @@ module.exports = (knexfile) => {
`Stream query should have failed, got: ${JSON.stringify(res)}`
);
})
.catch(Bluebird.TimeoutError, (e) => {
.catch(KnexTimeoutError, (e) => {
t.fail(`Stream query should have failed with non timeout error`);
})
.catch((e) => {
Expand Down Expand Up @@ -104,7 +104,7 @@ module.exports = (knexfile) => {
.then(() => {
t.fail('query should have stalled');
})
.catch(Bluebird.TimeoutError, (e) => {
.catch(KnexTimeoutError, (e) => {
t.pass('Got acquireTimeout error');
})
.catch((e) => {
Expand Down
2 changes: 2 additions & 0 deletions types/index.d.ts
Expand Up @@ -1990,6 +1990,8 @@ declare namespace Knex {
) => QueryBuilder<TRecord, TResult>
): void;
}

export class KnexTimeoutError extends Error {}
}

export = Knex;