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.

Sign up for GitHub

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

Jump to bottom

Some fixes to connection pooling #7045

Merged
merged 2 commits into from
Feb 3, 2020
Merged

Some fixes to connection pooling #7045

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82b8e08
Some fixes to connection pooling
Jan 31, 2020
3bfc7d1
pool: Clear connection before releasing
Feb 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions helper/pool/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"sync/atomic"
"time"

"github.com/hashicorp/consul/lib"
hclog "github.com/hashicorp/go-hclog"
msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
"github.com/hashicorp/nomad/helper/tlsutil"
Expand All @@ -36,6 +37,7 @@ type StreamClient struct {

func (sc *StreamClient) Close() {
sc.stream.Close()
sc.codec.Close()
}

// Conn is a pooled connection to a Nomad server
Expand Down Expand Up @@ -427,6 +429,13 @@ func (p *ConnPool) RPC(region string, addr net.Addr, version int, method string,
sc.Close()
p.releaseConn(conn)

// If we read EOF, the session is toast. Clear it and open a
// new session next time
// See https://github.com/hashicorp/consul/blob/v1.6.3/agent/pool/pool.go#L471-L477
if lib.IsErrEOF(err) {
p.clearConn(conn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In getClient above on L406 we call release and clear in the opposite order. Looking at the code I don't think it matters and calling releaseConn doesn't even seem necessary if we're calling clearConn.

It might be nice to make the 2 clear+releaseConn cases the same, but it's not a blocker. I don't see any reason why either would be unsafe.

Copy link
Contributor Author

@notnoop notnoop Feb 3, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - also your proposed order matches Consul's code too. Thanks for catching this.

}

// If the error is an RPC Coded error
// return the coded error without wrapping
if structs.IsErrRPCCoded(err) {
Expand Down