You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2021/02/05 15:40:27 UTC

[GitHub] [dubbo-go] AlexStocks opened a new pull request #1045: Fix: DubboInvoker.reqnum should be atomic

AlexStocks opened a new pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045


   <!--  Thanks for sending a pull request!
   Read https://github.com/apache/dubbo-go/blob/master/contributing.md before commit pull request. 
   -->
   
   **What this PR does**:
   fix bug: protocol/dubbo/dubbo_invoker.go:DubboInvoker.reqNum should be a atomic number


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573836198



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,31 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {
 		// Generally, the case will not happen, because the invoker has been removed
 		// from the invoker list before destroy,so no new request will enter the destroyed invoker
 		logger.Warnf("this dubboInvoker is destroyed")
-		result.Err = ErrDestroyedInvoker
+		result.Err = protocol.ErrDestroyedInvoker
+		return &result
+	}
+
+	di.clientGuard.RLock()
+	defer di.clientGuard.RUnlock()
+
+	client := di.client

Review comment:
       here we use Rlock to protect client, so we can use di.client directly, new variable client is not needed




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r571593524



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -123,18 +138,22 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 	//response := NewResponse(inv.Reply(), nil)
 	rest := &protocol.RPCResult{}
 	timeout := di.getTimeout(inv)
-	if async {
-		if callBack, ok := inv.CallBack().(func(response common.CallbackResponse)); ok {
-			//result.Err = di.client.AsyncCall(NewRequest(url.Location, url, inv.MethodName(), inv.Arguments(), inv.Attachments()), callBack, response)
-			result.Err = di.client.AsyncRequest(&invocation, url, timeout, callBack, rest)
-		} else {
-			result.Err = di.client.Send(&invocation, url, timeout)
-		}
+	client := di.getClient()

Review comment:
       getClient返回的是client指针，下面关闭的时候，同样会把这个client赋值为nil




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] flycash commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
flycash commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r571636678



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,24 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {

Review comment:
       It's double-check pattern, I think.

##########
File path: protocol/grpc/grpc_invoker.go
##########
@@ -83,21 +127,47 @@ func (gi *GrpcInvoker) Invoke(ctx context.Context, invocation protocol.Invocatio
 
 // IsAvailable get available status
 func (gi *GrpcInvoker) IsAvailable() bool {
-	return gi.BaseInvoker.IsAvailable() && gi.client.GetState() != connectivity.Shutdown
+	client := gi.getClient()
+	if client != nil {
+		return gi.BaseInvoker.IsAvailable() && client.GetState() != connectivity.Shutdown
+	}
+
+	return false
 }
 
 // IsDestroyed get destroyed status
 func (gi *GrpcInvoker) IsDestroyed() bool {
-	return gi.BaseInvoker.IsDestroyed() && gi.client.GetState() == connectivity.Shutdown
+	client := gi.getClient()
+	if client != nil {
+		return gi.BaseInvoker.IsDestroyed() && client.GetState() == connectivity.Shutdown
+	}
+
+	return false
 }
 
 // Destroy will destroy gRPC's invoker and client, so it is only called once
 func (gi *GrpcInvoker) Destroy() {
 	gi.quitOnce.Do(func() {
-		gi.BaseInvoker.Destroy()
-
-		if gi.client != nil {
-			_ = gi.client.Close()
+		gi.BaseInvoker.Stop()
+		var times int64
+		for {
+			times = gi.BaseInvoker.InvokeTimes()
+			if times == 0 {
+				gi.BaseInvoker.AddInvokerTimes(-1)
+				logger.Infof("grpcInvoker is destroyed, url:{%s}", gi.GetUrl().Key())
+				gi.BaseInvoker.Destroy()
+				client := gi.getClient()
+				if client != nil {
+					client.Close()
+					gi.setClient(nil)

Review comment:
       I am not sure whether exchange those two lines' order:
   ```
   gi.setClient(nil)
   client.Close()
   ```
   It looks more safe.

##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -162,27 +181,38 @@ func (di *DubboInvoker) getTimeout(invocation *invocation_impl.RPCInvocation) ti
 }
 
 func (di *DubboInvoker) IsAvailable() bool {
-	return di.client.IsAvailable()
+	client := di.getClient()
+	if client != nil {
+		return client.IsAvailable()
+	}
+
+	return false
 }
 
 // Destroy destroy dubbo client invoker.
 func (di *DubboInvoker) Destroy() {
 	di.quitOnce.Do(func() {
+		di.BaseInvoker.Stop()
+		var times int64
 		for {
-			if di.reqNum == 0 {
-				di.reqNum = -1
-				logger.Infof("dubboInvoker is destroyed,url:{%s}", di.GetUrl().Key())
+			times = di.BaseInvoker.InvokeTimes()
+			if times == 0 {
+				di.BaseInvoker.AddInvokerTimes(-1)

Review comment:
       -1 indicates `BaseInvoker` is unavailable




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r574249357



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -101,8 +101,12 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		return &result
 	}
 
-	di.AddInvokerTimes(1)
-	defer di.AddInvokerTimes(-1)
+	client := di.getClient()
+	if client == nil {
+		result.Err = protocol.ErrClientClosed
+		logger.Debugf("result.Err: %v", result.Err)

Review comment:
       good suggestion.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573600852



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,28 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {
 		// Generally, the case will not happen, because the invoker has been removed
 		// from the invoker list before destroy,so no new request will enter the destroyed invoker
 		logger.Warnf("this dubboInvoker is destroyed")
-		result.Err = ErrDestroyedInvoker
+		result.Err = protocol.ErrDestroyedInvoker
+		return &result
+	}
+
+	client := di.getClient()

Review comment:
       这个地方个人感觉完全可以把锁提出来
   
   di.clientGuard.RLock()
   client := di.client
   defer di.clientGuard.RUnlock()
   
   这样可以保证有请求的时候，不会说获取到client之后，到后面的client.call的时候，报链接关闭的错误。
   
   
   




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573839340



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,31 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {
 		// Generally, the case will not happen, because the invoker has been removed
 		// from the invoker list before destroy,so no new request will enter the destroyed invoker
 		logger.Warnf("this dubboInvoker is destroyed")
-		result.Err = ErrDestroyedInvoker
+		result.Err = protocol.ErrDestroyedInvoker
+		return &result
+	}
+
+	di.clientGuard.RLock()
+	defer di.clientGuard.RUnlock()
+
+	client := di.client
+	if client == nil {
+		result.Err = protocol.ErrClientClosed

Review comment:
       may be client should never be nil with di.clientGuard.RLock() ?




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] gaoxinge edited a comment on pull request #1045: Fix: DubboInvoker.reqnum should be atomic

Posted by GitBox <gi...@apache.org>.
gaoxinge edited a comment on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-774380816


   I think some specific interleaving execution of `Invoke` and `Destroy` may lead to panic:
   
   |Invoke|Destroy|Description|
   |-------|---------|------------|
   |di.closed.Load() / di.reqNum().Load()<0||return false because destroy has not been called|
   ||di.closed.Store(true)||
   ||di.reqNum.Load() == 0|return true because no reqNum has been added in invoke|
   ||di.reqNum.Add(-1)||
   ||di.client = nil|set di.client to nil|
   |di.reqNum.Add(1)|||
   |di.client.Request||may panic because di.client is nil||
   |di.reqNum.Add(-1)|||


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-776571924


   @flycash pls check it again. I have reconstructed this pr as @wenxuwan's suggestion.


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] codecov-io edited a comment on pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-774497965


   # [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=h1) Report
   > Merging [#1045](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=desc) (5d8bcb4) into [1.5](https://codecov.io/gh/apache/dubbo-go/commit/968650f658b63c11bb0409897d29c57b91cfaf50?el=desc) (968650f) will **decrease** coverage by `0.22%`.
   > The diff coverage is `55.87%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go/pull/1045/graphs/tree.svg?width=650&height=150&src=pr&token=dcPE6RyFAL)](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##              1.5    #1045      +/-   ##
   ==========================================
   - Coverage   59.53%   59.30%   -0.23%     
   ==========================================
     Files         259      265       +6     
     Lines       12737    13137     +400     
   ==========================================
   + Hits         7583     7791     +208     
   - Misses       4199     4362     +163     
   - Partials      955      984      +29     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cluster/cluster\_impl/available\_cluster.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9jbHVzdGVyX2ltcGwvYXZhaWxhYmxlX2NsdXN0ZXIuZ28=) | `100.00% <ø> (ø)` | |
   | [cluster/router/healthcheck/factory.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvaGVhbHRoY2hlY2svZmFjdG9yeS5nbw==) | `66.66% <0.00%> (ø)` | |
   | [cluster/router/tag/router\_rule.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvdGFnL3JvdXRlcl9ydWxlLmdv) | `89.47% <ø> (+12.20%)` | :arrow_up: |
   | [common/config/environment.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2NvbmZpZy9lbnZpcm9ubWVudC5nbw==) | `51.72% <ø> (ø)` | |
   | [common/extension/config\_post\_processor.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9jb25maWdfcG9zdF9wcm9jZXNzb3IuZ28=) | `0.00% <0.00%> (ø)` | |
   | [common/extension/conn\_checker.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9jb25uX2NoZWNrZXIuZ28=) | `0.00% <0.00%> (ø)` | |
   | [common/extension/metadata\_service.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9tZXRhZGF0YV9zZXJ2aWNlLmdv) | `0.00% <0.00%> (ø)` | |
   | [common/proxy/proxy\_factory/default.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL3Byb3h5L3Byb3h5X2ZhY3RvcnkvZGVmYXVsdC5nbw==) | `13.55% <0.00%> (+0.22%)` | :arrow_up: |
   | [config/base\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29uZmlnL2Jhc2VfY29uZmlnLmdv) | `64.02% <ø> (ø)` | |
   | [config\_center/configurator/override.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29uZmlnX2NlbnRlci9jb25maWd1cmF0b3Ivb3ZlcnJpZGUuZ28=) | `72.22% <0.00%> (ø)` | |
   | ... and [161 more](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=footer). Last update [7d0b63a...d84ece5](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r571589352



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -162,27 +181,38 @@ func (di *DubboInvoker) getTimeout(invocation *invocation_impl.RPCInvocation) ti
 }
 
 func (di *DubboInvoker) IsAvailable() bool {
-	return di.client.IsAvailable()
+	client := di.getClient()
+	if client != nil {
+		return client.IsAvailable()
+	}
+
+	return false
 }
 
 // Destroy destroy dubbo client invoker.
 func (di *DubboInvoker) Destroy() {
 	di.quitOnce.Do(func() {
+		di.BaseInvoker.Stop()
+		var times int64
 		for {
-			if di.reqNum == 0 {
-				di.reqNum = -1
-				logger.Infof("dubboInvoker is destroyed,url:{%s}", di.GetUrl().Key())
+			times = di.BaseInvoker.InvokeTimes()
+			if times == 0 {
+				di.BaseInvoker.AddInvokerTimes(-1)
+				logger.Infof("dubboInvoker is destroyed, url:{%s}", di.GetUrl().Key())
 				di.BaseInvoker.Destroy()
-				if di.client != nil {
-					di.client.Close()
-					di.client = nil
+				client := di.getClient()
+				if client != nil {
+					client.Close()
+					di.setClient(nil)
 				}
 				break
+			} else if times < 0 {

Review comment:
       This check not needed




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] codecov-io commented on pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-774497965


   # [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=h1) Report
   > Merging [#1045](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=desc) (ef22815) into [1.5](https://codecov.io/gh/apache/dubbo-go/commit/968650f658b63c11bb0409897d29c57b91cfaf50?el=desc) (968650f) will **decrease** coverage by `0.23%`.
   > The diff coverage is `54.15%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go/pull/1045/graphs/tree.svg?width=650&height=150&src=pr&token=dcPE6RyFAL)](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##              1.5    #1045      +/-   ##
   ==========================================
   - Coverage   59.53%   59.30%   -0.24%     
   ==========================================
     Files         259      261       +2     
     Lines       12737    13068     +331     
   ==========================================
   + Hits         7583     7750     +167     
   - Misses       4199     4334     +135     
   - Partials      955      984      +29     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cluster/cluster\_impl/available\_cluster.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9jbHVzdGVyX2ltcGwvYXZhaWxhYmxlX2NsdXN0ZXIuZ28=) | `100.00% <ø> (ø)` | |
   | [cluster/cluster\_impl/base\_cluster\_invoker.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9jbHVzdGVyX2ltcGwvYmFzZV9jbHVzdGVyX2ludm9rZXIuZ28=) | `61.11% <0.00%> (-5.56%)` | :arrow_down: |
   | [cluster/router/condition/listenable\_router.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvY29uZGl0aW9uL2xpc3RlbmFibGVfcm91dGVyLmdv) | `54.71% <0.00%> (ø)` | |
   | [cluster/router/healthcheck/health\_check\_route.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvaGVhbHRoY2hlY2svaGVhbHRoX2NoZWNrX3JvdXRlLmdv) | `70.37% <0.00%> (ø)` | |
   | [cluster/router/tag/router\_rule.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvdGFnL3JvdXRlcl9ydWxlLmdv) | `89.47% <ø> (+12.20%)` | :arrow_up: |
   | [cluster/router/tag/tag\_router.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvdGFnL3RhZ19yb3V0ZXIuZ28=) | `72.59% <0.00%> (ø)` | |
   | [common/config/environment.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2NvbmZpZy9lbnZpcm9ubWVudC5nbw==) | `51.72% <ø> (ø)` | |
   | [common/extension/config\_post\_processor.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9jb25maWdfcG9zdF9wcm9jZXNzb3IuZ28=) | `0.00% <0.00%> (ø)` | |
   | [common/extension/metadata\_service.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9tZXRhZGF0YV9zZXJ2aWNlLmdv) | `0.00% <0.00%> (ø)` | |
   | [common/proxy/proxy\_factory/default.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL3Byb3h5L3Byb3h5X2ZhY3RvcnkvZGVmYXVsdC5nbw==) | `13.55% <0.00%> (+0.22%)` | :arrow_up: |
   | ... and [151 more](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=footer). Last update [7d0b63a...ef22815](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] gaoxinge commented on a change in pull request #1045: Fix: DubboInvoker.reqnum should be atomic

Posted by GitBox <gi...@apache.org>.
gaoxinge commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r571335661



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -62,7 +62,8 @@ type DubboInvoker struct {
 	// timeout for service(interface) level.
 	timeout time.Duration
 	// Used to record the number of requests. -1 represent this DubboInvoker is destroyed
-	reqNum int64
+	reqNum uatomic.Int64
+	closed  uatomic.Bool

Review comment:
       `go fmt`.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r571737529



##########
File path: protocol/grpc/grpc_invoker.go
##########
@@ -83,21 +127,47 @@ func (gi *GrpcInvoker) Invoke(ctx context.Context, invocation protocol.Invocatio
 
 // IsAvailable get available status
 func (gi *GrpcInvoker) IsAvailable() bool {
-	return gi.BaseInvoker.IsAvailable() && gi.client.GetState() != connectivity.Shutdown
+	client := gi.getClient()
+	if client != nil {
+		return gi.BaseInvoker.IsAvailable() && client.GetState() != connectivity.Shutdown
+	}
+
+	return false
 }
 
 // IsDestroyed get destroyed status
 func (gi *GrpcInvoker) IsDestroyed() bool {
-	return gi.BaseInvoker.IsDestroyed() && gi.client.GetState() == connectivity.Shutdown
+	client := gi.getClient()
+	if client != nil {
+		return gi.BaseInvoker.IsDestroyed() && client.GetState() == connectivity.Shutdown
+	}
+
+	return false
 }
 
 // Destroy will destroy gRPC's invoker and client, so it is only called once
 func (gi *GrpcInvoker) Destroy() {
 	gi.quitOnce.Do(func() {
-		gi.BaseInvoker.Destroy()
-
-		if gi.client != nil {
-			_ = gi.client.Close()
+		gi.BaseInvoker.Stop()
+		var times int64
+		for {
+			times = gi.BaseInvoker.InvokeTimes()
+			if times == 0 {
+				gi.BaseInvoker.AddInvokerTimes(-1)
+				logger.Infof("grpcInvoker is destroyed, url:{%s}", gi.GetUrl().Key())
+				gi.BaseInvoker.Destroy()
+				client := gi.getClient()
+				if client != nil {
+					client.Close()
+					gi.setClient(nil)

Review comment:
       good advice.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573533985



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -162,27 +181,38 @@ func (di *DubboInvoker) getTimeout(invocation *invocation_impl.RPCInvocation) ti
 }
 
 func (di *DubboInvoker) IsAvailable() bool {
-	return di.client.IsAvailable()
+	client := di.getClient()
+	if client != nil {
+		return client.IsAvailable()
+	}
+
+	return false
 }
 
 // Destroy destroy dubbo client invoker.
 func (di *DubboInvoker) Destroy() {
 	di.quitOnce.Do(func() {
+		di.BaseInvoker.Stop()
+		var times int64
 		for {
-			if di.reqNum == 0 {
-				di.reqNum = -1
-				logger.Infof("dubboInvoker is destroyed,url:{%s}", di.GetUrl().Key())
+			times = di.BaseInvoker.InvokeTimes()
+			if times == 0 {
+				di.BaseInvoker.AddInvokerTimes(-1)
+				logger.Infof("dubboInvoker is destroyed, url:{%s}", di.GetUrl().Key())
 				di.BaseInvoker.Destroy()
-				if di.client != nil {
-					di.client.Close()
-					di.client = nil
+				client := di.getClient()
+				if client != nil {
+					client.Close()
+					di.setClient(nil)
 				}
 				break
+			} else if times < 0 {

Review comment:
       万一发生什么幺蛾子呢？防御性编程，这里面有日志的。




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] gaoxinge commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
gaoxinge commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r571572642



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -162,27 +181,38 @@ func (di *DubboInvoker) getTimeout(invocation *invocation_impl.RPCInvocation) ti
 }
 
 func (di *DubboInvoker) IsAvailable() bool {
-	return di.client.IsAvailable()
+	client := di.getClient()
+	if client != nil {
+		return client.IsAvailable()
+	}
+
+	return false
 }
 
 // Destroy destroy dubbo client invoker.
 func (di *DubboInvoker) Destroy() {
 	di.quitOnce.Do(func() {
+		di.BaseInvoker.Stop()
+		var times int64
 		for {
-			if di.reqNum == 0 {
-				di.reqNum = -1
-				logger.Infof("dubboInvoker is destroyed,url:{%s}", di.GetUrl().Key())
+			times = di.BaseInvoker.InvokeTimes()
+			if times == 0 {
+				di.BaseInvoker.AddInvokerTimes(-1)

Review comment:
       Why set invoke time to -1?

##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,24 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {

Review comment:
       This check is duplicated to below. What is it intended for?




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] flycash commented on pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
flycash commented on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-776585622


   > @flycash pls check it again. I have reconstructed this pr as @wenxuwan's suggestion.
   
   LGTM


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] codecov-io edited a comment on pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-774497965


   # [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=h1) Report
   > Merging [#1045](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=desc) (d84ece5) into [1.5](https://codecov.io/gh/apache/dubbo-go/commit/968650f658b63c11bb0409897d29c57b91cfaf50?el=desc) (968650f) will **decrease** coverage by `0.22%`.
   > The diff coverage is `53.73%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go/pull/1045/graphs/tree.svg?width=650&height=150&src=pr&token=dcPE6RyFAL)](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##              1.5    #1045      +/-   ##
   ==========================================
   - Coverage   59.53%   59.31%   -0.23%     
   ==========================================
     Files         259      265       +6     
     Lines       12737    13137     +400     
   ==========================================
   + Hits         7583     7792     +209     
   - Misses       4199     4362     +163     
   - Partials      955      983      +28     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cluster/cluster\_impl/available\_cluster.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9jbHVzdGVyX2ltcGwvYXZhaWxhYmxlX2NsdXN0ZXIuZ28=) | `100.00% <ø> (ø)` | |
   | [cluster/router/condition/listenable\_router.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvY29uZGl0aW9uL2xpc3RlbmFibGVfcm91dGVyLmdv) | `57.89% <0.00%> (+3.17%)` | :arrow_up: |
   | [cluster/router/healthcheck/health\_check\_route.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvaGVhbHRoY2hlY2svaGVhbHRoX2NoZWNrX3JvdXRlLmdv) | `71.42% <0.00%> (+1.05%)` | :arrow_up: |
   | [cluster/router/tag/router\_rule.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvdGFnL3JvdXRlcl9ydWxlLmdv) | `89.47% <ø> (+12.20%)` | :arrow_up: |
   | [cluster/router/tag/tag\_router.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y2x1c3Rlci9yb3V0ZXIvdGFnL3RhZ19yb3V0ZXIuZ28=) | `72.99% <ø> (+0.40%)` | :arrow_up: |
   | [common/config/environment.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2NvbmZpZy9lbnZpcm9ubWVudC5nbw==) | `51.72% <ø> (ø)` | |
   | [common/extension/config\_post\_processor.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9jb25maWdfcG9zdF9wcm9jZXNzb3IuZ28=) | `0.00% <0.00%> (ø)` | |
   | [common/extension/metadata\_service.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL2V4dGVuc2lvbi9tZXRhZGF0YV9zZXJ2aWNlLmdv) | `0.00% <0.00%> (ø)` | |
   | [common/proxy/proxy\_factory/default.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29tbW9uL3Byb3h5L3Byb3h5X2ZhY3RvcnkvZGVmYXVsdC5nbw==) | `13.55% <0.00%> (+0.22%)` | :arrow_up: |
   | [config/base\_config.go](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree#diff-Y29uZmlnL2Jhc2VfY29uZmlnLmdv) | `64.02% <ø> (ø)` | |
   | ... and [166 more](https://codecov.io/gh/apache/dubbo-go/pull/1045/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=footer). Last update [7d0b63a...d84ece5](https://codecov.io/gh/apache/dubbo-go/pull/1045?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] gaoxinge edited a comment on pull request #1045: Fix: DubboInvoker.reqnum should be atomic

Posted by GitBox <gi...@apache.org>.
gaoxinge edited a comment on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-774380816


   Some specific interleaving execution of `Invoke` and `Destroy` may lead to panic:
   
   |Invoke|Destroy|Description|
   |-------|---------|------------|
   |di.closed.Load()||return false because destroy has not been called|
   ||di.closed.Store(true)||
   ||di.reqNum.Load() == 0|return true because no reqNum has been added in invoke|
   ||di.reqNum.Add(-1)||
   ||di.client = nil|set di.client to nil|
   |di.reqNum.Add(1)|||
   |di.client.Request||may panic because di.client is nil||
   |di.reqNum.Add(-1)|||


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks merged pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks merged pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045


   


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] zouyx commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
zouyx commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r571518970



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -162,27 +181,38 @@ func (di *DubboInvoker) getTimeout(invocation *invocation_impl.RPCInvocation) ti
 }
 
 func (di *DubboInvoker) IsAvailable() bool {
-	return di.client.IsAvailable()
+	client := di.getClient()
+	if client != nil {
+		return client.IsAvailable()
+	}
+
+	return false
 }
 
 // Destroy destroy dubbo client invoker.
 func (di *DubboInvoker) Destroy() {
 	di.quitOnce.Do(func() {
+		di.BaseInvoker.Stop()
+		var times int64
 		for {
-			if di.reqNum == 0 {
-				di.reqNum = -1
-				logger.Infof("dubboInvoker is destroyed,url:{%s}", di.GetUrl().Key())
+			times = di.BaseInvoker.InvokeTimes()
+			if times == 0 {

Review comment:
       判断标准应该是 client 还是 times ?
   我觉得还是像以前一样都兼顾一下比较好

##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -123,18 +138,22 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 	//response := NewResponse(inv.Reply(), nil)
 	rest := &protocol.RPCResult{}
 	timeout := di.getTimeout(inv)
-	if async {
-		if callBack, ok := inv.CallBack().(func(response common.CallbackResponse)); ok {
-			//result.Err = di.client.AsyncCall(NewRequest(url.Location, url, inv.MethodName(), inv.Arguments(), inv.Attachments()), callBack, response)
-			result.Err = di.client.AsyncRequest(&invocation, url, timeout, callBack, rest)
-		} else {
-			result.Err = di.client.Send(&invocation, url, timeout)
-		}
+	client := di.getClient()

Review comment:
       di.BaseInvoker.IsAvailable() 时，同时返回 client 是否可以减少一次再加锁的机会？

##########
File path: protocol/grpc/grpc_invoker.go
##########
@@ -83,21 +127,47 @@ func (gi *GrpcInvoker) Invoke(ctx context.Context, invocation protocol.Invocatio
 
 // IsAvailable get available status
 func (gi *GrpcInvoker) IsAvailable() bool {
-	return gi.BaseInvoker.IsAvailable() && gi.client.GetState() != connectivity.Shutdown
+	client := gi.getClient()
+	if client != nil {
+		return gi.BaseInvoker.IsAvailable() && client.GetState() != connectivity.Shutdown
+	}
+
+	return false
 }
 
 // IsDestroyed get destroyed status
 func (gi *GrpcInvoker) IsDestroyed() bool {
-	return gi.BaseInvoker.IsDestroyed() && gi.client.GetState() == connectivity.Shutdown
+	client := gi.getClient()
+	if client != nil {
+		return gi.BaseInvoker.IsDestroyed() && client.GetState() == connectivity.Shutdown
+	}
+
+	return false
 }
 
 // Destroy will destroy gRPC's invoker and client, so it is only called once
 func (gi *GrpcInvoker) Destroy() {
 	gi.quitOnce.Do(func() {
-		gi.BaseInvoker.Destroy()
-
-		if gi.client != nil {
-			_ = gi.client.Close()
+		gi.BaseInvoker.Stop()
+		var times int64
+		for {
+			times = gi.BaseInvoker.InvokeTimes()
+			if times == 0 {

Review comment:
       同上




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r574218028



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,31 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {
 		// Generally, the case will not happen, because the invoker has been removed
 		// from the invoker list before destroy,so no new request will enter the destroyed invoker
 		logger.Warnf("this dubboInvoker is destroyed")
-		result.Err = ErrDestroyedInvoker
+		result.Err = protocol.ErrDestroyedInvoker
+		return &result
+	}
+
+	di.clientGuard.RLock()
+	defer di.clientGuard.RUnlock()
+
+	client := di.client

Review comment:
       can agree with u more.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] gaoxinge commented on pull request #1045: Fix: DubboInvoker.reqnum should be atomic

Posted by GitBox <gi...@apache.org>.
gaoxinge commented on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-774380816


   I think some specific interleaving execution of `Invoke` and `Destroy` may lead to panic:
   
   |Invoke|Destroy|Description|
   |-------|---------|------------|
   |di.closed.Load()||return false because destroy has not been called|
   ||di.closed.Store(true)||
   ||di.reqNum.Load() == 0|return true because no reqNum has been added in invoke|
   ||di.reqNum.Add(-1)||
   ||di.client = nil|set di.client to nil|
   |di.reqNum.Add(1)|||
   |di.client.Request||may panic because di.client is nil||
   |di.reqNum.Add(-1)|||


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573768781



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,28 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {
 		// Generally, the case will not happen, because the invoker has been removed
 		// from the invoker list before destroy,so no new request will enter the destroyed invoker
 		logger.Warnf("this dubboInvoker is destroyed")
-		result.Err = ErrDestroyedInvoker
+		result.Err = protocol.ErrDestroyedInvoker
+		return &result
+	}
+
+	client := di.getClient()

Review comment:
       done




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573643072



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,28 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {
 		// Generally, the case will not happen, because the invoker has been removed
 		// from the invoker list before destroy,so no new request will enter the destroyed invoker
 		logger.Warnf("this dubboInvoker is destroyed")
-		result.Err = ErrDestroyedInvoker
+		result.Err = protocol.ErrDestroyedInvoker
+		return &result
+	}
+
+	client := di.getClient()

Review comment:
       力度不大的，destroy本身就只会调用一次，invoke的时候拿的是读锁，destroy的时候才会发生互斥。所以你拿出来不拿出来都是在destroy那边阻塞一下




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] flycash commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
flycash commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573584025



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -101,8 +101,12 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		return &result
 	}
 
-	di.AddInvokerTimes(1)
-	defer di.AddInvokerTimes(-1)
+	client := di.getClient()
+	if client == nil {
+		result.Err = protocol.ErrClientClosed
+		logger.Debugf("result.Err: %v", result.Err)

Review comment:
       How about using Warn? I think this only happens in some abnormal cases.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573536437



##########
File path: protocol/grpc/grpc_invoker.go
##########
@@ -83,21 +127,47 @@ func (gi *GrpcInvoker) Invoke(ctx context.Context, invocation protocol.Invocatio
 
 // IsAvailable get available status
 func (gi *GrpcInvoker) IsAvailable() bool {
-	return gi.BaseInvoker.IsAvailable() && gi.client.GetState() != connectivity.Shutdown
+	client := gi.getClient()
+	if client != nil {
+		return gi.BaseInvoker.IsAvailable() && client.GetState() != connectivity.Shutdown
+	}
+
+	return false
 }
 
 // IsDestroyed get destroyed status
 func (gi *GrpcInvoker) IsDestroyed() bool {
-	return gi.BaseInvoker.IsDestroyed() && gi.client.GetState() == connectivity.Shutdown
+	client := gi.getClient()
+	if client != nil {
+		return gi.BaseInvoker.IsDestroyed() && client.GetState() == connectivity.Shutdown
+	}
+
+	return false
 }
 
 // Destroy will destroy gRPC's invoker and client, so it is only called once
 func (gi *GrpcInvoker) Destroy() {
 	gi.quitOnce.Do(func() {
-		gi.BaseInvoker.Destroy()
-
-		if gi.client != nil {
-			_ = gi.client.Close()
+		gi.BaseInvoker.Stop()
+		var times int64
+		for {
+			times = gi.BaseInvoker.InvokeTimes()
+			if times == 0 {
+				gi.BaseInvoker.AddInvokerTimes(-1)
+				logger.Infof("grpcInvoker is destroyed, url:{%s}", gi.GetUrl().Key())
+				gi.BaseInvoker.Destroy()
+				client := gi.getClient()
+				if client != nil {
+					client.Close()
+					gi.setClient(nil)

Review comment:
       have fixed it. pls check again.




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573531850



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,24 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {

Review comment:
       yes. 




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r571593524



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -123,18 +138,22 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 	//response := NewResponse(inv.Reply(), nil)
 	rest := &protocol.RPCResult{}
 	timeout := di.getTimeout(inv)
-	if async {
-		if callBack, ok := inv.CallBack().(func(response common.CallbackResponse)); ok {
-			//result.Err = di.client.AsyncCall(NewRequest(url.Location, url, inv.MethodName(), inv.Arguments(), inv.Attachments()), callBack, response)
-			result.Err = di.client.AsyncRequest(&invocation, url, timeout, callBack, rest)
-		} else {
-			result.Err = di.client.Send(&invocation, url, timeout)
-		}
+	client := di.getClient()

Review comment:
       getClient返回的是client指针，下面关闭的时候，同样会把这个client赋值为nil




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573621048



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,28 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {
 		// Generally, the case will not happen, because the invoker has been removed
 		// from the invoker list before destroy,so no new request will enter the destroyed invoker
 		logger.Warnf("this dubboInvoker is destroyed")
-		result.Err = ErrDestroyedInvoker
+		result.Err = protocol.ErrDestroyedInvoker
+		return &result
+	}
+
+	client := di.getClient()

Review comment:
       关键是，这样锁粒度太大了。可能还是原来的方案好点，顶多在 destroy 里面阻塞下。你再想想




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-774815258


   这边可以用给一个原子操作代表invoker是不是可用状态，然后加一个读写锁，invoke的时候先用原子操作判断是不是可用的，如果可用加读锁，然后再通过原子操作判断一次，如果还可用，调用下面的call,完成后释放读锁。destroy的时候直接把invoker标记为不可用状态，然后阻塞获取写锁，获取成功后然后关闭invoker，释放client就可以了。


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] wenxuwan commented on a change in pull request #1045: Imp: destroy invoker smoothly

Posted by GitBox <gi...@apache.org>.
wenxuwan commented on a change in pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#discussion_r573839340



##########
File path: protocol/dubbo/dubbo_invoker.go
##########
@@ -87,15 +93,31 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati
 		err    error
 		result protocol.RPCResult
 	)
-	if atomic.LoadInt64(&di.reqNum) < 0 {
+	if !di.BaseInvoker.IsAvailable() {
 		// Generally, the case will not happen, because the invoker has been removed
 		// from the invoker list before destroy,so no new request will enter the destroyed invoker
 		logger.Warnf("this dubboInvoker is destroyed")
-		result.Err = ErrDestroyedInvoker
+		result.Err = protocol.ErrDestroyedInvoker
+		return &result
+	}
+
+	di.clientGuard.RLock()
+	defer di.clientGuard.RUnlock()
+
+	client := di.client
+	if client == nil {
+		result.Err = protocol.ErrClientClosed

Review comment:
       may be client should never be nil with di.clientGuard.RLock() ?




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] gaoxinge edited a comment on pull request #1045: Fix: DubboInvoker.reqnum should be atomic

Posted by GitBox <gi...@apache.org>.
gaoxinge edited a comment on pull request #1045:
URL: https://github.com/apache/dubbo-go/pull/1045#issuecomment-774380816


   I think some specific interleaving execution of `Invoke` and `Destroy` may lead to panic:
   
   |Invoke|Destroy|Description|
   |-------|---------|------------|
   |di.closed.Load()||return false because destroy has not been called|
   ||di.closed.Store(true)||
   ||di.reqNum.Load() == 0|return true because no reqNum has been added in invoke|
   ||di.reqNum.Add(-1)||
   ||di.client = nil|set di.client to nil|
   |di.reqNum.Add(1)|||
   |di.client.Request||may panic because di.client is nil||
   |di.reqNum.Add(-1)|||


----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org