Strange code fragments

[rip-mm]

Describe the bug

Hello! I checked Garnet with the PVS-Studio static analyzer. I was wondering If you could take a look at several suspicious fragments:
Issue 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];
}

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.
Link to the method.

Issue 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();                 // <=
    }
  }
}

The analyzer has detected a case when an object is used after checking for null. It looks weird. I think this is an impossible case, and the ?. operator is redundant.
Link to the method.

Issue 3

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

  InitClients(clusterConfig?.Nodes.ToArray());         // <=
  Thread[] workers = InitializeThreadWorkers();
}
....
private void InitClients(ClusterNode[] nodes)
{
  switch (opts.Client)
  {
    case ClientType.GarnetClient:
      gclient = new GarnetClient[nodes.Length];   // <=
      ....
    ....
  }
}

The clusterConfig?.Nodes.ToArray() expression is used as an argument of the InitClients method. As a result, null may be passed to the method. As we can see, in the first case of the switch operator, the parameter is dereferenced without checking for null.
Link to the method.

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

The value obtained with the ?. operator is assigned to the opPercent and opWorkload fields. Then, in the next line, these fields are used without checking for null.
Link to the method.

Issue 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;
}

Developer 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.
Link to the method.

Issue 6

internal static bool 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);
}

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;
2. 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.
Link to the method.

Issue 7

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;
  ....
}

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.
Link to the method.

Issue 8

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 developer writes the value to lastVotedConfigEpoch twice. Most likely, the CurrentConfigEpoch property should've been used here instead.
Link to the method.

Issue 9

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

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.
Link to the method.

Steps to reproduce the bug

No response

Expected behavior

No response

Screenshots

No response

Release version

No response

IDE

No response

OS version

No response

Additional context

No response

[badrishc]

Re-opened pending addressing issue 5,6 in Tsavorite.