Copilot commented on code in PR #1484:
URL: https://github.com/apache/pulsar-client-go/pull/1484#discussion_r3239079268
##########
pulsar/internal/connection.go:
##########
@@ -654,35 +651,43 @@ func (c *connection) checkServerError(err
*pb.ServerError) {
}
}
+func (c *connection) registerIncomingRequest() bool {
+ c.mu.RLock()
+ defer c.mu.RUnlock()
+
+ if c.getState() == connectionClosed {
+ return false
+ }
+
+ c.incomingRequestsWG.Add(1)
+ return true
+}
+
func (c *connection) SendRequest(requestID uint64, req *pb.BaseCommand,
callback func(command *pb.BaseCommand, err error)) {
- c.incomingRequestsWG.Add(1)
+ if !c.registerIncomingRequest() {
+ callback(req, ErrConnectionClosed)
+ return
+ }
defer c.incomingRequestsWG.Done()
- if c.getState() == connectionClosed {
+ select {
+ case <-c.closeCh:
callback(req, ErrConnectionClosed)
- } else {
- select {
- case <-c.closeCh:
- callback(req, ErrConnectionClosed)
-
- case c.incomingRequestsCh <- &request{
- id: &requestID,
- cmd: req,
- callback: callback,
- }:
- }
+ case c.incomingRequestsCh <- &request{
+ id: &requestID,
+ cmd: req,
+ callback: callback,
+ }:
}
}
func (c *connection) SendRequestNoWait(req *pb.BaseCommand) error {
- c.incomingRequestsWG.Add(1)
- defer c.incomingRequestsWG.Done()
-
- if c.getState() == connectionClosed {
+ if !c.registerIncomingRequest() {
return ErrConnectionClosed
}
+ defer c.incomingRequestsWG.Done()
Review Comment:
The regression being fixed is the Add/Wait race in SendRequest and
SendRequestNoWait during Close, but the added test only covers WriteData on an
already closed connection. Please add a regression test that exercises
concurrent SendRequest/SendRequestNoWait calls racing with
Close/failLeftRequestsWhenClose so the WaitGroup misuse is covered directly.
--
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]
