# Garnet\. Does Microsoft's cache \-store also store bugs? Microsoft has introduced Garnet, an open\-source, cross\-platform, fast cache\-store project written in C\#\. Let's equip ourselves with a static analyzer to see what bugs and oddities hide in the project source code\. ![1124_Garnet/image1.png](https://import.viva64.com/docx/blog/1124_Garnet/image1.png) Garnet is a remote cache\-store from Microsoft Research\. The project offers the following key advantages: * it adopts the popular [RESP](https://redis.io/docs/latest/develop/reference/protocol-spec/) protocol; * it provides better server throughput and scalability with many client sessions; * it shows low client latency on virtual machines with accelerated networking support; * due to the use of the \.NET technology, Garnet is a cross\-platform, extensible, and modern solution\. According to the developers, the project emerged from the [FASTER](https://github.com/microsoft/FASTER) project, which was completed in 2018\. Microsoft claims that they're already using Garnet in their infrastructure\. The project is open source, so we can examine the source code up and down to find out what bugs and weird stuff are in there\. For our check, let's take the latest Garnet release \(the [1\.0\.6 version](https://github.com/microsoft/garnet/releases/tag/v1.0.6) at the time of writing the article\) and analyze the source code using the PVS\-Studio static analyzer 7\.30\. The check results gave me the impression that the project is new \(which is fine\) and seems to have been done in a hurry\. What's sparked my conclusions? The analysis results contain several warnings of the following types: * unused constructor parameters; * a method parameter is overwritten before it's used; * a check for _null_ where the value surely can't be _null_; * expressions taken in parentheses several times; * methods that have different names but the same implementation; * methods that always return the same value\. We won't go into the analyzer warnings issued for this code, because it's rather boring for the article\. The code is most likely just poorly written and contains no real errors\. Anyway, it's still worth checking and possibly refactoring, but that's beyond the scope of the article\. Of course, developers can check their project themselves and find all such fragments\. To do so, they may simply request a [trial key](https://pvs-studio.com/en/pvs-studio/try-free/)\. I'd also like to note that, based on the available information, the project is very complex\. The developers had to use a number of tricks to achieve such a performance level: * a lot of code is written using _unsafe_; * Garnet uses its own memory manager to bypass GC\. All in all, the code is a far cry from the usual style of writing C\# projects\. I feel like it would've been better to use a different language instead of applying a bunch of hacks\. Moreover, even though the project is open source, the barrier to entry is so high that the community is unlikely to develop it\. Now that we've talked about the project, let's move on to looking at the errors\. ## Errors and oddities ### NullReferenceException A potential NullReferenceException is one of the most common issues identified by static analysis\. Garnet is no exception\. It contains code that is susceptible to such an "affliction"\. We won't analyze all the warnings, as there are dozens of them, but rather pick the most obvious or interesting ones\. **Fragment 1** ```cpp public (....) GetCustomTransactionProcedure(....) { if (sessionTransactionProcMap[id].Item1 == null) { var entry = customCommandManager.transactionProcMap[id]; sessionTransactionProcMap[id].Item1 = entry.proc != null ? entry.proc() : null; sessionTransactionProcMap[id].Item2 = entry.NumParams; sessionTransactionProcMap[id].Item1.txnManager = txnManager; sessionTransactionProcMap[id].Item1.scratchBufferManager = scratchBufferManager; } return sessionTransactionProcMap[id]; } ``` The PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'sessionTransactionProcMap\[id\]\.Item1'\. CustomCommandManagerSession\.cs 28 Depending on the ternary operator condition, a null reference is assigned in the _sessionTransactionProcMap\[id\]\.Item1_ tuple field\. In addition, literally the next line later, the field is dereferenced without any check\. **Fragment 2** ```cpp public void Log(....) { var msg = string.Format("[{0:D3}.{1}] ({2}) <{3}> {4}", ....); lock (this.lockObj) { streamWriter?.WriteLine(msg); // <= //var now = DateTime.UtcNow; //if(now - lastFlush > flushInterval) { //lastFlush = now; streamWriter.Flush(); // <= } } } ``` The PVS\-Studio warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'streamWriter' object was used after it was verified against null\. Check lines: 95, 89\. FileLoggerProvider\.cs 95 The analyzer has detected a case when an object is used after checking for _null_\. If we remove the commented lines, the code looks like this: ```cpp .... streamWriter?.WriteLine(msg); streamWriter.Flush(); .... ``` It looks weird\. I think this is an impossible case, and the '?\.' operator is redundant\. So, there's no error here\. However, the analyzer still got it right\. It accurately warns us of an anomaly in the code\. It's either necessary to write '?\.' everywhere \(if there's a possible scenario where the reference is null\) or just write '\.' to make the code simpler and more consistent\. Also, other programmers wouldn't have to think over such code when modifying or refactoring it\. **Fragment 3** ```cpp public void Run() { PrintClusterConfig(); Console.WriteLine($"Running benchmark using {opts.Client} client type"); InitClients(clusterConfig?.Nodes.ToArray()); // <= Thread[] workers = InitializeThreadWorkers(); } ``` The PVS\-Studio warning: [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The result of null\-conditional operator is dereferenced inside the 'InitClients' method\. NullReferenceException is possible\. Inspect the first argument 'clusterConfig?\.Nodes\.ToArray\(\)'\. ShardedRespOnlineBench\.cs 414 The _clusterConfig?\.Nodes\.ToArray\(\)_ expression is used as an argument of the _InitClients_ method\. As a result, _null_ may be passed to the method\. Let's look at the code of the _InitClients_ method: ```cpp private void InitClients(ClusterNode[] nodes) { switch (opts.Client) { case ClientType.GarnetClient: gclient = new GarnetClient[nodes.Length]; // <= .... .... } } ``` As we can see, in the first _case_ of the _switch_ operator, the parameter is dereferenced without checking for _null_\. **Fragment 4** Usually, we don't include errors found in the code of tests in our articles, but this time I decided to pick some of them\. The thing is that, according to the comments, the code triggers the built\-in stress test: ```cpp public EmbeddedPerformanceTest(...., Options opts, ....) { this.server = server; this.opts = opts; logger = loggerFactory.CreateLogger("EmbeddedBench"); opPercent = opts.OpPercent?.ToArray(); // <= opWorkload = opts.OpWorkload?.ToArray(); // <= if (opPercent.Length != opWorkload.Length) // <= throw new Exception($"...."); .... } ``` The PVS\-Studio warnings: * [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The 'opPercent' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. EmbeddedPerformanceTest\.cs 58 * [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The 'opWorkload' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. EmbeddedPerformanceTest\.cs 58 The value obtained with the '?\.' operator is assigned to the _opPercent_ and _opWorkload_ fields again\. Then, in the next line, these fields are used without checking for _null_\. **Fragment 5** ```cpp public IEnumerable GetPendingRequests() { foreach (var kvp in ctx.prevCtx?.ioPendingRequests) yield return kvp.Value.serialNum; foreach (var kvp in ctx.ioPendingRequests) yield return kvp.Value.serialNum; } ``` The PVS\-Studio warning: [V3153](https://pvs-studio.com/en/docs/warnings/v3153/) Enumerating the result of null\-conditional access operator can lead to NullReferenceException\. Consider inspecting: ctx\.prevCtx?\.ioPendingRequests\. ClientSession\.cs 748 Developers used the '?\.' operator in this code fragment\. Apparently, the programmer implied that _prevCtx_ could have the _null_ value\. However, the thing is that _foreach_ doesn't work with _null_\. So, the exception still occurs, but when the _GetEnumerator_ method is called\. As we've written many times in other articles, this isn't obvious to many developers\. We have a whole article on this topic: "[The ?\. operator in foreach will not protect from NullReferenceException](https://pvs-studio.com/en/blog/posts/csharp/0832/)"\. ![1124_Garnet/image2.png](https://import.viva64.com/docx/blog/1124_Garnet/image2.png) Such errors aren't critical in C\# and are quickly detected if the reference becomes null \(unlike in C\+\+\)\. Perhaps a null reference is rare or impossible in such code, so this code contains such errors for a long time and in large numbers\. However, it causes problems over time\. A program crash due to a _NullReferenceException_ can turn into a frustrating and time\-consuming search for the issue\. This is especially true if the error occurs only on the client's side, and it's very difficult or even impossible to reproduce the error\. ### Ternary operator issues I don't know why, but a lot of erroneous code in this project involves the ternary operator\. Another error related to it was in the _NullReferenceException_ section\. Let's look at some examples\. **Fragment 1** ```cpp internal static void DoInternalLock<....>(....) { OperationStatus status; if (cancellationToken.IsCancellationRequested) status = OperationStatus.CANCELED; else { status = DoInternalLock(clientSession, key); if (status == OperationStatus.SUCCESS) continue; // Success; continue to the next key. } var unlockIdx = keyIdx - (status == OperationStatus.SUCCESS ? 0 : 1); } ``` The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'status \=\= OperationStatus\.SUCCESS' is always false\. LockableContext\.cs 133 In such a case, the analyzer shows that the condition of the ternary operator is always _false_\. The _status_ local variable is assigned a value within _if_\. Let's look at two possible scenarios: 1. In the _then_ branch, _OperationStatus\.CANCELED _is assigned to_ status_; 1. In the _else_ branch, the return value of the _DoInternalLock_ method is assigned to_ status_\. If after that _status_ equals to _OperationStatus\.SUCCESS_, then _continue_ is executed\. So, at the moment of the ternary operator execution, _status_ is never equal to _OperationStatus\.SUCCESS_, and the result of the ternary operator is always 0\. **Fragment 2** ```cpp public class FileLoggerOutput : IDisposable { private StreamWriter streamWriter; private readonly object lockObj = new object(); private readonly TimeSpan flushInterval = Debugger.IsAttached ? TimeSpan.FromMilliseconds(10) : TimeSpan.FromMilliseconds(10); private DateTime lastFlush = DateTime.UtcNow; .... } ``` The PVS\-Studio warning: [V3012](https://pvs-studio.com/en/docs/warnings/v3012/) The '?:' operator, regardless of its conditional expression, always returns one and the same value: TimeSpan\.FromMilliseconds\(10\)\. FileLoggerProvider\.cs 43 Developers use the ternary operator to assign different values to the _flushInterval_ field depending on whether the debugger is connected to the process or not\. Because of the error, the value is the same regardless of the condition\. You can find the following entry in one of the class constructors: ```cpp public FileLoggerOutput(string filename, int flushInterval = default) { this.flushInterval = flushInterval == default ? Debugger.IsAttached ? TimeSpan.FromMilliseconds(10) : TimeSpan.FromSeconds(1) : TimeSpan.FromMilliseconds(flushInterval); .... } ``` Most likely, this is the proper use of the ternary operator\. **Fragment 3 \(fixed\)** Initially, I checked Garnet v1\.0\.5\. In the new version \(1\.0\.6\), one of the errors has disappeared: ```cpp private async Task SendFailoverAuthReq(long requestedEpoch) { int firstClientIndex = option == FailoverOption.FORCE ? 1 : 0; .... int majority = (clients.Length - firstClientIndex / 2) + 1; .... } ``` The PVS\-Studio warning: [V3157](https://pvs-studio.com/en/docs/warnings/v3157/) The 'firstClientIndex / 2' expression evaluates to 0 because absolute value of the left operand is less than the right operand\. ReplicaFailoverSession\.cs 85 In this case, the analyzer understands that the _firstClientIndex_ local variable is assigned 1 or 0\. Both numbers are less than 2, so the result of the division is always 0\. Now this PVS\-Studio warning is gone, as well as the method :\) The code fragment still managed to get into one of the releases\. Yes, the developers fixed it, but only after the users got it\. However, if the developers had used static code analysis at an earlier stage, they could've avoided this error in the release\. This shows how important it is to use such tools at the development stage to catch potential issues before they end up in the release version of the program\. ### Other errors **Fragment 1** ```cpp public ClusterConfig InitializeLocalWorker(....) { Worker[] newWorkers = new Worker[workers.Length]; Array.Copy(workers, newWorkers, workers.Length); newWorkers[1].address = address; newWorkers[1].port = port; newWorkers[1].nodeid = nodeId; newWorkers[1].configEpoch = configEpoch; newWorkers[1].lastVotedConfigEpoch = currentConfigEpoch; // <= newWorkers[1].lastVotedConfigEpoch = lastVotedConfigEpoch; // <= newWorkers[1].role = role; newWorkers[1].replicaOfNodeId = replicaOfNodeId; newWorkers[1].replicationOffset = 0; newWorkers[1].hostname = hostname; return new ClusterConfig(slotMap, newWorkers); } ``` The PVS\-Studio warning: [V3127](https://pvs-studio.com/en/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'currentConfigEpoch' variable should be used instead of 'lastVotedConfigEpoch' ClusterConfig\.cs 116 The copy\-paste error is a frequent guest in our articles\. Static analyzers are good at finding typos and copy\-paste errors\. In this example, the developer writes the value to _lastVotedConfigEpoch_ twice\. Most likely, the _CurrentConfigEpoch_ property should've been used here instead\. **Fragment 2** ```cpp public long GetConfigEpochFromSlot(int slot) { if (slotMap[slot].workerId < 0) return 0; return workers[slotMap[slot].workerId].configEpoch; } ``` The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'slotMap\[slot\]\.workerId < 0' is always false\. Unsigned type value is always \>\= 0\. ClusterConfig\.cs 460 Here, the analyzer shows that the _if_ statement condition is always _false_\. The thing is that the _workerId_ property is of the _ushort_ type\. The possible range is 0 to 65535\. This means that _workerId_ can't be less than 0\. The check doesn't make any sense\. It should either be deleted or look different, for example: ```cpp if (slotMap[slot].workerId == 0) return 0; ``` ## Conclusion The project gets frequent updates, and of course, bugs will be fixed eventually\. Even one of the examples shown in the article has already been fixed\. However, as I've written before, the community is skeptical of such a coding style in C\#\. It seems that C\# doesn't fit for writing most of the project code using _unsafe_ and custom memory manager\. I can only wish the project success in its development and will contribute to this by reporting all bugs found to the developers via issues on GitHub\.