Why use static analysis? Exploring an error from Akka.NET

Apr 25 2022

Author: Sergey Vasiliev

"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?

Use a static analyzer. You can download one here. Remember to use the pvs_akka promo code — then, instead of 7 days, the trial license will work 30 days.
If you want to read more articles and notes like this one, follow me on Twitter.

#CSharp