-
Notifications
You must be signed in to change notification settings - Fork 931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix:deal the panic when invoker destroy #358
Conversation
protocol/dubbo/dubbo_invoker.go
Outdated
@@ -47,6 +47,7 @@ var ( | |||
|
|||
// DubboInvoker ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment for this file, pls.
Codecov Report
@@ Coverage Diff @@
## develop #358 +/- ##
===========================================
- Coverage 66.74% 66.55% -0.19%
===========================================
Files 150 150
Lines 7936 7960 +24
===========================================
+ Hits 5297 5298 +1
- Misses 2137 2157 +20
- Partials 502 505 +3
Continue to review full report at Codecov.
|
@@ -110,11 +120,20 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati | |||
// Destroy ... | |||
func (di *DubboInvoker) Destroy() { | |||
di.quitOnce.Do(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
someone check this pls: Can this be async
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reqNum is used for judging if dubbo invoker is available ? Why not use isAvailable() func instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used to check whether there are any stock requests in the invoker during destroy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore this lgtm
@flycash I think maybe graceful down should consider the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In extreme cases, infinite circulation possible? Should we control the upper limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In extreme cases,there is always a request in the invoker that has not ended. If it is forcibly destroyed, it may cause a program panic. The severity of this result is greater than adding a loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix:deal the panic when invoker destroy
Merge pull request #358 from pantianying/addRlockForDubboInvoker
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: