node-postgres icon indicating copy to clipboard operation
node-postgres copied to clipboard

Handle bad message ordering - make it catchable. Fixes 3174

Open brianc opened this issue 1 year ago • 2 comments

The biggest issue was when messages came in out of order sometimes the client would throw a null ref error in an uncatchable place in the code & crash the node process. As far as I know, this PR should totally fix that issue, changing the unhandled null ref error in the background into a catchable error emitted by the client (or pool if you use pool.query).

Wrote a fake postgres backend to send messages out of order to test these weird edge cases outlined in #3174 where proxies or pg-bouncer or the like is getting messages mixed up. Can extend the test suite as needed and improve other edge cases. The native driver only emits a notice event when it receives an out of order message.

brianc avatar Aug 01 '24 19:08 brianc

Deploying node-postgres with  Cloudflare Pages  Cloudflare Pages

Latest commit: 93b9fad
Status: ✅  Deploy successful!
Preview URL: https://de73bc38.node-postgres.pages.dev
Branch Preview URL: https://bmc-fix-for-3174.node-postgres.pages.dev

View logs

Hey @brianc, here to add my two cents, first wanna say thanks for your attention to the issue report and thanks to @charmander as well for weighing in! I can see that this PR as is addresses _handleParseComplete() being called on a idle client and that makes good sense to me, however if you review this comment on 3174 you will see that we also have a history of seeing _handleCommandComplete being called on (most likely) an idle client and crashing the lib as well.

I think it makes sense that whatever is decided to be done for _handleParseComplete can be done within _handleCommandComplete as well, right? And on the subject of there being a second case is there a third or fourth kind of protocol message which could show up unexpected on an idle client and maybe if there is then there could be a place further up in the call stack to check if the client is idle and emit errors from there?

Thanks!

rhodey avatar Aug 02 '24 14:08 rhodey