rueian commented on a change in pull request #248:
URL: https://github.com/apache/pulsar-client-go/pull/248#discussion_r429380427
##########
File path: pulsar/internal/connection.go
##########
@@ -656,18 +662,20 @@ func (c *connection) Close() {
c.log.Info("Connection closed")
c.state = connectionClosed
- if c.cnx != nil {
- c.cnx.Close()
- }
+ c.TriggerClose()
c.pingTicker.Stop()
c.pingCheckTicker.Stop()
for _, listener := range c.listeners {
listener.ConnectionClosed()
}
- for _, req := range c.pendingReqs {
+ if c.runLoopStoppedCh != nil {
Review comment:
After tracing the call path of the `Close()`, I found that it is called
directly by the `Close()` in the `client_impl.go`.
When performing a graceful shutdown, although normally we will first call
the `producer.Close()/consumer.Close()` and then call `client.Close()` at the
end. I think it is still a good idea to keep the `Close()` as a sync operation
to remain similar behavior between the `producer.Close()/consumer.Close()`.
We could move the cleanup code of `c.pendingReqs`, `c.listeners` and
`c.consumerHandlers` into the run loop. However in that case, I would suggest
keeping the `runLoopStoppedCh` channel to inform the `Close()` that the cleanup
process has finished.
What do you think? @cckellogg
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
