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

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