Correct self-comparison in ShardCoordinator ResendShardHost handler#8050
Correct self-comparison in ShardCoordinator ResendShardHost handler#8050MattKotsenas wants to merge 5 commits intoakkadotnet:devfrom
Conversation
region.Equals(region) compared the variable to itself, making the condition always true and the else branch (shard reallocated to another region) unreachable dead code. Changed to region.Equals(m.Region) to compare the current region from state against the original region captured when the ResendShardHost timer was scheduled.
| case ResendShardHost m: | ||
| { | ||
| if (State.Shards.TryGetValue(m.Shard, out var region) && region.Equals(region)) | ||
| if (State.Shards.TryGetValue(m.Shard, out var region) && region.Equals(m.Region)) |
There was a problem hiding this comment.
Woof - yeah this is a real bug alright. It's probably really rare in practice since the surface area for when this could cause a problem is in the middle of shard rebalancing during the timeout window, but you could potentially end up in a situation like #6973 - we know from affected users that the primary source of that issue was actually https://petabridge.com/blog/worst-dotnet-bug/ (ClusterSingleton split brain) but this is another route by which that same problem could be achieved. Nice catch.
Failed: Timeout 00:00:03 while waiting for a message of type System.String unrelated racy spec. I'll file an issue for that an attempt a fix. |
2 participants
Changes
I'm new to contributing to Akka.NET, and as I was reading / reviewing code, I noticed that
region.Equals(region)compared the variable to itself, making the condition always true and the else branch (which is empty) unreachable. Changed to region.Equals(m.Region) to compare the current region from state against the original region captured when the ResendShardHost timer was scheduled.I believe that this is wasteful but has no user visible impact. However, I spent enough time looking at it that I thought it was worth fixing just to prevent future confusion.
If there's anything I did incorrect, I apologize in advance, please let me know. Thanks!
Checklist