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

Prevent double release with callback #115

Closed

Conversation

johanneswuerbach
Copy link
Contributor

When using the done callback instead of client.release, double releasing a client was possible causing clients to be re-added multiple times.

Ensure that the release method itself can't be called twice instead of patching on the client.

I also did a small refactoring to ensure that new client and re-used client acquiring goes through the same code path and moved release into the client instead of using bind. Happy to remove this, but I found the logic a bit more readable afterwards.

@johanneswuerbach johanneswuerbach force-pushed the fix-double-release branch 3 times, most recently from 95ed717 to 40482a0 Compare January 14, 2019 15:56
When using the callback instead of client.release, double releasing
a client was possible causing clients to be re-added multiple times.
@brianc
Copy link
Owner

brianc commented Feb 11, 2019

Code changes look good. Thanks for diving in here. Question is....do you think this is a minor or major version bump? I feel more so like it's minor as the old behavior was undefined or would introduce silent bugs...but it does change the behavior. What you think?

@brianc brianc requested a review from charmander February 11, 2019 19:34
@johanneswuerbach
Copy link
Contributor Author

I would see this as a (critical) bug fix as releasing the same client multiple times will result in multiple idleQueue entries all referencing the same client, which will cause various other problems.

@brianc
Copy link
Owner

brianc commented Feb 11, 2019

patch version then...yeah?

@johanneswuerbach
Copy link
Contributor Author

I would say so, the same behaviour was already happening when calling client.release() twice.

@olgkpln
Copy link

olgkpln commented Feb 19, 2019

@brianc any idea when you plan to merge this? I would like to use it on my project.

@johanneswuerbach
Copy link
Contributor Author

@brianc friendly ping :-)

@johanneswuerbach
Copy link
Contributor Author

@brianc anything else required to move this forward?

@johanneswuerbach
Copy link
Contributor Author

Fixed in #123

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.

3 participants