BewareMyPower commented on pull request #663:
URL: https://github.com/apache/pulsar-client-go/pull/663#issuecomment-963751704
The current `waitUntilReady` implementation is
```go
func (c *connection) waitUntilReady() error {
c.Lock()
defer c.Unlock()
c.cond.L.Lock()
defer c.cond.L.Unlock()
for c.getState() != connectionReady {
c.log.Debugf("Wait until connection is ready state=%s",
c.getState().String())
// 1. `c.getState()` is called again
if c.getState() == connectionClosed {
return errors.New("connection error")
}
// 2. Even if the 2nd call of `c.getState()` returns
connectionReady, `c.cond.Wait()` will still be called
c.cond.Wait()
}
return nil
}
```
See my comments for my concern. `getState()` and `setState()` are both
atomic. But if we the combination of two `getState()` calls is not atomic.
Therefore, I think a better solution is
```go
func (c *connection) waitUntilReady() error {
for {
switch state := c.getState(); state {
case connectionReady:
return nil
case connectionClosed:
return errors.New("connection error")
default:
c.log.Debugf("Wait until connection is ready state=%s",
state.String())
// Here we use c.cond.L.Lock() because we might not use
c.Lock as the cond's internal Locker in future
c.cond.L.Lock()
defer c.cond.L.Unlock()
c.cond.Wait()
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
