Skip to content

Commit

Permalink
Fix #2576: PhysicalConnection: Better shutdown handling (#2629)
Browse files Browse the repository at this point in the history
This adds a bit of null ref handling (few ifs). Fixes #2576.

Overall, this is biting people in the shutdown race, more likely under load, so let's eat the if checks here to prevent it. I decided to go with the specific approach here as to not affect inlining.
  • Loading branch information
NickCraver committed Jan 11, 2024
1 parent ad2e69f commit b2694b3
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 15 deletions.
1 change: 1 addition & 0 deletions docs/ReleaseNotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Current package versions:

- Fix [#2619](https://github.com/StackExchange/StackExchange.Redis/issues/2619): Type-forward `IsExternalInit` to support down-level TFMs ([#2621 by mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/2621))
- `InternalsVisibleTo` `PublicKey` enhancements([#2623 by WeihanLi](https://github.com/StackExchange/StackExchange.Redis/pull/2623))
- Fix [#2576](https://github.com/StackExchange/StackExchange.Redis/issues/2576): Prevent `NullReferenceException` during shutdown of connections ([#2629 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2629))

## 2.7.10

Expand Down
54 changes: 39 additions & 15 deletions src/StackExchange.Redis/PhysicalConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -790,39 +790,44 @@ internal void Write(in RedisKey key)
var val = key.KeyValue;
if (val is string s)
{
WriteUnifiedPrefixedString(_ioPipe!.Output, key.KeyPrefix, s);
WriteUnifiedPrefixedString(_ioPipe?.Output, key.KeyPrefix, s);
}
else
{
WriteUnifiedPrefixedBlob(_ioPipe!.Output, key.KeyPrefix, (byte[]?)val);
WriteUnifiedPrefixedBlob(_ioPipe?.Output, key.KeyPrefix, (byte[]?)val);
}
}

internal void Write(in RedisChannel channel)
=> WriteUnifiedPrefixedBlob(_ioPipe!.Output, ChannelPrefix, channel.Value);
=> WriteUnifiedPrefixedBlob(_ioPipe?.Output, ChannelPrefix, channel.Value);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal void WriteBulkString(in RedisValue value)
=> WriteBulkString(value, _ioPipe!.Output);
internal static void WriteBulkString(in RedisValue value, PipeWriter output)
=> WriteBulkString(value, _ioPipe?.Output);
internal static void WriteBulkString(in RedisValue value, PipeWriter? maybeNullWriter)
{
if (maybeNullWriter is not PipeWriter writer)
{
return; // Prevent null refs during disposal
}

switch (value.Type)
{
case RedisValue.StorageType.Null:
WriteUnifiedBlob(output, (byte[]?)null);
WriteUnifiedBlob(writer, (byte[]?)null);
break;
case RedisValue.StorageType.Int64:
WriteUnifiedInt64(output, value.OverlappedValueInt64);
WriteUnifiedInt64(writer, value.OverlappedValueInt64);
break;
case RedisValue.StorageType.UInt64:
WriteUnifiedUInt64(output, value.OverlappedValueUInt64);
WriteUnifiedUInt64(writer, value.OverlappedValueUInt64);
break;
case RedisValue.StorageType.Double: // use string
case RedisValue.StorageType.String:
WriteUnifiedPrefixedString(output, null, (string?)value);
WriteUnifiedPrefixedString(writer, null, (string?)value);
break;
case RedisValue.StorageType.Raw:
WriteUnifiedSpan(output, ((ReadOnlyMemory<byte>)value).Span);
WriteUnifiedSpan(writer, ((ReadOnlyMemory<byte>)value).Span);
break;
default:
throw new InvalidOperationException($"Unexpected {value.Type} value: '{value}'");
Expand All @@ -833,6 +838,11 @@ internal static void WriteBulkString(in RedisValue value, PipeWriter output)

internal void WriteHeader(RedisCommand command, int arguments, CommandBytes commandBytes = default)
{
if (_ioPipe?.Output is not PipeWriter writer)
{
return; // Prevent null refs during disposal
}

var bridge = BridgeCouldBeNull ?? throw new ObjectDisposedException(ToString());

if (command == RedisCommand.UNKNOWN)
Expand All @@ -856,14 +866,14 @@ internal void WriteHeader(RedisCommand command, int arguments, CommandBytes comm
// *{argCount}\r\n = 3 + MaxInt32TextLen
// ${cmd-len}\r\n = 3 + MaxInt32TextLen
// {cmd}\r\n = 2 + commandBytes.Length
var span = _ioPipe!.Output.GetSpan(commandBytes.Length + 8 + Format.MaxInt32TextLen + Format.MaxInt32TextLen);
var span = writer.GetSpan(commandBytes.Length + 8 + Format.MaxInt32TextLen + Format.MaxInt32TextLen);
span[0] = (byte)'*';

int offset = WriteRaw(span, arguments + 1, offset: 1);

offset = AppendToSpanCommand(span, commandBytes, offset: offset);

_ioPipe.Output.Advance(offset);
writer.Advance(offset);
}

internal void RecordQuit() // don't blame redis if we fired the first shot
Expand Down Expand Up @@ -1116,7 +1126,11 @@ private static int AppendToSpan(Span<byte> span, ReadOnlySpan<byte> value, int o

internal void WriteSha1AsHex(byte[] value)
{
var writer = _ioPipe!.Output;
if (_ioPipe?.Output is not PipeWriter writer)
{
return; // Prevent null refs during disposal
}

if (value == null)
{
writer.Write(NullBulkString.Span);
Expand Down Expand Up @@ -1156,8 +1170,13 @@ internal static byte ToHexNibble(int value)
return value < 10 ? (byte)('0' + value) : (byte)('a' - 10 + value);
}

internal static void WriteUnifiedPrefixedString(PipeWriter writer, byte[]? prefix, string? value)
internal static void WriteUnifiedPrefixedString(PipeWriter? maybeNullWriter, byte[]? prefix, string? value)
{
if (maybeNullWriter is not PipeWriter writer)
{
return; // Prevent null refs during disposal
}

if (value == null)
{
// special case
Expand Down Expand Up @@ -1259,8 +1278,13 @@ internal static unsafe void WriteRaw(PipeWriter writer, string value, int expect
}
}

private static void WriteUnifiedPrefixedBlob(PipeWriter writer, byte[]? prefix, byte[]? value)
private static void WriteUnifiedPrefixedBlob(PipeWriter? maybeNullWriter, byte[]? prefix, byte[]? value)
{
if (maybeNullWriter is not PipeWriter writer)
{
return; // Prevent null refs during disposal
}

// ${total-len}\r\n
// {prefix}{value}\r\n
if (prefix == null || prefix.Length == 0 || value == null)
Expand Down

0 comments on commit b2694b3

Please sign in to comment.