Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/StackExchange.Redis/PhysicalBridge.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
Expand Down Expand Up @@ -56,6 +56,8 @@ internal sealed class PhysicalBridge : IDisposable

private volatile bool reportNextFailure = true, reconfigureNextFailure = false;

private TaskCompletionSource<bool> _handshakeCompletion = new(TaskCreationOptions.RunContinuationsAsynchronously);

private volatile int state = (int)State.Disconnected;

internal long? ConnectionId => physical?.ConnectionId;
Expand Down Expand Up @@ -526,6 +528,7 @@ internal void OnFullyEstablished(PhysicalConnection connection, string source)
connection.SetIdle();
if (physical == connection && !isDisposed && ChangeState(State.ConnectedEstablishing, State.ConnectedEstablished))
{
_handshakeCompletion.TrySetResult(true);
reportNextFailure = reconfigureNextFailure = true;
LastException = null;
Interlocked.Exchange(ref failConnectCount, 0);
Expand Down Expand Up @@ -800,6 +803,10 @@ private WriteResult WriteMessageInsideLock(PhysicalConnection physical, Message
internal WriteResult WriteMessageTakingWriteLockSync(PhysicalConnection physical, Message message)
{
Trace("Writing: " + message);
if (ConnectionState == State.ConnectedEstablishing)
{
_handshakeCompletion.Task.GetAwaiter().GetResult();
}
message.SetEnqueued(physical); // this also records the read/write stats at this point

// AVOID REORDERING MESSAGES
Expand Down Expand Up @@ -1235,6 +1242,10 @@ private WriteResult TimedOutBeforeWrite(Message message)
internal ValueTask<WriteResult> WriteMessageTakingWriteLockAsync(PhysicalConnection physical, Message message, bool bypassBacklog = false)
{
Trace("Writing: " + message);
if (ConnectionState == State.ConnectedEstablishing && !_handshakeCompletion.Task.IsCompleted)
{
return WriteMessageTakingWriteLockAsync_Gate(physical, message, bypassBacklog);
}
message.SetEnqueued(physical); // this also records the read/write stats at this point

// AVOID REORDERING MESSAGES
Expand Down Expand Up @@ -1296,6 +1307,12 @@ internal ValueTask<WriteResult> WriteMessageTakingWriteLockAsync(PhysicalConnect
}
}

private async ValueTask<WriteResult> WriteMessageTakingWriteLockAsync_Gate(PhysicalConnection physical, Message message, bool bypassBacklog)
{
await _handshakeCompletion.Task.ConfigureAwait(false);
return await WriteMessageTakingWriteLockAsync(physical, message, bypassBacklog).ConfigureAwait(false);
}

private async ValueTask<WriteResult> WriteMessageTakingWriteLockAsync_Awaited(
ValueTask<bool> pending,
PhysicalConnection physical,
Expand Down Expand Up @@ -1404,6 +1421,7 @@ private bool ChangeState(State oldState, State newState)
Volatile.Write(ref _needsReconnect, false);
Interlocked.Increment(ref socketCount);
Interlocked.Exchange(ref connectStartTicks, Environment.TickCount);
Interlocked.Exchange(ref _handshakeCompletion, new(TaskCreationOptions.RunContinuationsAsynchronously));
// separate creation and connection for case when connection completes synchronously
// in that case PhysicalConnection will call back to PhysicalBridge, and most PhysicalBridge methods assume that physical is not null;
physical = new PhysicalConnection(this, Multiplexer.RawConfig.WriteMode);
Expand Down
71 changes: 71 additions & 0 deletions tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
using System;
using System.Threading;
using System.Threading.Tasks;
using Xunit;

namespace StackExchange.Redis.Tests;

public class ConnectionRaceTests(ITestOutputHelper output) : TestBase(output)
{
[Fact]
public async Task HandshakeCompletionGatePreventsUnauthenticatedPayloads()
{
// Simulate severe thread pool exhaustion using ThreadPool.SetMinThreads(1, 1);
ThreadPool.GetMinThreads(out int workerThreads, out int completionPortThreads);
ThreadPool.SetMinThreads(1, 1);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't a great idea in a CI setup; I understand the intent, but... this might be something we simply can't do in this context due to parallelism concerns, plus the impact on the overall runtime

try
{
var options = new ConfigurationOptions
{
EndPoints = { { TestConfig.Current.MasterServer, TestConfig.Current.MasterPort } },

Check failure on line 20 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'TestConfig.Config' does not contain a definition for 'MasterPort' and no accessible extension method 'MasterPort' accepting a first argument of type 'TestConfig.Config' could be found (are you missing a using directive or an assembly reference?)

Check failure on line 20 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'TestConfig.Config' does not contain a definition for 'MasterServer' and no accessible extension method 'MasterServer' accepting a first argument of type 'TestConfig.Config' could be found (are you missing a using directive or an assembly reference?)

Check failure on line 20 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'TestConfig.Config' does not contain a definition for 'MasterPort' and no accessible extension method 'MasterPort' accepting a first argument of type 'TestConfig.Config' could be found (are you missing a using directive or an assembly reference?)

Check failure on line 20 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'TestConfig.Config' does not contain a definition for 'MasterServer' and no accessible extension method 'MasterServer' accepting a first argument of type 'TestConfig.Config' could be found (are you missing a using directive or an assembly reference?)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda confused here... this hasn't existed for 4+ years... was this PR based on a really old fork?

Password = TestConfig.Current.MasterPassword,

Check failure on line 21 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'TestConfig.Config' does not contain a definition for 'MasterPassword' and no accessible extension method 'MasterPassword' accepting a first argument of type 'TestConfig.Config' could be found (are you missing a using directive or an assembly reference?)

Check failure on line 21 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'TestConfig.Config' does not contain a definition for 'MasterPassword' and no accessible extension method 'MasterPassword' accepting a first argument of type 'TestConfig.Config' could be found (are you missing a using directive or an assembly reference?)
AbortOnConnectFail = false,
AllowAdmin = true

Check failure on line 23 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

};

await using var conn = await ConnectionMultiplexer.ConnectAsync(options);
var db = conn.GetDatabase();
var server = conn.GetServer(TestConfig.Current.MasterServer, TestConfig.Current.MasterPort);

Check failure on line 28 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'TestConfig.Config' does not contain a definition for 'MasterPort' and no accessible extension method 'MasterPort' accepting a first argument of type 'TestConfig.Config' could be found (are you missing a using directive or an assembly reference?)

Check failure on line 28 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'TestConfig.Config' does not contain a definition for 'MasterServer' and no accessible extension method 'MasterServer' accepting a first argument of type 'TestConfig.Config' could be found (are you missing a using directive or an assembly reference?)

// Trigger an asynchronous connection reset loop
var resetLoop = Task.Run(async () =>
{
for (int i = 0; i < 5; i++)
{
server.SimulateConnectionFailure(SimulatedFailureType.AuthenticationFailure);

Check failure on line 35 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

'SimulatedFailureType' does not contain a definition for 'AuthenticationFailure'
await Task.Delay(10);
}
});

// Concurrently launch 100 parallel Tasks trying to dispatch rapid PingAsync() operations.
int taskCount = 100;
var tasks = new Task[taskCount];
for (int i = 0; i < taskCount; i++)
{
tasks[i] = Task.Run(async () =>
{
for (int j = 0; j < 50; j++)
{
try
{
await db.PingAsync().ConfigureAwait(false);
}
catch (Exception ex)
{
// Assert that no commands throw a NOAUTH or protocol corruption exception
Assert.DoesNotContain("NOAUTH", ex.Message);
Assert.DoesNotContain("protocol", ex.Message.ToLowerInvariant());
}
}
});
}

await Task.WhenAll(tasks);
await resetLoop;
}
finally
{
ThreadPool.SetMinThreads(workerThreads, completionPortThreads);
}
}
}
Loading