search

brianc / node-pg-pool

A connection pool for node-postgres
MIT License
180 stars 64 forks source link

Prevent client reuse after release #51

Closed kjdelisle closed 7 years ago

kjdelisle commented 7 years ago

closes #50

This will add an intermediate client object (SafeClient) that wraps calls to .query and .release, with the goal of preventing reuse of objects that have been released back to the pool. Attempting to call .query after .release will throw an error.

kjdelisle commented 7 years ago

@brianc PTAL :)

brianc commented 7 years ago

I really appreciate you trying to address this issue! I think it's something we can fix, however, your approach might need some adjustment:

The SafeClient approach of making a new instance will unfortunately break a bunch of backwards compatibility with things - there are more methods & properties on the client than just query and release - I think a better approach would be to monkey patch over query and release on the client when it's first checked out of the pool & do the check there in the monkey-patched version of those methods. That way a pooled client has additional behavior, but still is an actual instance of a Client object.

We already monkey patch the release method on to the client, infact:

https://github.com/brianc/node-pg-pool/blob/master/index.js#L125

So that would be a good place to start looking to add additional functionality.

kjdelisle commented 7 years ago

I broke everything out into SafeClient to keep it tidy, but I hadn't thought about things relying on the object type to be "Client". 😞

Let me see what I can hack together; I'll push it to this branch when it's ready.

kjdelisle commented 7 years ago

I'm having a heck of a time trying to make this work by modifying the client instances directly. The main advantage of abstracting it out with the SafeClient object was that the lifecycle of those instances was a single run from connect to release.

By modifying the client instances themselves, we're now responsible for tracking locks of some sort on each of the instances to ensure that the current caller actually owns the resource in question, and it seems like generic-pool@2.x doesn't support any sort of locking mechanism.

I'm constantly seeing failures in the emits acquire every time a client is acquired test where the parallel calls to pool.connect and pool.query are receiving the same client instances, detecting the mismatched locks and erroring out.

@brianc Do you have any idea if generic-pool@3.x helps solve or mitigate this problem? I'm hesitant to force-push my experiments over top of this branch just yet, since it's a bit of a mess...

brianc commented 7 years ago

Unfortunately this is never going to merge at this point & a new tact needs to be taken.