-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(bridge): enforce TaskCompletionSource handshake gate to prevent connection race #3110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
81c9788
0a71a91
8511786
8b9f829
b53c407
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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); | ||
| try | ||
| { | ||
| var options = new ConfigurationOptions | ||
| { | ||
| EndPoints = { { TestConfig.Current.MasterServer, TestConfig.Current.MasterPort } }, | ||
|
Check failure on line 20 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
| AbortOnConnectFail = false, | ||
| AllowAdmin = true | ||
|
Check failure on line 23 in tests/StackExchange.Redis.Tests/ConnectionRaceTests.cs
|
||
| }; | ||
|
|
||
| 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
|
||
|
|
||
| // Trigger an asynchronous connection reset loop | ||
| var resetLoop = Task.Run(async () => | ||
| { | ||
| for (int i = 0; i < 5; i++) | ||
| { | ||
| server.SimulateConnectionFailure(SimulatedFailureType.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); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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