Always check if activeQuery is null before using it

[rhodey]

Always check if activeQuery is null before using it

My company is using pg version 8.13.0 in production on a few hundred AWS ECS containers

I have a PR against version 8.13.0 available here (rhodey#2) for show and this patch has been tested in production for about 1 month now

The issue that this PR addresses was first discussed here (#3174)

In response to that @brianc opened and merged this PR (https://github.com/brianc/node-postgres/pull/3289/changes)

3289 patched _handleCommandComplete and _handleParseComplete and we were happy for this and saw app crashes mostly go away but not disappear entirely but we moved onto other work

This month we saw some app crashes again due to activeQuery being null and these errors cause crash not just an error event emitted

This PR patches the remaining _handle* functions such that now in pg/lib/client.js every access of activeQuery is tested for null before use

Every use of activeQuery is now guarded against null / undefined / falsey

[rhodey]

It cannot be said if node-postgres is receiving these protocol messages at unexpected times because of something node-postgres is doing wrong or if it is because of something PSQL is doing wrong or maybe pgbouncer is doing wrong

In any case the PSQL server or pgbouncer or some other middleware may mess up some times and when they do I think it is much better that node-postgres emits an error event which closes the pooled conn and can be logged versus a null access which crashes the nodejs process without the app having any way to avoid a crash / exit

[charmander]

Looks good; tests?

[brianc]

Thanks @rhodey! Excited to get this in! In this case I think I have some tests set up already to fake a backend, connect to it, and send messages in the wrong order. Could probably extend that. Or just unit test w/ manual message emitting or something. Since the native bindings I'm 99% sure already ignore out of order messages its probably fine to not do an end-to-end style test..though w/o any tests at all there's always a chance this behavior could regress in the future, so tests are 💯

[rhodey]

Hi team, thanks for checking in,

The PR 3289 which was merged awhile ago was done by @brianc and I remember there were some tests added to that and I found them here:

https://github.com/brianc/node-postgres/pull/3289/changes#diff-240e046ba1eb32d39e04db3ea57a62ff7830c92961e1e9e1da18f84d81315fe3

Unless I hear back otherwise I will plan to add tests to this PR in the style of the tests I link to above

I cant promise I will have tests pushed very very soon it is now on my list

[brianc]

@rhodey that sounds great. I'll try to get to it tomorrow if you don't have it done by then. I'll just push to your PR. (always takes me a min to remember how to do that)

[brianc]

@rhodey don't worry about it. I pushed tests! Thanks for finding that old PR. that made it extremely easy. ❤️ I'll push a new minor version tomorrow morning because this backwards compat API change. I don't really publish before bed because...I need to be around in case something lights on fire! Try as I might to keep backwards compat & perfectly follow semver...is hard!

[rhodey]

Great! Thanks @brianc :)

[brianc]

Thanks for the contribution!

Successfully published:
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]
 - [email protected]