-
Notifications
You must be signed in to change notification settings - Fork 929
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
Changes from 2 commits
0713fc1
6da0921
c250592
7f6fd6a
0ee1cd4
167d221
873c7d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,8 @@ import ( | |
"context" | ||
"strconv" | ||
"sync" | ||
"sync/atomic" | ||
"time" | ||
) | ||
|
||
import ( | ||
|
@@ -50,18 +52,26 @@ type DubboInvoker struct { | |
protocol.BaseInvoker | ||
client *Client | ||
quitOnce sync.Once | ||
|
||
// Used to record the number of requests | ||
reqNum int64 | ||
} | ||
|
||
// NewDubboInvoker ... | ||
func NewDubboInvoker(url common.URL, client *Client) *DubboInvoker { | ||
return &DubboInvoker{ | ||
BaseInvoker: *protocol.NewBaseInvoker(url), | ||
client: client, | ||
reqNum: 0, | ||
} | ||
} | ||
|
||
// Invoke ... | ||
func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocation) protocol.Result { | ||
|
||
atomic.AddInt64(&(di.reqNum), 1) | ||
defer atomic.AddInt64(&(di.reqNum), -1) | ||
|
||
var ( | ||
err error | ||
result protocol.RPCResult | ||
|
@@ -110,11 +120,20 @@ func (di *DubboInvoker) Invoke(ctx context.Context, invocation protocol.Invocati | |
// Destroy ... | ||
func (di *DubboInvoker) Destroy() { | ||
di.quitOnce.Do(func() { | ||
di.BaseInvoker.Destroy() | ||
|
||
if di.client != nil { | ||
di.client.Close() | ||
for { | ||
if di.reqNum == 0 { | ||
logger.Info("dubboInvoker is destroyed,url:{%s}", di.GetUrl().Key()) | ||
di.BaseInvoker.Destroy() | ||
if di.client != nil { | ||
di.client.Close() | ||
di.client = nil | ||
} | ||
break | ||
} | ||
logger.Warnf("DubboInvoker is to be destroyed, wait {%v} req end,url:{%s}", di.reqNum, di.GetUrl().Key()) | ||
time.Sleep(1 * time.Second) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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.
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