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

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

• использует популярный протокол RESP;

• обеспечивает высокую пропускную способность и масштабируемость при большом числе клиентских подключений;

• показывает низкие задержки клиентов на виртуальных машинах с поддержкой ускоренной сети;

• является кроссплатформенным, расширяемым и современным благодаря использованию .NET;

По сообщениям разработчиков, проект появился из завершённого в 2018 году проекта FASTER. Сами Microsoft говорят, что уже используют Garnet в своей инфраструктуре.

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

Для проверки возьмём последний релиз Garnet (на момент написания статьи это версия 1.0.6) и проанализируем исходный код с помощью статического анализатора PVS-Studio версии 7.30.

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

• неиспользуемые параметры конструкторов;

• параметр метода перезаписывается до использования;

• проверка на равенство null, когда значение точно не может быть равно null;

• выражения, взятые несколько раз в скобки;

• методы с разными названиями, но одинаковой реализацией;

• методы, которые всегда возвращают одно и то же значение.

Предупреждения анализатора на подобный код мы рассматривать не будем, т.к. в рамках статьи это довольно скучно. Этот код, скорее всего, просто является неудачно написанным и не содержит реальных ошибок. В любом случае его стоит проверить и, возможно, провести рефакторинг, но это всё выходит за рамки статьи. Естественно, разработчики могут самостоятельно проверить свой проект и найти все подобные места. Для этого достаточно запросить пробный ключ.

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

• много кода написано с использованием unsafe;

• используется собственный менеджер памяти в обход GC.

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

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

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

NullReferenceException

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

Фрагмент 1

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 Possible null dereference. Consider inspecting 'sessionTransactionProcMap[id].Item1'. CustomCommandManagerSession.cs 28

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

Фрагмент 2

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 The 'streamWriter' object was used after it was verified against null. Check lines: 95, 89. FileLoggerProvider.cs 95

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

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

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

Фрагмент 3

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

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

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

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:

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

Как мы видим, в первом case оператора switch идёт разыменование параметра без проверки на null.

Фрагмент 4

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

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 The 'opPercent' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. EmbeddedPerformanceTest.cs 58

• V3105 The 'opWorkload' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. EmbeddedPerformanceTest.cs 58

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

Фрагмент 5

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 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, которая не работает".

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

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

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

Фрагмент 1

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 Expression 'status == OperationStatus.SUCCESS' is always false. LockableContext.cs 133

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

1. В then ветви status присваивается OperationStatus.CANCELED;

2. В else ветви status присваивается возвращаемое значение метода DoInternalLock. Если после этого status равен OperationStatus.SUCCESS, то выполняется continue.

Получается, что status в момент выполнения тернарного оператора всегда не равен OperationStatus.SUCCESS, и результат тернарного оператора всегда 0.

Фрагмент 2

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 The '?:' operator, regardless of its conditional expression, always returns one and the same value: TimeSpan.FromMilliseconds(10). FileLoggerProvider.cs 43

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

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

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) пропала одна из ошибок:

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

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

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

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

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

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

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. Эта проверка не имеет смысла. Она должна быть или убрана, или выглядеть по-другому. Например:

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

Заключение

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Artem Rovenskii. Garnet. Does Microsoft's cache -store also store bugs?.