Intro
Every developer knows that good code quality is not granted. It comes by way of an iterative process of continuous improvement. We know how innocent changes can subtly introduce bugs in another area - a developer changes something in one module and breaks something in another.
It’s essential to have some static ananlysis tool in the developer’s toolbelt. Such a tool can easily save you hours and days of debugging and prevent production failures. And you can use any tools you like, as long as you are paying attention to its warnings and hints.
In this post, I’m going to use PVS-Studio to analyze “Microsoft Orleans”. If you are not aware what PVS-Studio is - let me grab a snippet from official website >
PVS-Studio 6.0 performs static code analysis and generates a report that helps a programmer find and fix bugs. PVS-Studio does a wide range of code checks, but it is especially useful to search for misprints and Copy-Paste errors.
Recently, the company released version 6.0 with initial C# support. I was waiting for this release for a while, so I’ve contacted the team behind the tool and asked for a trial license to test and write an article.
As you may know - PVS-Studio team checks open-source projects from time to time, to show the strengths of their product. They were doing their own check of Microsoft Orleans project, when I contacted them. Team let me to do the check and kindly shared their traditional “unicorn-branded” image for the article.
### Microsoft Orleans, virtual actors and … profit :).
Microsoft Orleans is a framework based on the concept of virtual actors. Orleans terminology is different from other actor frameworks (Akka, Service Fabric) - Actor objects are Grains and Server\Host instances, who forms a single Cluster, are Silos.
A huge benefit of using virtual actors is that at any moment your code can obtain, from Orleans runtime, a proxy class for a specific grain with known Id and interface and invoke any method on it. The invocation will send a message to your cluster, which will be delivered to a grain with a given Id. Runtime guarantees that any grain with a given Id will be created only once and all messages will be delivered to that instance. Runtime does automatic activation and deactivation of grains based on grain-related activity. If the silo which hosts your grain goes offline - cluster automatically balances your grain to other Silo and redirect grain messages there. Your code won’t be affected except a minor call delay.
Also, runtime gives you guarantees that code on the grain will be executed in single-threaded mode, so no other calls can come in between method operators, and you don’t need to complicate method with any multi-threading fences and locks.
The “profit” of such frameworks is that they are very easy to scale - product takes many heavy duties off of developers’ shoulders and allows them to focus on writing business-valuable code. You don’t need to know any special syntax - simply write traditional OOP-style code and let runtime care for the rest.
Microsoft Orleans has plenty of other great features, like pluggable storage providers, explicit messaging via streams, pub-sub and others valuable additions for developing cloud- or on-prem-based highly scalable solutions. You can read more about the project here - Microsoft Orleans@GitHub.
And it is worth to note, that Orleans is used in Halo 4 and Halo 5 backends, so it’s literally production battle-tested ;)
PVS-Studio goes Orleans, Analysis results
Microsoft Orleans is being developed very rapidly, and it’s obvious that a one-off analysis won’t make any significant quality shift in a long run, it just allows us to highlight some of the current issues.
I’ve checked latest source-code after PR#1288 was merged. PVS-studio shows
- 18 High-severity warnings
- 7 Medium-severity warnings
- 58 Low-severity warnings.
Also it worth to mention the split of warnings between main runtime code and unit\integration tests projects:
| Priority | Total | Runtime | Tests |
|---|---|---|---|
| High | 18 | 12 * | 6 |
| Medium | 7 | 4 | 3 |
| Low | 58 | 13 | 45 |
* - 4 warnings were in a single method and other 3 were in old and least used code.
Okay, let’s drill down and see if PVS-Studio found something dangerous.
High-severity warnings
The most significant finding of this check is this pretty serious bug in a key sanitization method. As was discovered later team were scratching their heads about another problem which roots from this bug. And were fully convinced that “sanitization works fine”.
PVS-Studio gave this message
**V3010 The return value of function ‘Replace’ is required to be utilized. AzureStorageUtils.cs 278,279,280,281 **
and it was about this method:
public static string SanitizeTableProperty(string key)
{
// Remove any characters that can't be used in Azure PartitionKey or RowKey values
key.Replace('/', '_'); // Forward slash
key.Replace('\\', '_'); // Backslash
key.Replace('#', '_'); // Pound sign
key.Replace('?', '_'); // Question mark
if (key.Length >= 1024)
throw new ArgumentException(string.Format("Key length {0} is too long to be an Azure table key. Key={1}", key.Length, key));
return key;
}
Oops, classical snake oil - sanitization is there, but the key passed into the method is returned unchanged.
Strings are immutable in .NET and calling Replace like this doesn’t change the original string.
Another finding -
V3006 The object was created but it is not being used. The 'throw’ keyword could be missing: throw new InconsistentStateException(FOO). MemoryStorageGrain.cs 129
if (currentETag != null)
{
string error = string.Format("Etag mismatch during {0} for grain {1}: Expected = {2} Received = null", operation, grainStoreKey, currentETag.ToString());
logger.Warn(0, error);
new InconsistentStateException(error);
}
Creating new InconsistentStateException without throw keyword. The code below this section has a
properly thrown exception, so this can be considered as a missed bug.
Next warning that worth to pay attention to (false alarm in our case):
V3022 Expression 'USE_DEBUG_CONTEXT_PARAMS && arguments != null && arguments.Length > 0’ is always false. GrainReference.cs 480*
[NonSerialized] private const bool USE_DEBUG_CONTEXT_PARAMS = false;
...
if (USE_DEBUG_CONTEXT_PARAMS && arguments != null && arguments.Length > 0)
{
...
}
The statement is always false and can be optimized and removed by .NET compiler. It’s fine in our case as we have some debugging statements there but such behaviour may be damaging if some important code will be “optimised”
The following warning was found in rarely executed code, so it didn’t cause noticeable problems, but could:
V3025 Incorrect format. A different number of format items is expected while calling 'Format’ function. Expected: 3. Present: 2. Program.cs 169,183
WriteStatus(string.Format("**Calling DeleteGrain({0}, {1}, {2})", silo, grainId));
WriteStatus(string.Format("**Calling LookupGrain({0}, {1}, {2})", silo, grainId));
It may look like everything is fine here and failing to write logs is OK. But actually this code will throw an exception:
FormatException: Index (zero based) must be greater than or equal to zero and less than the size of the argument list.
Formatting wars. They were for a good cause. Re-phrasing what warning says - “this code is hard to read and prone to potential errors”
V3033 It is possible that this 'else’ branch must apply to the previous 'if’ statement. Interner.cs 251
private void InternCacheCleanupTimerCallback(object state)
{
...
long numRemoved = numEntries - internCache.Count;
if (numRemoved>0)
if (logger.IsVerbose) logger.Verbose(ErrorCode.Runtime_Error_100296, "Removed {0} / {1} unused {2} entries in {3}", numRemoved, numEntries, internCacheName, clock.Elapsed);
else
if (logger.IsVerbose2) logger.Verbose2(ErrorCode.Runtime_Error_100296, "Removed {0} / {1} unused {2} entries in {3}", numRemoved, numEntries, internCacheName, clock.Elapsed);
}
Such implicit and loosely formatted blocks are doors wide-open to bugs, like Apple had in their certificate verification:
Apple certificate check bug:
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail; **V3054 Potentially unsafe double-checked locking. Use volatile variable(s) or synchronization primitives to avoid this. StreamImpl.cs 142, 144**
I’ve cut some code parts for brevity
private readonly object initLock; // need the lock since the same code runs in the provider on the
...
internal IInternalAsyncObservable<t> GetConsumerInterface()
{
// Canonical double-checked locking
if (consumerInterface == null)
{
lock (initLock)
{
if (consumerInterface == null)
{
if (provider == null)
provider = GetStreamProvider();
consumerInterface = provider.GetConsumerInterface<t>(this);
}
}
}
return consumerInterface;
}
Initially, when I saw this code, I had some doubts (yes, multi-threading is hard ;) ) about if this double-locking pattern can be applied to a non-static object. But this is not a singleton pattern as I thought after the first look… This pattern implementation is correct and it may be a false positive alert in PVS-Studio.
NB: I actually tried to understand this case deeper to clarify it for myself. But digging deeper I found
“Sayonara volatile” by Joe Duffy and couple other articles and posts on this matter,
which opened my personal “can of multi-threaded worms”.
Worth to note that PVS-Studio is capable of detecting certain complex patterns, like Double-checked locking.
Next good spot - a problem that hard to catch by a million eyes, but easy by a static code analysis:
V3022 Expression 'n1 == 0 && n1 != 0’ is always false. Unsigned type value is always >= 0. Probably the ’||’ operator should be used here. UniqueKey.cs 113
private static UniqueKey NewKey(ulong n0, ulong n1, Category category, long typeData, string keyExt)
{
// in the string representation of a key, we grab the least significant half of n1.
// therefore, if n0 is non-zero and n1 is 0, then the string representation will always be
// 0x0 and not useful for identification of the grain.
if (n1 == 0 && n1 != 0)
throw new ArgumentException("n0 cannot be zero unless n1 is non-zero.", "n0");
Comment is describing one set of checks and conditions and if statement has another.
And again - every time you’re following a certain code style - God saves a kitten.
Compare how obvious this problem with clearly named variables.
if (n1 == 0 && n1 != 0)
vs
if (secondHalf == 0 && secondHalf != 0)
And this warning appears pretty often in logs - methods were copy-pasted, renamed but old logic stays.
V3013 It is odd that the body of 'IncrementMetric’ function is fully equivalent to the body of 'DecrementMetric’ function (1079, line 1095). TraceLogger.cs 1079
Spaced Copy-Paste, actually there is one properly implemented copy pasted method
public override void IncrementMetric(string name, double value)
{
foreach (var tc in TelemetryConsumers.OfType<imetrictelemetryconsumer>())
{
tc.IncrementMetric(name, value);
}
}
...
public override void DecrementMetric(string name, double value)
{
foreach (var tc in TelemetryConsumers.OfType<imetrictelemetryconsumer>())
{
tc.IncrementMetric(name, value); // This Decrement method increments...
}
}
The unreliable code in unit tests may be annoying. If test randomly halts once per 10 or 100 runs - it feels like a Heisenbug - unexpected and very hard to debug. This doesn’t mean that we have one here, but if you can - write your tests as your main code - no dangerous expressions allowed.
V3032 Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. LoggerTest.cs 468
// Wait until the BulkMessageInterval time interval expires before wring the final log message - should cause bulk message flush
while (stopwatch.Elapsed **V3051 An excessive type check. The object is already of the 'Exception' type. PersistenceGrainTests.cs 178**
catch (AggregateException ae)
{
exceptionThrown = true;
Exception e = ae.GetBaseException();
->> if (e is Exception)
{
// Expected error
}
else
{
throw e; // this will never happens
}
}
throw e; will never be executed, as if will be always true - any DerivedException is Exception
Is this critical? Yes and no, it depends on the context - in some components, it may be minor but critical in others.
Low-severity warnings
Relatively many minor warnings PVS-Studio produced for the unit testing code. Checking all of them I can say - there is no critical issues there, mostly - some complex testing logic to trigger edge case scenarios, like this one
V3008 The 'promise’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 91, 84. TestInternalGrains ErrorGrain.cs 91
// the grain method returns but leaves some unobserved promise
Task<long> promise = Task<long>.Factory.StartNew(() =>
{
if (!doThrow)
return 0;
logger.Info("About to throw 1.");
throw new ArgumentException("ErrorGrain left Immideate Unobserved Error 1.");
});
-->>promise = null;
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
GC.WaitForPendingFinalizers();
return Task.FromResult(11);
This code is trying to verify the behaviour of garbage-collected Task. Which may be a sign of a problem, but this is intentionally done this way in tests.
Or another: Initialize and Cleanup methods have same code
V3013 It is odd that the body of 'TestInitialize’ function is fully equivalent to the body of 'TestCleanup’ function (44, line 52). ConfigTests.cs 44
[TestInitialize]
public void TestInitialize()
{
TraceLogger.UnInitialize();
GrainClient.Uninitialize();
GrainClient.TestOnlyNoConnect = false;
}
[TestCleanup]
public void TestCleanup()
{
TraceLogger.UnInitialize();
GrainClient.Uninitialize();
GrainClient.TestOnlyNoConnect = false;
}
And another error-prone code formatting
V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. RequestContextTest.cs 87,97,111,155,181
if(msg.RequestContextData != null) foreach (var kvp in msg.RequestContextData)
{
headers.Add(kvp.Key, kvp.Value);
};
Practical outcome
Out of all findings reported to Orleans bug tracker, 5 were marked as bugs. One with Replace is epic ;)
Pretty good catch for barely 20 mins of automatic code analysis.
Why Code analysis is not a favour of the month.
Microsoft Orleans has very high velocity, you can see the pulse and commits frequency on GitHub - Orleans-contributors. Over the course of writing this article I’ve checked the project few times, saving analysis logs. The table below shows that even in a very professional team of developers “bugs happen…”
| Priority | 28 Jan | 2 Feb | 4 Feb v6.01 | Comments |
|---|---|---|---|---|
| High | 19 | 21 | 18 | 2 days and -3 critical warnings, which is a good sign, but 2 of these criticals were V3025 (String.Format args) so they must be cleaned before sending PR. |
| Medium | 4 | 4 | 7 | +3 medium severity - may be fresh bugs. |
| Low | 52 | 46 | 58 | +12 inaccurate code areas |
As you can see, even within the short timespan, new bugs rise and fall. Having a tool for quickly bringing some of them into the spotlight is always a good thing. And every static analysis can help. Whatever you like - Resharper, PVS-Studio, FxCop or any other code analysis - use it, and use it regularly. It’ll save your precious time you otherwise may be spending in the debugger or chewing through megabytes of trace logs. If you aren’t using one - you’re doing dev wrong.
On the convenience of modern tools - they all pretty much on the same page - “It just works”.
PVS-Studio has nice and noticeable context menu entry when right-clicking in Solution Explorer.
Also, you can run it from Visual Studio “quick launch” menu - Ctrl+Q and start typing 'PVS’.
It’d be good to see some kind of nuget that enables dev to type something like PVS-Solution
or PVS-Project and, later on, add this command to your CI build script. But
current integration works well for most typical usage scenarios.
Conclusions
Modern realities set pretty high expectations for the code quality and velocity. Everyone is running fast just to stay on the same level in the industry. No surprise that even strong teams and senior developers can miss some tricky or, sometimes, obvious bugs. Comprehensive test suit helps with staying on track. But someone needs to build one first, not talking about the effort to maintain such suite in up-to-date state. Thus any “helping hand” you can get, is valuable, doesn’t matter if it’s a paid tool or some free alternative.
Static analysers is just one of them, whether it’s Visual Studio built-in one, PVS-Studio or Resharper. They don’t really compete with each other but complement one another to make the best out of developer time and energy. Even such simple thing like enabling “Treat Warnings as Errors” may save you from couple of shots in the foot.
Try many, choose few, use regularly as toothbrush ;)
Thanks for reading that far. Happy and bug-free programming!
PS: Author wants to thank everyone who’s helped to proof-read this article and PVS-Studio team for a trial license offered for this testing.
