Redo API locking #3508
Redo API locking #3508
Conversation
predictable and add locking to make it safer to use with Clone() and if multiple goroutines for some reason decide to change things. Along the way I discovered that currently, the x/net/http2 package is broke with the built-in h2 support in released Go. For those using DefaultConfig (the vast majority of cases) this will be a non-event. Others can manually call http2.ConfigureTransport as needed. We should keep an eye on commits on that repo and consider more updates before release. Alternately we could go back revisions but miss out on bug fixes; my theory is that this is not a purposeful break and I'll be following up on this in the Go issue tracker. In a few tests that don't use NewTestCluster, either for legacy or other reasons, ensure that http2.ConfigureTransport is called.
| @@ -43,18 +43,21 @@ type WrappingLookupFunc func(operation, path string) string | |||
|
|
|||
| // Config is used to configure the creation of the client. | |||
| type Config struct { | |||
| modifyLock sync.RWMutex | |||
There was a problem hiding this comment.
Nitpick here, but can we go for embedding sync.RWMutex so that we call c.Lock() vs c.modifyLock.Lock()?
There was a problem hiding this comment.
I don't want to allow API consumers to toggle the lock...doing it this way makes it unexpired.
There was a problem hiding this comment.
Add that makes sense, nvm then!
| c.modifyLock.RLock() | ||
| c.config.modifyLock.RLock() | ||
| defer c.config.modifyLock.RUnlock() | ||
| c.modifyLock.RUnlock() |
There was a problem hiding this comment.
I think it wouldn't hurt if we defer here.
There was a problem hiding this comment.
Doing so would mean holding the lock for the lifetime of the client request, including redirects and timeouts, even though we no longer need to access anything it's protecting.
There was a problem hiding this comment.
You’re right, holding the client lock would be unnecessary.
jefferai
force-pushed
the
redo-api-client
branch
from
e326543 to
949549d
Compare
…rly without requiring the http2 package
api/client.go
Outdated
| func (c *Client) Clone() (*Client, error) { | ||
| return NewClient(c.config) | ||
| c.config.modifyLock.RLock() |
There was a problem hiding this comment.
Should this also call c.modifyLock.RLock()?
Redo the API client quite a bit to make the behavior of NewClient more
predictable and add locking to make it safer to use with Clone() and if
multiple goroutines for some reason decide to change things.
In a few tests that don't use NewTestCluster, either for legacy or other
reasons, ensure that http2.ConfigureTransport is called.
Fixes #3435