Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

condchan: simplify ChanPool #1

Closed
wants to merge 2 commits into from
Closed

Conversation

bcmills
Copy link

@bcmills bcmills commented Sep 6, 2017

I do not guarantee that this is free from bugs, but the general idea should be sound.

@bcmills
Copy link
Author

bcmills commented Sep 6, 2017

I should note that this implementation slightly weakens the “Close should interrupt Get” property: it does reliably interrupt Get eventually, but pending Get calls will block until the number of active connections drops below the limit.

That could be fixed (at a small cost in LoC) by changing the closed field from a boolean to a channel.

@bcmills bcmills force-pushed the master branch 4 times, most recently from 6a39ae9 to 8594b47 Compare September 6, 2017 17:31
@cespare
Copy link
Owner

cespare commented Sep 6, 2017

That could be fixed (at a small cost in LoC) by changing the closed field from a boolean to a channel.

Can you describe that in a bit more detail? I don't see how it would work, since you can't select on a channel and a semaphore acquisition at the same time. (Or do you also mean spawning an extra goroutine per Get?)

@cespare
Copy link
Owner

cespare commented Sep 6, 2017

Thanks for this, btw!

@bcmills
Copy link
Author

bcmills commented Sep 6, 2017

I don't see how it would work, since you can't select on a channel and a semaphore acquisition at the same time.

Hmm, good point. Will think a bit and get back to you.

The cheating way to do it would be to store a context.Context in the Pool struct and use that instead of context.TODO(): then you can just cancel it in Close, but you can't use a real Context.

@bcmills
Copy link
Author

bcmills commented Sep 6, 2017

The slightly-less-cheaty way to do it would be to add a Close method to *semaphore.Weighted that would terminate pending calls to Acquire (and cause future calls to Acquire and TryAcquire to fail).

@cespare
Copy link
Owner

cespare commented Sep 6, 2017

Maybe we need semaphore.AcquireChan ;)

@bcmills
Copy link
Author

bcmills commented Sep 6, 2017

I was wrong; it's actually shorter with closed as a channel. :)

@bcmills bcmills changed the title condchan: simplify ChanPool using a semaphore condchan: simplify ChanPool Sep 6, 2017
@bcmills
Copy link
Author

bcmills commented Sep 6, 2017

Maybe we need semaphore.AcquireChan ;)

That would be pretty dangerous: if the caller abandons the Acquire they need to give up their slot in the queue. (If we returned a channel they could wait on, we would also have to return an “abandon” callback, and it would be really easy to accidentally omit or duplicate the abandon() call.)

@cespare
Copy link
Owner

cespare commented Sep 6, 2017

Hey, I like this one a lot more. Nice.

@cespare
Copy link
Owner

cespare commented Sep 7, 2021

Point made. We have some internal connection pools which use a pattern like this these days :)

@cespare cespare closed this Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants