Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix leaking timed-out callbacks in InsideRuntimeClient #9041

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Fix leaking timed-out callbacks in InsideRuntimeClient #9041

merged 2 commits into from
Jul 8, 2024

Conversation

krasin-ga
Copy link
Contributor

@krasin-ga krasin-ga commented Jun 15, 2024 •
edited by dotnet-policy-service bot
Loading

Callbacks for timed-out request messages in InsideRuntimeClient were never removed because they were unregistered by msg.TargetGrain, instead of msg.SendingGrain.

Microsoft Reviewers: Open in CodeFlow
@krasin-ga
Copy link
Contributor Author

@dotnet-policy-service agree
@krasin-ga
Copy link
Contributor Author

Just to note: I'm not quite happy with adding the GetRunningRequestsCount method to IRuntimeClient only for testing purposes, and I'm not sure that this leak should be tested at all
ReubenBond
ReubenBond reviewed Jul 6, 2024
View reviewed changes
src/Orleans.Core/Runtime/OutsideRuntimeClient.cs Outdated
@@ -335,7 +335,11 @@ public void ReceiveResponse(Message response)

private void UnregisterCallback(CorrelationId id)
{
callbacks.TryRemove(id, out _);
if (!callbacks.TryRemove(id, out _))
Copy link
Member

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
@ReubenBond
Copy link
Member

Just to note: I'm not quite happy with adding the GetRunningRequestsCount method to IRuntimeClient only for testing purposes, and I'm not sure that this leak should be tested at all

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.
ReubenBond
ReubenBond approved these changes Jul 6, 2024
View reviewed changes
Copy link
Member

@ReubenBond ReubenBond left a 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!
@ReubenBond ReubenBond merged commit 3f93995 into dotnet:main Jul 8, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants
@krasin-ga @ReubenBond