Code review - Simple connection pool for net.Conn

[Fatih Arslan]

Hi, 

I wrote a simple connection pool for net.Conn.  First I've used a map with locks

to retrieve and put connections. But later I found this way much more simpler 

because there is no more locks involved.

I was wondering if this is a good approach to the problem? What do you think?

If there is anything I should improve or change I would be happy.

The code is here: https://github.com/fatih/pool

Thanks in advance

[Martin Angers]

Hi Fatih,

Actually, there are still locks involved, as channels use futexes internally. Also, if the Pool is meant to be used concurrently, this looks racy, I suggest you run your tests with the -race option.

Martin

[Fatih Arslan]

Hi Martin,

What exactly is looking racy? I just run it with -race as you said,
and it seems the race detector couldn't find any problem:

PASS
ok github.com/fatih/pool 1.326s

> --
> You received this message because you are subscribed to the Google Groups "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.



--
Fatih Arslan

[Martin Angers]

The non-chan fields of the struct are not protected, and len and cap are not thread-safe.

[Jesse McNelis]

On Fri, Nov 29, 2013 at 12:05 PM, Martin Angers <martin....@gmail.com> wrote:

The non-chan fields of the struct are not protected, and len and cap are not thread-safe.

That's fine, They are never assigned to concurrently.

cap() has to be threadsafe since the cap of a channel can't change. len() is going to be inaccurate, and it doesn't look like it's specified whether it's threadsafe.

But it doesn't seem reasonable for len() to not be threadsafe since the len value would already need to be protected for send/recieve operations.

--
=====================
http://jessta.id.au

[Matt Harden]

Destroy doesn't close all connections as it is documented to do.

I would drop isDestroyed, and rely on the closed-ness of the channel to tell you the pool is destroyed. As it is now, there is a race between Get/Put and Destroy. Your tests don't do Get/Put concurrently with Destroy, so they don't reveal the race.

[Martin Angers]

You're right, cap doesn't matter. But if Destory is called concurrently with Get, there is a race. That's why I asked if Pool was meant to be used concurrently (which I suppose it is). Matt put it more clearly than I did.

[Jesse McNelis]

On Fri, Nov 29, 2013 at 2:01 PM, Martin Angers <martin....@gmail.com> wrote:

You're right, cap doesn't matter. But if Destory is called concurrently with Get, there is a race. That's why I asked if Pool was meant to be used concurrently (which I suppose it is). Matt put it more clearly than I did.

Certainly, I didn't actually see the Destroy method.

With Destroy modifying the value of the channel value, cap() and len() wouldn't be threadsafe because the value could change under them.

 

--
=====================
http://jessta.id.au

[Martin Angers]

cap() is still fine though, it can never change (for channels) after `make`.

[Sugu Sougoumarane]

Take a look at https://github.com/youtube/vitess/blob/master/go/pools/resource_pool.go.

It's surprisingly non-trivial to make such few lines of code work correctly while keeping it readable.

In your implementation, here are the problems:

- If New fails, it leaves all those connections open.

- Get can suffer from thundering herd, and create more connections than the pool capacity.

- Access to isDestroyed needs to be atomic or protected by a mutex.

- Destroy does not close the connections.

- Get and Put have a data race with Destroy w.r.t. p.conn that can cause panics.

As for len & cap, I hear they are atomically set and read. So, it should be safe to use them as is.

[Sugu Sougoumarane]

Forgot to give credit where it's due. The resource_pool code was developed with a lot of help from Mr. Datarace himself, Dmitry.

[Martin Angers]

Any source/confirmation on the atomic len and cap? I couldn't find the source code for cap() (except for reflect.chancap(), which does not appear to be atomic), and my understanding is that it is translated to a simple lookup to the field in the C struct of the type (chan in this case). I think it's Rob Pike who mentioned somewhere in the mailing-list that a len() call was very cheap and fast, a simple lookup, so that would seem to imply a non-atomic operation (which are more costly).

[Sugu Sougoumarane]

Actually, even if len & cap were atomic, they still have a race with Destroy on the p.conns access, just like Get & Put do. So, that's still a problem.

I was only talking about len racing with a channel send/receive.

I think cap need not be atomically fetched because it never changes.

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/E467Ec0-bYQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.

[Fatih Arslan]

That's difficult because there is no way to learn the difference
between an empty and closed channel for buffered channels.

We spoke that yesterday in #go-nuts too, here is an example(via
Athiwat) that shows that :

http://play.golang.org/p/tylaBWRKLh

That was the reason I introduced the "isDestroyed" flag.

--
Fatih Arslan

[Dmitry Vyukov]

On Fri, Nov 29, 2013 at 8:27 AM, Martin Angers
<martin....@gmail.com> wrote:
> Any source/confirmation on the atomic len and cap? I couldn't find the
> source code for cap() (except for reflect.chancap(), which does not appear
> to be atomic), and my understanding is that it is translated to a simple
> lookup to the field in the C struct of the type (chan in this case). I think
> it's Rob Pike who mentioned somewhere in the mailing-list that a len() call
> was very cheap and fast, a simple lookup, so that would seem to imply a
> non-atomic operation (which are more costly).

"A single channel may be used for send and receive operations and
calls to the built-in functions cap and len by any number of
goroutines without further synchronization."
http://tip.golang.org/ref/spec#Send_statements

[Nico]

On 29/11/13 10:52, Fatih Arslan wrote:
> That's difficult because there is no way to learn the difference
> between an empty and closed channel for buffered channels.

I may be missing what you meant, but here's my question:

doesn't the variable ok tell you whether the channel is closed or not?

[Fatih Arslan]

The variable also tells whether it is empty or not. Therefore, as seen
in the example, you don't have ability to know the difference between
empty or closed for buffered channels..

[Fatih Arslan]

Hi Sugu, thanks for the recommendations

I've done this now (changes are commited to: https://github.com/fatih/pool):

* New now creates a buffered slice before it puts it to the channel,
and if it fails it will close all connections
* Destroys closes all connections that are currently in the pool
* isDestroyed is now protected via a lock. The lock is also used for Destroy().
* Added a test with concurrent access of Get() and Put(), but the test
fails now with "go test -race". I have to look why it fails.

[Martin Angers]

Nice, thanks! Looks like the doc is new.

[Nico]

On 29/11/13 11:55, Fatih Arslan wrote:
> The variable also tells whether it is empty or not. Therefore, as seen
> in the example, you don't have ability to know the difference between
> empty or closed for buffered channels..

I see. The key is that it isn't possible to distinguish a *non-empty*
channel that has been closed from a *non-empty* channel that hasn't.

I haven't thought this through, but why not replace the channel by
pointer to a channel and use a nil pointer to indicate a destroyed pool?

[Fatih Arslan]

A pointer to a channel is a good idea. I removed the isDestroyed and
mutex that was protecting it. But do I need a mutex around the test of
nilness?:

if p.conns == nil {
return errors.New("pool is destroyed")

--
Fatih Arslan

[Nico]

On 29/11/13 17:04, Fatih Arslan wrote:
> A pointer to a channel is a good idea. I removed the isDestroyed and
> mutex that was protecting it. But do I need a mutex around the test of
> nilness?:
>
> if p.conns == nil {
> return errors.New("pool is destroyed")
> }

Wouldn't be enough to make a copy of the pointer?

c := p.conns
if c == nil {

return errors.New("pool is destroyed")
}

// At this point you can use c safely
...

[Fatih Arslan]

What's the difference between making a copy of pointer and testing the
pointer directly?

--
Fatih Arslan

[Nico]

On 29/11/13 17:26, Fatih Arslan wrote:
> What's the difference between making a copy of pointer and testing the
> pointer directly?

if p.conns == nil {
return errors.New("pool is destroyed")
}

// p.conns can be nil after the test
// because other goroutines can set it to nil
...

[Matt Harden]

If reading / writing a pointer concurrently were safe, there wouldn't functions in sync.atomic to manipulate pointers. So yes, the reading and writing of the pointer would have to be protected by a mutex or handled atomically.

Also, currently an attempt to Put a connection back into a Destroyed pool will fail, leaving the net.Conn still open. Nothing stops this from happening even when the pool is used correctly (each Get is associated with exactly one subsequent Put of the same net.Conn).

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.

To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.

[Sugu Sougoumarane]

I've sent you a pull request (it may not compile). It shows one way to simplify the code while also taking care of race conditions.

Feel free to adopt it the way you like :).

https://github.com/sougou/pool/commit/afdf1db00dc18d2b69f45a59e20e726e749c78ae

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/E467Ec0-bYQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.

[Fatih Arslan]

With the help of Sugu some of the race problems are fixed now. I've
also add travis ci and add some examples.
You can see the latest state here: https://github.com/fatih/pool

Thanks for all contributions :)

>>> email to golang-nuts...@googlegroups.com.
>>>
>>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>>
>> --
>> You received this message because you are subscribed to a topic in the
>> Google Groups "golang-nuts" group.
>> To unsubscribe from this topic, visit
>> https://groups.google.com/d/topic/golang-nuts/E467Ec0-bYQ/unsubscribe.
>> To unsubscribe from this group and all its topics, send an email to
>> golang-nuts...@googlegroups.com.
>>
>> For more options, visit https://groups.google.com/groups/opt_out.
>
>

--
Fatih Arslan