Skip to content

Implement a PoolError with dedicated codes #2498

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implement a PoolError with dedicated codes #2498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98bbffb
Implement a `PoolError` with dedicated codes
rmedaer Mar 18, 2021
8af6885
Fix linting
rmedaer Mar 18, 2021
72dfbe7
Fix english grammar
rmedaer Jul 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/pg-pool/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,20 @@ var bluebirdPool = new Pool({

__please note:__ in node `<=0.12.x` the pool will throw if you do not provide a promise constructor in one of the two ways mentioned above. In node `>=4.0.0` the pool will use the native promise implementation by default; however, the two methods above still allow you to "bring your own."

## error management

Errors throwed by the pool are instances of `PoolError`. Each `PoolError` has a message (short description of the exception) and a structured code composed of 5 characters string starting with letter `Z`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • How about meaningful text like the error codes in the Node standard library?
  • I’m not sure if most of these deserve their own code. Nobody should be handling library misuse like multiple release, multiple end, use after end, or type errors programmatically. “Timeout exceeded when trying to connect” makes sense, though.
  • throwed → thrown

Copy link
Author

Choose a reason for hiding this comment

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

How about meaningful text like the error codes in the Node standard library?

Current implementation mimic the NoticeOrError interface from pg-protocol, cf this message

Nobody should be handling library misuse like multiple release, multiple end, use after end, or type errors programmatically.

That's why it raises exceptions errors.

“Timeout exceeded when trying to connect” makes sense, though.

That's actually the only one I want to catch for now. For instance in a HTTP API, my goal is to return (a more) meaningful message to the client (in this case a 503 which means that the service is unavailable for now).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think mimicking the format of PostgreSQL error codes (with values that don’t come from PostgreSQL) is useful. https://nodejs.org/api/errors.html#nodejs-error-codes

The following table list the existing `PoolError`s and their code:

| Code | Message |
| ----- | ------------------------------------------------------------------------ |
| Z0001 | Cannot use a pool after calling end on the pool |
| Z0002 | Passing a function as the first parameter to pool.query is not supported |
| Z0003 | Release called on client which has already been released to the pool |
| Z0004 | Called end on pool more than once |
| Z0005 | Timeout exceeded when trying to connect |
| Z0009 | Unexpected condition |

## maxUses and read-replica autoscaling (e.g. AWS Aurora)

The maxUses config option can help an application instance rebalance load against a replica set that has been auto-scaled after the connection pool is already full of healthy connections.
Expand Down
24 changes: 18 additions & 6 deletions packages/pg-pool/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class PendingItem {
}

function throwOnDoubleRelease() {
throw new Error('Release called on client which has already been released to the pool.')
throw new PoolError('Release called on client which has already been released to the pool.', 'Z0003')
}

function promisify(Promise, callback) {
Expand Down Expand Up @@ -58,6 +58,14 @@ function makeIdleListener(pool, client) {
}
}

class PoolError extends Error {
// A PoolError is an error thrown during a Pool operation. Each type of error contains a specific "code", documented in pg-pool’s README.
constructor(message, code) {
super(message)
this.code = code
}
}

class Pool extends EventEmitter {
constructor(options, Client) {
super()
Expand Down Expand Up @@ -143,7 +151,7 @@ class Pool extends EventEmitter {
if (!this._isFull()) {
return this.newClient(pendingItem)
}
throw new Error('unexpected condition')
throw new PoolError('unexpected condition', 'Z0009')
}

_remove(client) {
Expand All @@ -160,7 +168,7 @@ class Pool extends EventEmitter {

connect(cb) {
if (this.ending) {
const err = new Error('Cannot use a pool after calling end on the pool')
const err = new PoolError('Cannot use a pool after calling end on the pool', 'Z0001')
return cb ? cb(err) : this.Promise.reject(err)
}

Expand Down Expand Up @@ -192,7 +200,7 @@ class Pool extends EventEmitter {
// we're going to call it with a timeout error
removeWhere(this._pendingQueue, (i) => i.callback === queueCallback)
pendingItem.timedOut = true
response.callback(new Error('timeout exceeded when trying to connect'))
response.callback(new PoolError('timeout exceeded when trying to connect', 'Z0005'))
}, this.options.connectionTimeoutMillis)

this._pendingQueue.push(pendingItem)
Expand Down Expand Up @@ -334,7 +342,9 @@ class Pool extends EventEmitter {
if (typeof text === 'function') {
const response = promisify(this.Promise, text)
setImmediate(function () {
return response.callback(new Error('Passing a function as the first parameter to pool.query is not supported'))
return response.callback(
new PoolError('Passing a function as the first parameter to pool.query is not supported', 'Z0002')
)
})
return response.result
}
Expand Down Expand Up @@ -385,7 +395,7 @@ class Pool extends EventEmitter {
end(cb) {
this.log('ending')
if (this.ending) {
const err = new Error('Called end on pool more than once')
const err = new PoolError('Called end on pool more than once', 'Z0004')
return cb ? cb(err) : this.Promise.reject(err)
}
this.ending = true
Expand All @@ -408,3 +418,5 @@ class Pool extends EventEmitter {
}
}
module.exports = Pool
module.exports.Pool = Pool
module.exports.PoolError = PoolError
Comment on lines +421 to +422
Copy link
Collaborator

Choose a reason for hiding this comment

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

Putting Pool on itself doesn’t feel right pre-ESM, and it’s not easy to use via pg. Would this even need to be exported with non-colliding error codes like ERR_PG_POOL_…?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean: replacing PoolError by constants (for instance ERR_PG_POOL_CONNECT_TIMEOUT, ERR_PG_POOL_UNEXPECTED_CONDITION, ...) ?

My goal was to implement the same kind of structure than NoticeOrError interface from pg-protocol. What would be the PROs/CONs ?

Copy link
Contributor

@matthieusieben matthieusieben Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I believe what @charmander is trying to say, is that if, instead of error codes like Z0001, you used NodeJS like error codes, you would not need to do instanceof PoolError and you could simply perform a check like:

catch (err) {
  if (err.code === 'ERR_PG_POOL_UNEXPECTED_STATE') throw err // something's wrong
  else if (err.code === 'ERR_PG_POOL_CONNECTION_TIMEOUT') {} // handle overloaded database
  else {} // Probably not a pg-pool error
}

In this case, you would not even need to export PoolError at all.

Note that because of the way node_modules are structured, checking for instanceof might yield false negative in case pg-pool ends up twice in you node_modules hierarchy (blame this on NodeJS's ecosystem).

This also explain his other comment explaining that you don't need to explicit every possible error case. ERR_PG_POOL_UNEXPECTED_STATE + ERR_PG_POOL_CONNECTION_TIMEOUT (please find better names 😅) are probably enough.

I believe that his first comment is that this feels right in ESM:

export default Pool
export { Pool, PoolError }

but the following is weird in common-js (pre-ESM)

module.exports = Pool
module.exports.Pool = Pool

because it is the same as doing Pool.Pool = Pool which feels odd.