Skip to content
This repository has been archived by the owner on Dec 30, 2019. It is now read-only.

Prevent client reuse after release #51

Closed
wants to merge 1 commit into from

Conversation

kjdelisle
Copy link

@kjdelisle kjdelisle commented Apr 17, 2017

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.

test/index.js Outdated
}, 10)
})
pool.on('error', function (err) {
// Allow all of the event handles to run before checking
Copy link
Author

Choose a reason for hiding this comment

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

Errant comment, cleaning this up on next push

@kjdelisle
Copy link
Author

@brianc PTAL :)

@brianc
Copy link
Owner

brianc commented Apr 17, 2017

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
Copy link
Author

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
Copy link
Author

kjdelisle commented Apr 18, 2017

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 brianc mentioned this pull request Jul 9, 2017
Merged
@brianc
Copy link
Owner

brianc commented Aug 10, 2017

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

@brianc brianc closed this Aug 10, 2017
@kjdelisle kjdelisle deleted the prevent-reuse-on-release branch September 26, 2017 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent reuse of released client instances
2 participants