-
Notifications
You must be signed in to change notification settings - Fork 610
Description
Describe the bug
While analyzing thread safety/handling for another pull request, I discovered improvements for thread handling in the ACL SETUSER
command implementation.
Currently, the implementation of this command performs in place modifications to the User
properties and modifies commands within the CommandPermissionSet
as the ACL input to the command is parsed. In place modification of these objects/properties works well in a single threaded context, however it can produce inconsistent results in a multithreaded context due to modifications to shared state. (see code described here).
// Please Note: Omitted non-essential code / comments
private bool NetworkAclSetUser()
{
// User object retrieved from ACL shared by all sessions (aka shared state).
var user = aclAuthenticator.GetAccessControlList().GetUser(username);
try
{
// Remaining parameters are ACL operations
for (var i = 1; i < parseState.Count; i++)
{
var op = parseState.GetString(i);
ACLParser.ApplyACLOpToUser(ref user, op); // Modifications to shared state in multithreaded context.
}
}
catch (ACLException exception)
// Rest of method...
In this scenario, the User
(aka shared state) is stored in memory within a ConcurrentDictionary
located within the AccessControlList
. I believe the AccessControlList
is shared across all instances of RespServerSession
(see code reference here). If multiple clients concurrently call ACL SETUSER
for the same user it can produce inconsistent results that combine input arguments from multiple invocations of ACL SETUSER
.
Additionally, there is a race condition that can occur if multiple clients attempt to issue the initial ACL SETUSER
command for a user due to contention on the ConcurrentDictionary
in the AccessControlList
.
Steps to reproduce the bug
The following unit tests can be used to recreate the issue. I have included these in an accompanying PR that addresses the issue.
/// <summary>
/// Tests that ACL SETUSER works in parallel without corrupting the user's ACL.
///
/// Test launches multiple clients that apply two simple ACL changes to the same user many times in parallel.
/// Validates that ACL result after each execution is one of the possible valid responses.
///
/// Race conditions are not deterministic so test uses repeat.
///
/// </summary>
[TestCase(128, 2048)]
[Repeat(2)]
public async Task ParallelAclSetUserTest(int degreeOfParallelism, int iterationsPerSession)
{
string activeUserWithGetCommand = $"ACL SETUSER {TestUserA} on >{DummyPassword} +get";
string inactiveUserWithoutGetCommand = $"ACL SETUSER {TestUserA} off >{DummyPassword} -get";
string activeUserWithGet = $"user {TestUserA} on #{DummyPasswordHash} +get";
string inactiveUserWithoutGet = $"user {TestUserA} off #{DummyPasswordHash} -get";
string inactiveUserWithNoCommands = $"user {TestUserA} off #{DummyPasswordHash}";
var c = TestUtils.GetGarnetClientSession();
c.Connect();
_ = await c.ExecuteAsync(activeUserWithGetCommand.Split(" "));
// Run multiple sessions that stress AUTH
await Parallel.ForAsync(0, degreeOfParallelism, async (t, state) =>
{
using var c = TestUtils.GetGarnetClientSession();
c.Connect();
for (uint i = 0; i < iterationsPerSession; i++)
{
await Task.WhenAll(
c.ExecuteAsync(activeUserWithGetCommand.Split(" ")),
c.ExecuteAsync(inactiveUserWithoutGetCommand.Split(" ")));
var aclListResponse = await c.ExecuteForArrayAsync("ACL", "LIST");
if (!aclListResponse.Contains(activeUserWithGet) &&
!aclListResponse.Contains(inactiveUserWithoutGet) &&
!aclListResponse.Contains(inactiveUserWithNoCommands))
{
string corruptedAcl = aclListResponse.First(line => line.Contains(TestUserA));
throw new AssertionException($"Invalid ACL: {corruptedAcl}");
}
}
});
}
/// <summary>
/// Tests that ACL SETUSER works in parallel without fatal contention on user in ACL map.
///
/// Test launches multiple clients that apply the same ACL change to the same user. Creates race to become the
/// the first client to add the user to the ACL. Throws after initial insert into ACL if threading issues exist.
///
/// Race conditions are not deterministic so test uses repeat.
///
/// </summary>
[TestCase(128, 2048)]
[Repeat(10)]
public async Task ParallelAclSetUserAvoidsMapContentionTest(int degreeOfParallelism, int iterationsPerSession)
{
string command1 = $"ACL SETUSER {TestUserA} on >{DummyPassword}";
await Parallel.ForAsync(0, degreeOfParallelism, async (t, state) =>
{
using var c = TestUtils.GetGarnetClientSession();
c.Connect();
List<Task> tasks = new();
for (uint i = 0; i < iterationsPerSession; i++)
{
// Creates race between threads contending for first insert into ACL. Throws after first ACL insert.
tasks.Add(c.ExecuteAsync(command1.Split(" ")));
}
await Task.WhenAll(tasks);
});
ClassicAssert.Pass();
}
}
Expected behavior
I would expect the following behavior from the ACL SETUSER
command:
- It would not throw an exception when two threads or clients race to insert the first user. Instead, one thread/client would insert the user and the other would perform an update.
- It would maintain integrity of the user ACL, avoiding the combination of input arguments across client invocations of the command.
- It would handle contention among clients attempting to modify the same user, producing linearizable results.
- Subsequent calls to
ACL LIST
or in the futureACL GETUSER
would return with deterministic results for the ACL operations performed viaACL SETUSER
.
Screenshots
No response
Release version
I am building and testing against the source code in the repo as of February 1st, 2025.
IDE
No response
OS version
No response
Additional context
I have submitted this accompanying PR to address this issue: #989