Repository navigation
fix: log idle client errors instead of crashing - #235
TemoSulava wants to merge 3 commits into
Conversation
gurgunday
left a comment
There was a problem hiding this comment.
-
Every dropped connection logs two errors (index.js:95). When a backend dies (Postgres restarts, or pg_terminate_backend), pg's Client emits the original error and then a second Connection terminated unexpectedly. The new permanent listener logs both, while pg-pool deliberately ignores the follow-ups. With 20 pooled clients, one restart writes 40 pg client error lines at error level. The test only checks errorLogs[0], so it doesn't notice.
-
The test never covers the idle-client case from issue #168 (test/initialization.test.js:241). The new test kills a client that is still checked out. pg-pool removes its idle listener while a client is checked out, so the pool-level 'error' path never fires. You could delete pool.on('error', () => {}) at index.js:92, bringing back the #168 crash, and the suite would still pass. The PR description says the test kills an idle backend, but it doesn't. A better test would connect, release, terminate the now-idle backend, then assert the process survives and the error is logged.
|
@gurgunday thanks, both points were right. Fixed in ca7a549.
|
| // fires for idle and checked-out clients, so log only here | ||
| pool.on('connect', (client) => { | ||
| let errored = false | ||
| client.on('error', (err) => { |
There was a problem hiding this comment.
maybe once instead of errored?
| client.on('error', (err) => { | |
| client.once('error', (err) => { |
There was a problem hiding this comment.
Thanks for the suggestion, I tried it first since it reads cleaner. The problem is the checked-out case: pg-pool removes its idleListener while a client is checked out, so ours is the only 'error' listener on it. With once, it's gone after the first error, and the follow-up ECONNRESET / "Connection terminated unexpectedly" get emitted with no listener at all, which crashes the process. The checked-out test fails exactly that way with once (uncaught read ECONNRESET).
So the listener has to stay attached and just ignore the follow-ups. pg-pool solves the same thing for idle clients by swapping in a no-op listener after the first error. I can do it that way here too if you'd prefer it over the flag.
Fixes #168
If an idle client in the pool hits an error, for example when Postgres restarts or the
backend gets terminated, pg-pool emits 'error' on the pool. Nothing was listening
for it, so the whole Fastify process went down with an unhandled 'error' event.
The plugin now absorbs the pool-level re-emit and logs client errors with
fastify.log.error from a listener added on each client, which covers both idle
and checked-out clients. Only the first error per client is logged, because a
dropped connection follows up with errors like "Connection terminated
unexpectedly". The broken client is already removed by pg-pool, so the next
query just gets a fresh connection. Users who attach their own listener on
fastify.pg.pool still receive the event as before.
One behaviour change worth calling out: apps that relied on the crash to get
restarted by a supervisor will now keep running instead.
There are two tests. One releases a client and terminates its now-idle backend.
The other terminates a checked-out one. Both assert exactly one error log (57P01)
and that the pool still answers queries afterwards.