-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix locking in getConnection #288
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ import ( | |
"sync" | ||
|
||
"google.golang.org/grpc" | ||
|
||
"vitess.io/vitess/go/sqlescape" | ||
"vitess.io/vitess/go/sqltypes" | ||
"vitess.io/vitess/go/vt/grpcclient" | ||
|
@@ -59,32 +60,36 @@ func (proxy *VTGateProxy) getConnection(ctx context.Context, target string) (*vt | |
// If the connection exists, return it | ||
proxy.mu.RLock() | ||
existingConn := proxy.targetConns[target] | ||
proxy.mu.RUnlock() | ||
|
||
if existingConn != nil { | ||
proxy.mu.RUnlock() | ||
log.V(100).Infof("Reused connection for %v\n", target) | ||
return existingConn, nil | ||
} | ||
|
||
// No luck, need to create a new one. Serialize new additions so we don't create multiple | ||
// for a given target. | ||
log.V(100).Infof("Need to create connection for %v\n", target) | ||
proxy.mu.RUnlock() | ||
|
||
proxy.mu.Lock() | ||
defer proxy.mu.Unlock() | ||
|
||
// Otherwise create a new connection after dropping the lock, allowing multiple requests to | ||
// race to create the conn for now. | ||
grpcclient.RegisterGRPCDialOptions(func(opts []grpc.DialOption) ([]grpc.DialOption, error) { | ||
return append(opts, grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"round_robin":{}}]}`)), nil | ||
}) | ||
// Check again in case conn was made between lock acquisitions. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also very good idea, thanks. |
||
existingConn = proxy.targetConns[target] | ||
if existingConn != nil { | ||
log.V(100).Infof("Reused connection for %v\n", target) | ||
return existingConn, nil | ||
} | ||
|
||
// Otherwise create a new connection. TODO: confirm this doesn't actually make a TCP connection, and returns quickly, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For sure it does not. There's a WithBlock thing that IIRC you can pass to make this blocking, but if not then it is non-blocking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ya - confirmed, the model in grpc is pretty weird, you create a "conn", but that's basically just a handle to make RPC requests. It's not until you make an RPC call that it will actually start mapping the RPC to a subconn (which maps to a TCP connection). That's also why you can reuse the conn - it should represent a logical target to send the RPC commands to. |
||
// otherwise we're going to have to do this while not holding the lock. | ||
conn, err := vtgateconn.DialProtocol(ctx, "grpc", target) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
log.V(100).Infof("Created new connection for %v\n", target) | ||
proxy.targetConns[target] = conn | ||
proxy.mu.Unlock() | ||
|
||
return conn, nil | ||
} | ||
|
@@ -174,4 +179,8 @@ func (proxy *VTGateProxy) StreamExecute(ctx context.Context, session *vtgateconn | |
} | ||
|
||
func Init() { | ||
log.V(100).Infof("Registering GRPC dial options") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eek - I think demmer and I talked about this and never fixed it - thank you for moving this here and doing it once! |
||
grpcclient.RegisterGRPCDialOptions(func(opts []grpc.DialOption) ([]grpc.DialOption, error) { | ||
return append(opts, grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"round_robin":{}}]}`)), nil | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops much better thanks.
But I think we still need the RUnlock() because IIRC you can't "promote" a RWLock without first dropping the read lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's still there - just moved above the log / error checking logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG duuuh.