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

Handle bad message ordering - make it catchable. Fixes 3174 #3289

Merged
merged 11 commits into from
Sep 17, 2024

Conversation

brianc
Copy link
Owner

@brianc brianc commented Aug 1, 2024

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.

Copy link

cloudflare-workers-and-pages bot commented Aug 1, 2024

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

_handleParseComplete(msg) {
_handleParseComplete() {
if (this.activeQuery == null) {
this.emit('error', new Error('Received parseComplete when not in parsing state'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this use _handleErrorEvent, since the connection is in an unpredictable state afterwards?

Copy link

Choose a reason for hiding this comment

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

I can see how using _handleErrorEvent might be a good idea here too, I think what @brianc communicated to me about this PR is that he thinks the issue is always related to an unexpected message arriving at a client which is in the idle state and so if that is the case it might not be required to do the full _handleErrorEvent but I can see how maybe doing full _handleErrorEvent is more robust.

@rhodey
Copy link

rhodey commented Aug 2, 2024

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!

@brianc brianc merged commit f73b22f into master Sep 17, 2024
11 checks passed
@brianc brianc deleted the bmc/fix-for-3174 branch September 17, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants