You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:36:06 UTC
[GitHub] [pulsar-client-go] wolfstudy commented on a change in pull request #337: Avoid producer deadlock on connection closing
wolfstudy commented on a change in pull request #337:
URL: https://github.com/apache/pulsar-client-go/pull/337#discussion_r461983200
##########
File path: pulsar/internal/connection.go
##########
@@ -729,7 +756,7 @@ func (c *connection) Close() {
func (c *connection) changeState(state connectionState) {
c.Lock()
- c.state = state
+ atomic.StoreInt32(&c.state, int32(state))
Review comment:
In here, Is it necessary for us to `c.Lock()` and `c.Unlock()` actions on an atomic operation?
##########
File path: pulsar/internal/connection.go
##########
@@ -729,7 +756,7 @@ func (c *connection) Close() {
func (c *connection) changeState(state connectionState) {
c.Lock()
- c.state = state
+ atomic.StoreInt32(&c.state, int32(state))
Review comment:
I am wondering if there are other problems with this approach, because for an atomic primitive, we rarely see the operation of Lock and Unlock it.
```
atomic.StoreInt32(&c.state, int32(state))
c.Lock()
c.cond.Broadcast()
c.Unlock()
```
##########
File path: pulsar/internal/connection.go
##########
@@ -729,7 +756,7 @@ func (c *connection) Close() {
func (c *connection) changeState(state connectionState) {
c.Lock()
- c.state = state
+ atomic.StoreInt32(&c.state, int32(state))
Review comment:
yes, i agree with you, The main difference between us here is whether we need to lock to protect the atomic primitive, I think it is not needed, atomic itself is a synchronization primitive, so here, we can reduce the granularity of the lock and only lock sync.cond. The code example is as follows:
```
atomic.StoreInt32(&c.state, int32(state))
c.Lock()
c.cond.Broadcast()
c.Unlock()
```
##########
File path: pulsar/internal/connection.go
##########
@@ -729,7 +756,7 @@ func (c *connection) Close() {
func (c *connection) changeState(state connectionState) {
c.Lock()
- c.state = state
+ atomic.StoreInt32(&c.state, int32(state))
Review comment:
yes, i agree with you, The main difference between us here is whether we need to lock to protect the atomic primitive, I think it is not needed, atomic itself is a synchronization primitive, so here, we can reduce the scope of the lock and only lock sync.cond. The code example is as follows:
```
atomic.StoreInt32(&c.state, int32(state))
c.Lock()
c.cond.Broadcast()
c.Unlock()
```
##########
File path: pulsar/internal/connection.go
##########
@@ -729,7 +756,7 @@ func (c *connection) Close() {
func (c *connection) changeState(state connectionState) {
c.Lock()
- c.state = state
+ atomic.StoreInt32(&c.state, int32(state))
Review comment:
Please ignore me
----------------------------------------------------------------
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:
users@infra.apache.org