-
Notifications
You must be signed in to change notification settings - Fork 2k
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 leaking timed-out callbacks in InsideRuntimeClient #9041
Conversation
@dotnet-policy-service agree |
Just to note: I'm not quite happy with adding the |
@@ -335,7 +335,11 @@ public void ReceiveResponse(Message response) | |||
|
|||
private void UnregisterCallback(CorrelationId id) | |||
{ | |||
callbacks.TryRemove(id, out _); | |||
if (!callbacks.TryRemove(id, out _)) |
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 not an error - a callback might timeout and receive a response concurrently. It is a natural race
It's ok, I'll just put a note there saying that it's for testing purposes only. We have a pattern of exposing test hooks via a test-specific interface. That would be an alternative approach, but I don't feel it's necesarry here. |
118d087
to
b51b0d4
Compare
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.
I pushed my comments in the form of changes. Thank you for finding & fixing this!
Callbacks for timed-out request messages in
InsideRuntimeClient
were never removed because they were unregistered bymsg.TargetGrain
, instead ofmsg.SendingGrain
.Microsoft Reviewers: Open in CodeFlow