# Garnet\. Хранит ли баги кэш\-хранилище от Microsoft?

Microsoft представили Garnet — проект кроссплатформенного быстрого кэш\-хранилища с открытым исходным кодом, написанный на языке C\#\. Предлагаю расчехлить статический анализатор и посмотреть, какие ошибки и странности содержатся в исходном коде проекта\.

![1124_Garnet_ru/image1.png](https://import.viva64.com/docx/blog/1124_Garnet_ru/image1.png)

Garnet — это удалённое кэш\-хранилище от Microsoft Research\. Проект обладает рядом достоинств:

* использует популярный протокол [RESP](https://redis.io/docs/latest/develop/reference/protocol-spec/);
* обеспечивает высокую пропускную способность и масштабируемость при большом числе клиентских подключений;
* показывает низкие задержки клиентов на виртуальных машинах с поддержкой ускоренной сети;
* является кроссплатформенным, расширяемым и современным благодаря использованию \.NET; 

По сообщениям разработчиков, проект появился из завершённого в 2018 году проекта [FASTER](https://github.com/microsoft/FASTER)\. Сами Microsoft говорят, что уже используют Garnet в своей инфраструктуре\.

Проект имеет открытый исходный код, а значит мы можем вдоль и поперёк изучить исходники чтобы узнать, какие ошибки и странности в них хранятся\. 

Для проверки возьмём последний релиз Garnet \(на момент написания статьи это [версия 1\.0\.6](https://github.com/microsoft/garnet/releases/tag/v1.0.6)\) и проанализируем исходный код с помощью статического анализатора PVS\-Studio версии 7\.30\.

При просмотре результатов проверки сложилось впечатление, что проект новый \(это нормально\), и делали его как\-то поспешно\. Почему я пришёл к такому выводу? В результатах анализа достаточно много предупреждений следующего вида:

* неиспользуемые параметры конструкторов;
* параметр метода перезаписывается до использования;
* проверка на равенство _null_, когда значение точно не может быть равно _null_;
* выражения, взятые несколько раз в скобки;
* методы с разными названиями, но одинаковой реализацией;
* методы, которые всегда возвращают одно и то же значение\.

Предупреждения анализатора на подобный код мы рассматривать не будем, т\.к\. в рамках статьи это довольно скучно\. Этот код, скорее всего, просто является неудачно написанным и не содержит реальных ошибок\. В любом случае его стоит проверить и, возможно, провести рефакторинг, но это всё выходит за рамки статьи\. Естественно, разработчики могут самостоятельно проверить свой проект и найти все подобные места\. Для этого достаточно запросить [пробный ключ](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. 

Также добавлю, что, судя по имеющейся информации, проект очень сложный\. Для достижения подобной скорости работы разработчикам пришлось пойти на различные ухищрения:

* много кода написано с использованием unsafe;
* используется собственный менеджер памяти в обход GC\.

В общем, код далёк от привычного стиля написания проектов на C\#\. Складывается впечатление, что вместо применения кучи хаков лучше было бы взять другой язык\. Вдобавок, хоть проект и Open Source, но порог вхождения настолько велик, что сообщество вряд ли будет его развивать\.

А теперь, когда мы поговорили о проекте, перейдём к просмотру ошибок\.

## Ошибки и странности

### NullReferenceException

Потенциальный NullReferenceException — это одна из самых частых проблем, которую выявляет статический анализ\. Garnet не стал исключением\. В нём также есть код, который подвержен этому "недугу"\. Все предупреждения разбирать не будем, т\.к\. их десятки, а возьмём самые явные или интересные\.

**Фрагмент 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];
}
```

Предупреждение PVS\-Studio: 

[V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'sessionTransactionProcMap\[id\]\.Item1'\. CustomCommandManagerSession\.cs 28

В поле кортежа _sessionTransactionProcMap\[id\]\.Item1_, в зависимости от условия тернарного оператора, присваивается нулевая ссылка\. При этом буквально через строчку поле будет разыменовано без какой\-либо проверки\.

**Фрагмент 2**

```cpp
public void Log<TState>(....)
{
  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();                 // <=
    }
  }
}
```

Предупреждение PVS\-Studio: 

[V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'streamWriter' object was used after it was verified against null\. Check lines: 95, 89\. FileLoggerProvider\.cs 95

Анализатор обнаружил ситуацию, когда объект используется после проверки на _null_\. Если же убрать закомментированные строчки, то код получится таким:

```cpp
....
streamWriter?.WriteLine(msg);
streamWriter.Flush();
....
```

Выглядит странно\. Думаю, что это просто невозможная ситуация, и оператор '?\.' тут лишний\. То есть ошибки здесь нет\. Однако анализатор всё равно прав\. Он верно предупреждает об аномалии в коде\. Надо или везде писать '?\.', если возможен сценарий, когда ссылка нулевая, или везде писать просто '\.', делая код более простым и единообразным\. Вдобавок не придётся заставлять других программистов раздумывать над подобным кодом, когда они будут его модифицировать или рефакторить\.

**Фрагмент 3**

```cpp
public void Run()
{
  PrintClusterConfig();
  Console.WriteLine($"Running benchmark using {opts.Client} client type");

  InitClients(clusterConfig?.Nodes.ToArray());         // <=
  Thread[] workers = InitializeThreadWorkers();
}
```

Предупреждение PVS\-Studio: 

[V3105](https://pvs-studio.ru/ru/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

В качестве аргумента метода _InitClients_ используется выражение _clusterConfig?\.Nodes\.ToArray\(\)_\. То есть в результате в метод может быть передан _null_\. Взглянем на код метода _InitClients_:

```cpp
private void InitClients(ClusterNode[] nodes)
{
  switch (opts.Client)
  {
    case ClientType.GarnetClient:
      gclient = new GarnetClient[nodes.Length];   // <=
      ....
    ....
  }
}
```

Как мы видим, в первом _case_ оператора _switch_ идёт разыменование параметра без проверки на _null_\.

**Фрагмент 4**

Обычно для статей мы не берём ошибки, найденные в коде тестов, но в этот раз я решил всё же использовать их\. Дело в том, что это код, судя по комментариям в нём, запускает встроенный стресс тест:

```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($"....");
  ....
}
```

Предупреждения PVS\-Studio:

* [V3105](https://pvs-studio.ru/ru/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.ru/ru/docs/warnings/v3105/) The 'opWorkload' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. EmbeddedPerformanceTest\.cs 58

И здесь полям _opPercent_ и _opWorkload_ уже привычно присваивается значение, полученное с помощью оператора '?\.'\. После чего на следующей строке эти поля используют без проверки на _null_\.

**Фрагмент 5**

```cpp
public IEnumerable<long> GetPendingRequests()
{
  foreach (var kvp in ctx.prevCtx?.ioPendingRequests)
    yield return kvp.Value.serialNum;

  foreach (var kvp in ctx.ioPendingRequests)
    yield return kvp.Value.serialNum;
}
```

Предупреждение PVS\-Studio: 

[V3153](https://pvs-studio.ru/ru/docs/warnings/v3153/) Enumerating the result of null\-conditional access operator can lead to NullReferenceException\. Consider inspecting: ctx\.prevCtx?\.ioPendingRequests\. ClientSession\.cs 748

В этом фрагменте кода используется оператор '?\.'\. Видимо, программист подразумевал, что _prevCtx_ может иметь значение _null_\. Вот только проблема в том, что _foreach_ не работает с _null_\. Получается, что исключение всё равно настигнет разработчика, но при вызове метода _GetEnumerator_\.

Как мы уже неоднократно писали в других статьях, для многих разработчиков это неочевидный момент\. На эту тему у нас есть отдельная статья: "[Использование оператора ?\. в foreach: защита от NullReferenceException, которая не работает](https://pvs-studio.ru/ru/blog/posts/csharp/0832/)"\.

![1124_Garnet_ru/image2.png](https://import.viva64.com/docx/blog/1124_Garnet_ru/image2.png)

С одной стороны, подобные ошибки в C\# некритичны и быстро обнаруживаются \(в отличие от C\+\+\), если ссылка станет нулевой\. Возможно, в этом или другом коде нулевая ссылка — это редкая или вообще невозможная ситуация, поэтому в нём подобные ошибки присутствуют подолгу и в большом количестве\. Но со временем это создаст проблемы\. Падение программы из\-за _NullReferenceException_ может обернуться неприятным и долгим поиском проблемы\. Особенно если ошибка появляется только у клиента, и воспроизвести её очень трудно или не получается вовсе\.

### Проблемы с тернарным оператором

Не знаю почему, но в этом проекте множество ошибочного кода связано с тернарным оператором\. Ещё одна ошибка с ним была в разделе _NullReferenceException_\. Рассмотрим несколько примеров\.

**Фрагмент 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);
}
```

Предупреждение PVS\-Studio: 

[V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'status \=\= OperationStatus\.SUCCESS' is always false\. LockableContext\.cs 133

В этом случае анализатор указывает, что условие тернарного оператора всегда _false_\. Локальной переменной _status_ присваивается значение внутри _if_\. Посмотрим на два возможных развития событий:

1. В then ветви _status_ присваивается _OperationStatus\.CANCELED_;
1. В else ветви _status_ присваивается возвращаемое значение метода _DoInternalLock_\. Если после этого _status_ равен _OperationStatus\.SUCCESS_, то выполняется _continue_\. 

Получается, что _status_ в момент выполнения тернарного оператора всегда не равен _OperationStatus\.SUCCESS_, и результат тернарного оператора всегда 0\.

**Фрагмент 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;
  ....
}
```

Предупреждение PVS\-Studio: 

[V3012](https://pvs-studio.ru/ru/docs/warnings/v3012/) The '?:' operator, regardless of its conditional expression, always returns one and the same value: TimeSpan\.FromMilliseconds\(10\)\. FileLoggerProvider\.cs 43

Разработчики используют тернарный оператор для того, чтобы в зависимости от того, подключён отладчик к процессу или нет, присвоить полю _flushInterval_ различные значения\. Из\-за ошибки вне зависимости от условия значение будет одно и то же\.

При этом в одном из конструкторов класса можно найти следующую запись:

```cpp
public FileLoggerOutput(string filename, int flushInterval = default)
{
  this.flushInterval = flushInterval == default ?
    Debugger.IsAttached ? TimeSpan.FromMilliseconds(10) 
                        : TimeSpan.FromSeconds(1) 
    : TimeSpan.FromMilliseconds(flushInterval);
  ....
}
```

Скорее всего, это и есть правильное использование тернарного оператора\.

**Фрагмент 3 \(исправлено\)**

Первоначально я проверял Garnet версии v1\.0\.5\. В новой версии проекта \(1\.0\.6\) пропала одна из ошибок:

```cpp
private async Task<bool> SendFailoverAuthReq(long requestedEpoch)
{
  int firstClientIndex = option == FailoverOption.FORCE ? 1 : 0;
  ....
  int majority = (clients.Length - firstClientIndex / 2) + 1;
  ....
}
```

Предупреждение PVS\-Studio: 

[V3157](https://pvs-studio.ru/ru/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

В этом случае анализатор понимает, что локальной переменной _firstClientIndex_ присваивается 1 или 0\. Оба этих числа меньше 2, а значит результат деления всегда будет равен 0\.

Теперь этого предупреждения PVS\-Studio нет, как и самого метода :\) Этот фрагмент кода всё же успел попасть в один из релизов\. Да, его исправили, но уже после попадания к пользователям\. Однако, если бы разработчики применяли статический анализ кода заранее, можно было бы избежать появления этой ошибки в релизе\. Это подчёркивает важность использования таких инструментов ещё на этапе разработки, чтобы обнаруживать потенциальные проблемы до того, как они окажутся в финальной версии программы\. 

### Прочие ошибки

**Фрагмент 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);
}
```

Предупреждение PVS\-Studio: 

[V3127](https://pvs-studio.ru/ru/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

Частый гость наших статей — ошибка copy\-paste\. Статические анализаторы традиционно сильны в поиске опечаток и ошибок copy\-paste\. В этом примере разработчик дважды записывает значение в _lastVotedConfigEpoch_\. Скорее всего, здесь следует использовать свойство _CurrentConfigEpoch_\.

**Фрагмент 2**

```cpp
public long GetConfigEpochFromSlot(int slot)
{
    if (slotMap[slot].workerId < 0)
        return 0;
    return workers[slotMap[slot].workerId].configEpoch;
}
```

Предупреждение PVS\-Studio: 

[V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'slotMap\[slot\]\.workerId < 0' is always false\. Unsigned type value is always \>\= 0\. ClusterConfig\.cs 460

Здесь анализатор говорит о том, что условие оператора _if_ всегда _false_\. Дело в том, что свойство _workerId_ имеет тип _ushort_\. Следовательно, возможный диапазон значений от 0 до 65535, т\.е\. _workerId_ точно не может быть меньше 0\. Эта проверка не имеет смысла\. Она должна быть или убрана, или выглядеть по\-другому\. Например:

```cpp
if (slotMap[slot].workerId == 0)
        return 0;
```

## Заключение

Проект часто получает обновления и, соответственно, ошибки будут исправляться\. Даже один из примеров, представленных в статье, уже исправлен\. Но, как я уже писал ранее, сообщество скептически отнеслось к подобному стилю написания на C\#\. Кажется, что C\# не для того, чтобы большую часть проекта писать с использованием unsafe и с собственным менеджером памяти\. Я же лишь могу пожелать проекту успехов в дальнейшем развитии\. И внесу в это свой вклад, сообщив обо всех найденных ошибках разработчикам через issues на GitHub\.