brianc / node-libpq

Simple, low level native bindings to PostgreSQL's libpq from node.js
111 stars 41 forks source link

Fix memory leak and rare segfaults #87

Closed frakti closed 2 years ago

frakti commented 2 years ago

At Voucherify some time ago, we started using pg-native and soon noticed a memory leak in our production. We've fixed it and have been successfully using the fix for the last few months.

The memory leak was caused by an old bug related to commented self->Unref(). Using node-inspector, I've noticed that each connection object, although closed, was bound to the main event loop and never freed - due to missing Unref().

But as the commit message related to previously commented Unref() suggested, some rare segfaults came out. Using valgrind and ASAN I've been able to more or less track them. There could be at least two sources of those crashes.

Mandatory fixes:

Regarding other changes, I'm not sure if those are necessary - in the end, segfault reproduction was extremely rare and took a lot of time - and I didn't manage to test fixes without them. But I can confirm they didn't introduce any harm:

BTW. While working on this, I've also noticed another memory leak, but you already fixed that one 3 days ago - these changes are unrelated to that one.

brianc commented 2 years ago

Woa this is amazing. Thank you! Austin tx is having a major weather event right now & my power is cutting in and out but I'll do a thorough review of this tomorrow & get a new version shipped ASAP!

frakti commented 2 years ago

No worries. If you need some additional details, please let me know.

To clarify, we have 15 clusters, each with a few postgres instances (AWS RDS). Each cluster runs ~50 application pods. We have been using this fix (+ exactly the same from #77) since January 2022 and didn't notice any other memory leak or segfault. Before the fix, we had a nasty cronjob triggering apps restarting once or twice a day to avoid OOMs. So this fix might affect many projects worldwide.

BTW. While working on this issue, I made one iteration by partially rewriting the lib from NAN to node-addon-api and it even worked, so if you will consider such migration in the future, I would be glad to help.

brianc commented 2 years ago

BTW. While working on this issue, I made one iteration by partially rewriting the lib from NAN to node-addon-api and it even worked, so if you will consider such migration in the future, I would be glad to help.

Yes, 100% open to any and all support on this library!! The only real issue is pg still supports node@8.x since so many people are still on it. I would be open to dropping support for node-libpq for that version though if it doesn't work with node-addon-api