Skip to content

Commit

Permalink
remove bad connections from the postgres pool after a failed query
Browse files Browse the repository at this point in the history
typeorm currently lets pg.pool assume that clients are always safe to
add back into the pool, even if it encountered an error while running
a query. this is /usually/ fine, because the vast majority of errors
that happen during a query do not cause the client's connection itself
to become inoperable

however, if, during a query, the client's connection is killed, the
query will fail and the client will be placed back into the pool even
though it's linked to a now broken connection, i.e. that client will
never work again, but because pg.Pool doesn't know that, it'll keep
handing it out. end result: lots of failed queries

this patches `Connection.prototype.query` so that it passes the error
back to the driver when it calls `queryRunner.release`. the driver can
(if applicable) then inspect the error and decide if it should result
in the client being removed from the pool or just released as normal
  • Loading branch information
clarkdave committed Nov 19, 2019
1 parent f342ecd commit 6bd52e0
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 6 deletions.
10 changes: 8 additions & 2 deletions src/connection/Connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -396,11 +396,17 @@ export class Connection {
const usedQueryRunner = queryRunner || this.createQueryRunner("master");

try {
return await usedQueryRunner.query(query, parameters); // await is needed here because we are using finally
const result = await usedQueryRunner.query(query, parameters);

} finally {
if (!queryRunner)
await usedQueryRunner.release();

return result;
} catch (err) {
if (!queryRunner)
await usedQueryRunner.release(err);

throw err;
}
}

Expand Down
18 changes: 15 additions & 3 deletions src/driver/postgres/PostgresQueryRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,23 @@ export class PostgresQueryRunner extends BaseQueryRunner implements QueryRunner
/**
* Releases used database connection.
* You cannot use query runner methods once its released.
*
* If `error` is truthy the released connection may be removed from the pool
* depending on the driver.
*/
release(): Promise<void> {
release(error?: any): Promise<void> {
this.isReleased = true;
if (this.releaseCallback)
this.releaseCallback();

if (this.releaseCallback) {
// severe errors may indicate an unrecoverable client, e.g. a client
// whose connection no longer exists (due to network issues, etc).
// the safest thing to do is instruct the pool to remove it, or it
// will stick around in the pool indefinitely, breaking every
// subsequent query that runs on it
const removeFromPool = Boolean(error && error.severity === "FATAL");

this.releaseCallback(removeFromPool);
}

const index = this.driver.connectedQueryRunners.indexOf(this);
if (index !== -1) this.driver.connectedQueryRunners.splice(index);
Expand Down
5 changes: 4 additions & 1 deletion src/query-runner/QueryRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,11 @@ export interface QueryRunner {
/**
* Releases used database connection.
* You cannot use query runner methods after connection is released.
*
* If `error` is truthy the released connection may be removed from the pool
* depending on the driver.
*/
release(): Promise<void>;
release(error?: any): Promise<void>;

/**
* Removes all tables from the currently connected database.
Expand Down

0 comments on commit 6bd52e0

Please sign in to comment.