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

Fix locking in getConnection #288

Merged
merged 3 commits into from
Apr 10, 2024
Merged

Fix locking in getConnection #288

merged 3 commits into from
Apr 10, 2024

Conversation

henryr
Copy link

@henryr henryr commented Apr 10, 2024

  • Fix missing Unlock if an error happens when creating a connection
  • Only register global dial options once
  • Don't try to create a connection if one was made while waiting for write lock.

henryr added 3 commits April 10, 2024 12:56
Signed-off-by: Henry Robinson <[email protected]>
Signed-off-by: Henry Robinson <[email protected]>
Signed-off-by: Henry Robinson <[email protected]>
@henryr henryr requested a review from demmer April 10, 2024 12:09
proxy.mu.Lock()
defer proxy.mu.Unlock()
Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OMG duuuh.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also very good idea, thanks.


// Otherwise create a new connection. TODO: confirm this doesn't actually make a TCP connection, and returns quickly,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Choose a reason for hiding this comment

The 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.

@@ -174,4 +179,8 @@ func (proxy *VTGateProxy) StreamExecute(ctx context.Context, session *vtgateconn
}

func Init() {
log.V(100).Infof("Registering GRPC dial options")

Choose a reason for hiding this comment

The 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!

@henryr henryr merged commit 83f23bc into vtgateproxy Apr 10, 2024
152 of 241 checks passed
@henryr henryr deleted the hnr-fix-cnxn-locking branch April 10, 2024 18:30
Copy link
Collaborator

@demmer demmer left a comment

Choose a reason for hiding this comment

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

Yep looks great to me.

dedelala pushed a commit that referenced this pull request Jul 30, 2024
* Fix locking in getConnection

Signed-off-by: Henry Robinson <[email protected]>

* Comment

Signed-off-by: Henry Robinson <[email protected]>

* Undo bash change

Signed-off-by: Henry Robinson <[email protected]>

---------

Signed-off-by: Henry Robinson <[email protected]>
dedelala pushed a commit that referenced this pull request Nov 12, 2024
* Fix locking in getConnection

Signed-off-by: Henry Robinson <[email protected]>

* Comment

Signed-off-by: Henry Robinson <[email protected]>

* Undo bash change

Signed-off-by: Henry Robinson <[email protected]>

---------

Signed-off-by: Henry Robinson <[email protected]>
dedelala pushed a commit that referenced this pull request Jan 8, 2025
* Fix locking in getConnection

Signed-off-by: Henry Robinson <[email protected]>

* Comment

Signed-off-by: Henry Robinson <[email protected]>

* Undo bash change

Signed-off-by: Henry Robinson <[email protected]>

---------

Signed-off-by: Henry Robinson <[email protected]>
dedelala pushed a commit that referenced this pull request Jan 29, 2025
* Fix locking in getConnection

Signed-off-by: Henry Robinson <[email protected]>

* Comment

Signed-off-by: Henry Robinson <[email protected]>

* Undo bash change

Signed-off-by: Henry Robinson <[email protected]>

---------

Signed-off-by: Henry Robinson <[email protected]>
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.

3 participants