Use static analysis regularly, not just before releases...
The earlier you find errors, the cheaper they are to fix...
You probably heard this a hundred times. Today we'll answer the "Why?" question once again. An error from the Akka.NET project will assist us.
The error
We'll start with a task. Find a defect in the code below:
protected override bool ReceiveRecover(object message)
{
switch (message)
{
case ShardId shardId:
_shards.Add(shardId);
return true;
case SnapshotOffer offer when (offer.Snapshot is
ShardCoordinator.CoordinatorState state):
_shards.UnionWith(state.Shards.Keys.Union(state.UnallocatedShards));
return true;
case SnapshotOffer offer when (offer.Snapshot is State state):
_shards.Union(state.Shards);
_writtenMarker = state.WrittenMigrationMarker;
return true;
case RecoveryCompleted _:
Log.Debug("Recovery complete. Current shards [{0}]. Written Marker {1}",
string.Join(", ", _shards),
_writtenMarker);
if (!_writtenMarker)
{
Persist(MigrationMarker.Instance, _ =>
{
Log.Debug("Written migration marker");
_writtenMarker = true;
});
}
return true;
case MigrationMarker _:
_writtenMarker = true;
return true;
}
....
}
Let's examine the code above and see what the problem is.
The _shards
variable is of type HashSet<ShardId>
. The code above calls several methods that change the state of this set.
HashSet<T>.Add
:
_shards.Add(shardId);
HashSet<T>.UnionWith
:
_shards.UnionWith(state.Shards.Keys.Union(state.UnallocatedShards));
However, one of these calls is incorrect:
_shards.Union(state.Shards);
It does not change the state of the _shards
object. Enumerable.Union
is a LINQ extension method that does not change the original collection and returns a modified collection instead. This means, the method call's result must be saved somewhere or used somehow. We do not see that in the code.
The PVS-Studio analyzer issued the following warning: V3010 The return value of function 'Union' is required to be utilized. Akka.Cluster.Sharding EventSourcedRememberEntitiesCoordinatorStore.cs 123
By the way, here's what the fixed code looks like:
_shards.UnionWith(state.Shards);
How we found the error — or talk number 101 on the benefits of static analysis
Every night our server runs static analysis for several open-source projects. These include Akka.NET. Why do we do it? This practice offers a few benefits:
- it provides an extra way to test the analyzer;
- it helps us write notes like this one and provides us with interesting examples that demonstrate the benefits of static analysis.
We wrote more about it here.
And now a few words on our case at hand — how the error appeared and how it was fixed.
April 20, 2022:
- code with an error is committed to the Akka.NET project's dev branch (a link to the specific line of code);
April 21, 2022:
- our server analyzes the code and sends me an email with warnings;
- I investigate the problem and create an issue on GitHub;
- the developers fix the error. A link to the commit.
I think that was some pretty smooth collaboration! Thanks to the developers for the prompt fix.
And now to the important question — how long would this error have existed in the code if events had taken a different turn? Here I'll leave some room for your imagination.
So what can you do right now to avoid mistakes like this one?
Top comments (0)