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

Implement 'release' event and 'activeCount' property. #125

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PrimeJunta
Copy link

Implements feature request from #124 .

@PrimeJunta
Copy link
Author

Hi @brianc, in case you think the suggestion from the issue I reported is worth addressing, here's an implementation. Thanks for the work you're doing and don't worry, I won't be mad if you decline it.

@brianc
Copy link
Owner

brianc commented May 20, 2019

Hey thanks for the PR! My suggestion would be if you only want to track the active count that we just use an increment/decrement on a this._activeCount = 0 field instead of keeping an array and adding/removing items to it every time we add/remove an active client. What do you think?

@PrimeJunta
Copy link
Author

I have no objection. I implemented it that way to maintain consistency with the way you track idle and total clients, and also because I thought there might be a scenario where you'd want to know exactly which clients are checked out -- e.g. if you're debugging a mysterious client leak and track it by flagging each client with something that identifies the process that checked it out. I might be overthinking it however; a simple counter would certainly suffice for the actual scenarios I had in mind.

Would you like me to update the PR or shall you do it yourself?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@PrimeJunta
Copy link
Author

I've addressed the change requests @charmander made. If you'd like me to switch from the array to a simple int counter, let me know and I'll make that change also.

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