ClientManager

[imhoffd]

Here's an implementation of a potential client pool for this library. The request originated from #21. It has a few features one might expect in a pool, such as a maximum size and age (both of which can be disabled), as well as a way to create clients automatically if not found in the pool. Clients are looked up by doing a sha1 hash of the credential's bytes.

Implementation is basically a LRU cache with an optional time constraint. Lookups are O(1) using a map. LRU is achieved with a doubly linked list. These data structures are not thread safe, but the implementation should be thread safe as a simple mutex is wrapped around each potentially unsafe method.

I have not used this version in production. I wanted to submit this early to open discussion. What are some likes/dislikes, etc.

@sideshow @c3mb0 @nordicdyno

Credit to bradfitz: https://github.com/golang/groupcache/blob/master/lru/lru.go

[coveralls]

Coverage remained the same at 100.0% when pulling d810dcf on dwieeb:master into 729daff on sideshow:master.

[imhoffd]

I don't think the Go 1.4.3 failure in Travis is my doing, but I could be wrong.

[sideshow]

Thanks for the pull request @dwieeb
Looks pretty cool, I will have a play with this tomorrow

[imhoffd]

@sideshow You're welcome. Does everything look good?

[kirk91]

Behaviors of pool more like session . It support multiple application, but one application only hold one tcp connection @dwieeb

[imhoffd]

@AlphaPigger Right, it's not a "connection" pool one would be used to. What would a better name be? ClientSessionManager, ClientManager?

[kirk91]

ClientSessionManager, ClientManager is all ok,I just want to make sure the code that match up the point. @dwieeb

[coveralls]

Coverage remained the same at 100.0% when pulling fc2cbac on dwieeb:master into 729daff on sideshow:master.

[imhoffd]

Okay, that's renamed. All ready.

[NeoCN]

Is there any update on this？

[sideshow]

@dwieeb Sorry for the delay on reviewing this. We are doing something very similar in an internal project which is used in production, so my way of testing this was to try swapping out our current implementation for yours which looks superior. I think this will be an awesome addition to the library. Thanks again an will try and review shortly.

[imhoffd]

@sideshow Great to hear! I look forward to pulling this in.

[imhoffd]

@sideshow Have you had time to review it again? Really looking forward to having this in the library.

[nordicdyno]

Is only me seeing the problem with potential connection leaking (after eviction) in this code?

[sideshow]

Hey @dwieeb We are currently trying to merge into a large project to test.

[sideshow]

@nordicdyno can you be more specific. Thanks

[nordicdyno]

@sideshow There is no explicit disconnection before m.removeElement(ele). So I aware that system sockets could be held by Client's what are already removed from user space.

[sideshow]

@nordicdyno I ran some tests and this does leak/hold connections open after they are removed from the ConnectionManager. We need some kind of explicit CloseConnections method on the client which we can call before removing them. @dwieeb with the exception of this, it has been tested and is ready to merge. I just want to fix this before I merge in (any help appreciated). Thanks again for the code!

[imhoffd]

@nordicdyno @sideshow To me, this is normal behavior. See https://golang.org/src/net/http/transport.go#L54

57    // By default, Transport caches connections for future re-use.
58    // This may leave many open connections when accessing many hosts.
59    // This behavior can be managed using Transport's CloseIdleConnections method
60    // and the MaxIdleConnsPerHost and DisableKeepAlives fields.

I believe this PR has merely revealed a larger issue with this library, in that anyone who is "accessing many hosts" (by creating many Clients) will have connections left open. I also believe this is fine (it is the normal behavior of the http go libs).

If we want to address this, we can provide configuration options for http2.Transport (https://github.com/sideshow/apns2/blob/master/client.go#L60-L65), but the fine-tuning of this I believe should be left on the library consumer.

[sideshow]

@dwieeb "I believe this PR has merely revealed a larger issue with this library" Agree with this. I will go ahead and merge and we can address this as a seperate issue. Thanks again for the PR!