Improve ACL SETUSER Thread Handling #989

kevinmichaelbowersox · 2025-02-01T21:38:31Z

This pull requests fixes #988. Please see the detailed discussion provided in the issue for a deeper discussion of the problem this PR addresses.

Quick Problem Overview

The ACL SETUSER command may produce inconsistent results when multiple clients/threads attempt to concurrently modify the same user. Please find test cases in this PR that demonstrate the concurrency issues this PR addresses.

Solution Overview

This PR addresses the issue by implementing the following changes:

Establishing a table of lock objects in the AccessControlList that can be used for taking granular level locks on a per user basis. This locking strategy should be used sparingly, possibly only for modifications to users via ACL SETUSER.
In the ACL SETUSER implementation, a user level lock is taken prior to a read, copy, write sequence performed in the critical section of the lock. This is necessary to prevent one client from reading the user object from the AccessControlList while another client is in the process of modifying it during this sequence. A copy constructor was added to User to protect against external modification while performing this sequence.
An observer pattern is implemented for subscribers interested in being notified of when ACL modifications are performed on the AccessControlList. Sessions subscribe to these notifications at creation time, giving them awareness of changes to the ACL for the user in the current session and the ability to refresh the ACL for the respective user. With this capability, ACL modifications are still applied in real time when users are modified.
A flag was added to the session to signal staleness of the ACL. When the flag is raised, the session will refresh the ACL for the user before the next command execution.

Testing and Verification

Added unit tests that repro the situation prior to making changes and verified existence of the issue.
Performed local testing manually to verify expected results used in test cases.
Verified new and existing tests passed once fix was implemented.

kevin-montrose · 2025-02-03T19:29:35Z

libs/server/ACL/AccessControlList.cs

+        /// Registers a subscriber to receive notifications when modifications are performed to the <see cref="AccessControlList"/>.
+        /// </summary>
+        /// <param name="subscriber">The <see cref="IAccessControlListSubscriber"/> to register.</param>
+        internal void Subscribe(IAccessControlListSubscriber subscriber)


I don't see any way subscriptions are removed - won't this extend the lifetime of all RespServerSessions indefinitely?

Pushed a fix that unsubscribes the session from the ACL notifications when the session is killed.

/// Subsequent calls will return false. /// </summary> public bool TryKill() { if (!networkSender.TryClose()) { return false; } if (_authenticator is GarnetACLAuthenticator aclAuthenticator) { aclAuthenticator.GetAccessControlList().Unsubscribe(this); } return true; }

kevin-montrose · 2025-02-03T19:31:39Z

libs/server/ACL/AccessControlList.cs

+        /// <param name="user">The created or updated <see cref="User"/> that triggered the notification.</param>
+        private void NotifySubscribers(User user)
+        {
+            foreach (IAccessControlListSubscriber subscriber in _subscribers.Values)


I believe there is a race here:

A new RespServerSession is created, it runs through this.AuthenticateUser (L236) and the thread is paused before subscription runs

Another thread executes a ACL SETUSER, triggering this NotifySubscribers method

The thread running in #1 resumes, and registers itself

Net effect, that new RespServerSession ends up with the old user.

Pushed a fix that checks the authenticated user is current prior to finishing instantiation of the session.

// Ensure user was not updated during initialization. if (_authenticator is GarnetACLAuthenticator aclAuthenticator && _user != aclAuthenticator.GetUser()) { this.RefreshUser(); }

libs/server/ACL/IAccessControlListSubscriber.cs

libs/server/Resp/AdminCommands.cs

libs/server/Resp/RespServerSession.cs

kevin-montrose · 2025-02-03T19:37:56Z

libs/server/Resp/RespServerSession.cs

@@ -438,6 +469,11 @@ private void ProcessMessages()
                // Check ACL permissions for the command
                if (cmd != RespCommand.INVALID)
                {
+                    if (_isUserAclStale && cmd != RespCommand.AUTH)


This is very hot code, we'll want to see benchmarks to measure the impact.

I'd be more comfortable if this was in a failure path or something, rather than at the very start of the command dispatch.

This part is essential to the solution because it forces the refresh when the user's acl has been updated. We'll need to check the benchmarks to see if the approach is viable. I was hoping the conditional evaluation of the simple boolean would not be a drain on performance.

If the benchmarks do not perform well, the entire design will need reconsidered.

Main Commit Sha(a63ad80) BDN Results:

Method Params Mean Error StdDev Allocated

Set ACL 14.20 μs 0.052 μs 0.049 μs -

SetEx ACL 19.98 μs 0.035 μs 0.031 μs -

SetNx ACL 20.45 μs 0.055 μs 0.052 μs -

SetXx ACL 23.73 μs 0.101 μs 0.085 μs -

GetFound ACL 15.55 μs 0.029 μs 0.026 μs -

GetNotFound ACL 10.95 μs 0.016 μs 0.015 μs -

Increment ACL 21.16 μs 0.044 μs 0.039 μs -

Decrement ACL 21.12 μs 0.064 μs 0.053 μs -

IncrementBy ACL 25.56 μs 0.032 μs 0.028 μs -

DecrementBy ACL 27.57 μs 0.096 μs 0.085 μs -

Set AOF 19.96 μs 0.064 μs 0.057 μs -

SetEx AOF 25.55 μs 0.089 μs 0.079 μs -

SetNx AOF 27.04 μs 0.138 μs 0.129 μs -

SetXx AOF 28.60 μs 0.292 μs 0.273 μs -

GetFound AOF 15.62 μs 0.028 μs 0.025 μs -

GetNotFound AOF 10.80 μs 0.024 μs 0.022 μs -

Increment AOF 27.29 μs 0.120 μs 0.107 μs -

Decrement AOF 27.98 μs 0.053 μs 0.049 μs -

IncrementBy AOF 31.82 μs 0.121 μs 0.107 μs -

DecrementBy AOF 32.33 μs 0.150 μs 0.133 μs -

Set None 14.11 μs 0.018 μs 0.015 μs -

SetEx None 19.96 μs 0.118 μs 0.110 μs -

SetNx None 20.70 μs 0.038 μs 0.035 μs -

SetXx None 23.47 μs 0.039 μs 0.033 μs -

GetFound None 15.64 μs 0.041 μs 0.034 μs -

GetNotFound None 11.03 μs 0.023 μs 0.019 μs -

Increment None 20.76 μs 0.027 μs 0.023 μs -

Decrement None 21.87 μs 0.037 μs 0.033 μs -

IncrementBy None 27.58 μs 0.180 μs 0.160 μs -

DecrementBy None 27.71 μs 0.075 μs 0.066 μs -

This PR Commit SHA(4ca6db5) BDN Results:

Method Params Mean Error StdDev Allocated

Set ACL 15.13 us 0.103 us 0.091 us -

SetEx ACL 20.49 us 0.064 us 0.060 us -

SetNx ACL 21.70 us 0.072 us 0.060 us -

SetXx ACL 23.91 us 0.058 us 0.052 us -

GetFound ACL 15.58 us 0.105 us 0.087 us -

GetNotFound ACL 10.92 us 0.016 us 0.014 us -

Increment ACL 21.11 us 0.049 us 0.043 us -

Decrement ACL 21.81 us 0.057 us 0.050 us -

IncrementBy ACL 26.21 us 0.114 us 0.101 us -

DecrementBy ACL 26.17 us 0.063 us 0.059 us -

Set AOF 19.36 us 0.077 us 0.068 us -

SetEx AOF 25.41 us 0.063 us 0.056 us -

SetNx AOF 27.82 us 0.081 us 0.072 us -

SetXx AOF 27.71 us 0.086 us 0.076 us -

GetFound AOF 15.55 us 0.029 us 0.026 us -

GetNotFound AOF 11.46 us 0.010 us 0.009 us -

Increment AOF 26.35 us 0.036 us 0.032 us -

Decrement AOF 27.86 us 0.043 us 0.040 us -

IncrementBy AOF 32.92 us 0.109 us 0.097 us -

DecrementBy AOF 33.06 us 0.090 us 0.085 us -

Set None 14.89 us 0.019 us 0.017 us -

SetEx None 20.96 us 0.080 us 0.071 us -

SetNx None 20.12 us 0.039 us 0.037 us -

SetXx None 23.88 us 0.045 us 0.040 us -

GetFound None 15.77 us 0.026 us 0.022 us -

GetNotFound None 10.59 us 0.020 us 0.018 us -

Increment None 22.16 us 0.206 us 0.183 us -

Decrement None 22.51 us 0.115 us 0.096 us -

IncrementBy None 26.93 us 0.138 us 0.122 us -

kevin-montrose · 2025-02-03T19:42:32Z

libs/server/ACL/AccessControlList.cs

+        /// <summary>
+        /// Dictionary containing stable lock objects for each user in the ACL.
+        /// </summary>
+        ConcurrentDictionary<string, object> _userLockObjects = new();


I don't quite get why we're introducing lock objects - we can just lock on the User if it's already exists?

For new users things could be phrased in terms of GetOrAdd().

I considered using the user as the lock object, however we are replacing the user object in the access control list's dictionary when ACL SETUSER is called. This makes it a bad candidate for a lock object because we would introduce additional threading issues (different threads would lock on different objects for the user). We need a stable object for each thread to obtain the user level lock on.

Fair.

The dance with ConcurrentDictionaries isn't quite right though, which makes me think there might be a simpler approach that does enable User as a lock.

badrishc · 2025-02-04T19:46:09Z

Minor nit: can you fix the title of this PR to be a bit descriptive? Thanks.

kevin-montrose · 2025-02-06T16:26:34Z

libs/server/ACL/ACLParser.cs

@@ -110,6 +109,11 @@ public static User ParseACLRule(string input, AccessControlList acl = null)
                ApplyACLOpToUser(ref user, tokens[i]);
            }

+            if (acl != null)


nit: acl?.AddOrReplaceUser(user);

kevin-montrose · 2025-02-06T16:35:17Z

libs/server/ACL/AccessControlList.cs

@@ -98,7 +139,15 @@ public bool DeleteUser(string username)
            {
                throw new ACLException("The special 'default' user cannot be removed from the system");
            }
-            return _users.TryRemove(username, out _);
+
+            bool userDeleted = _users.TryRemove(username, out _);


There's a race here.

If Session A deletes a user and Session B adds the user it is possible for this to happen:

Sessions A Session B

_users.TryRemove("foo", ...) => true

Thread Suspends AddUser called with "foo", populates _users, does not modify _userLockObjects

Thread Resumes

Because userDeleted == true, _userLockObjects.TryRemove("foo", out _) called

At the end _users is populated, but _userLockObject is not.

kevin-montrose · 2025-02-06T16:38:01Z

libs/server/ACL/AccessControlList.cs

+        /// <summary>
+        /// Dictionary containing stable lock objects for each user in the ACL.
+        /// </summary>
+        ConcurrentDictionary<string, object> _userLockObjects = new();


Fair.

The dance with ConcurrentDictionaries isn't quite right though, which makes me think there might be a simpler approach that does enable User as a lock.

kevin-montrose · 2025-02-06T16:42:54Z

libs/server/ACL/AccessControlList.cs

+        /// <summary>
+        /// The <see cref="RespServerSession"/>s that will receive access control list change notifications.
+        /// </summary>
+        private readonly ConcurrentDictionary<string, RespServerSession> _subscribedSessions = new();


nit: it's not clear to me why this is keyed by a string? You could just use the RespServerSesssion directly, object identity should be sufficient.

kevin-montrose · 2025-02-06T16:45:38Z

libs/server/Resp/RespServerSession.cs

+            // Ensure user was not updated during initialization.
+            if (_authenticator is GarnetACLAuthenticator aclAuthenticator && _user != aclAuthenticator.GetUser())
+            {
+                this.RefreshUser();


I believe you need force: true here.

kevin-montrose · 2025-02-06T17:12:22Z

The more I look at this, the more I'm tempted to try an approach that skips all the ConcurrentDictionary, lock, and notify stuff.

Something akin to:

class User
{
   // If User has been modified, the resulting user will be stored here
   private User successorUser;

   public User GetEffectiveUser()
   => successUser?.GetEffectiveUser() ?? this;
}

// --- and then in RespServerSession, somewhere
_user = _user.GetEffectiveUser();

// --- and then in ACL updates
// 1. still clone the user and make changes to the copy
// 2. use an Interlocked.CompareExchange(ref successorUser, newUser, null)
// 3. if that returns non-null, restart the command

If I wasn't on-call I'd sketch it out properly...

kevinmichaelbowersox · 2025-02-06T18:58:09Z

The approach without the concurrent dictionary would be simpler and seems more promising. We'll hold on this implementation and shift efforts towards this alternative design in a separate PR.

kbowersox added 6 commits February 1, 2025 09:23

Test First: Add Failing ACL SETUSER Parallel Test Cases

65fabeb

Clean up extra calls in test

02b38ee

Improve ACL SETUSER thread safety

6f330f8

Subscribe to ACL notification after authentication of default user

21b69e0

Add User Locks for Linearizability of ACL SETUSER

7a4a76d

Clean up unused import in RespServerSession

dc08dcf

kevinmichaelbowersox marked this pull request as ready for review February 1, 2025 21:41

Remove newline from file to resolve format violation

2872177

kevinmichaelbowersox mentioned this pull request Feb 1, 2025

Improve ACL SETUSER Thread Handling #988

Open

Remove user lock object from dictionary upon deletion

b9ad4b2

badrishc requested a review from kevin-montrose February 2, 2025 03:23

kevin-montrose requested changes Feb 3, 2025

View reviewed changes

kbowersox added 2 commits February 3, 2025 12:51

Address feedback on PR regarding races, interfaces and errant debug code

d5629a7

Reformat unsubscribe logic

4ca6db5

kevinmichaelbowersox changed the title ~~Users/kbowersox/aclsetuser threads~~ Improve ACL SETUSER Thread Handling Feb 4, 2025

Fix Merge Conflict

20f8a7a

kevinmichaelbowersox requested a review from kevin-montrose February 5, 2025 14:59

kevin-montrose requested changes Feb 6, 2025

View reviewed changes

badrishc assigned kevin-montrose Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ACL SETUSER Thread Handling #989

Improve ACL SETUSER Thread Handling #989

kevinmichaelbowersox commented Feb 1, 2025 •

edited

Loading

kevin-montrose Feb 3, 2025

kevinmichaelbowersox Feb 3, 2025 •

edited

Loading

kevin-montrose Feb 3, 2025

kevinmichaelbowersox Feb 3, 2025

kevin-montrose Feb 3, 2025

kevinmichaelbowersox Feb 3, 2025

kevinmichaelbowersox Feb 4, 2025

kevinmichaelbowersox Feb 4, 2025

kevin-montrose Feb 3, 2025

kevinmichaelbowersox Feb 3, 2025 •

edited

Loading

kevin-montrose Feb 6, 2025

badrishc commented Feb 4, 2025

kevin-montrose Feb 6, 2025

kevin-montrose Feb 6, 2025

kevin-montrose Feb 6, 2025

kevin-montrose Feb 6, 2025

kevin-montrose Feb 6, 2025

kevin-montrose commented Feb 6, 2025

kevinmichaelbowersox commented Feb 6, 2025 •

edited

Loading

Method	Params	Mean	Error	StdDev	Allocated
Set	ACL	14.20 μs	0.052 μs	0.049 μs	-
SetEx	ACL	19.98 μs	0.035 μs	0.031 μs	-
SetNx	ACL	20.45 μs	0.055 μs	0.052 μs	-
SetXx	ACL	23.73 μs	0.101 μs	0.085 μs	-
GetFound	ACL	15.55 μs	0.029 μs	0.026 μs	-
GetNotFound	ACL	10.95 μs	0.016 μs	0.015 μs	-
Increment	ACL	21.16 μs	0.044 μs	0.039 μs	-
Decrement	ACL	21.12 μs	0.064 μs	0.053 μs	-
IncrementBy	ACL	25.56 μs	0.032 μs	0.028 μs	-
DecrementBy	ACL	27.57 μs	0.096 μs	0.085 μs	-
Set	AOF	19.96 μs	0.064 μs	0.057 μs	-
SetEx	AOF	25.55 μs	0.089 μs	0.079 μs	-
SetNx	AOF	27.04 μs	0.138 μs	0.129 μs	-
SetXx	AOF	28.60 μs	0.292 μs	0.273 μs	-
GetFound	AOF	15.62 μs	0.028 μs	0.025 μs	-
GetNotFound	AOF	10.80 μs	0.024 μs	0.022 μs	-
Increment	AOF	27.29 μs	0.120 μs	0.107 μs	-
Decrement	AOF	27.98 μs	0.053 μs	0.049 μs	-
IncrementBy	AOF	31.82 μs	0.121 μs	0.107 μs	-
DecrementBy	AOF	32.33 μs	0.150 μs	0.133 μs	-
Set	None	14.11 μs	0.018 μs	0.015 μs	-
SetEx	None	19.96 μs	0.118 μs	0.110 μs	-
SetNx	None	20.70 μs	0.038 μs	0.035 μs	-
SetXx	None	23.47 μs	0.039 μs	0.033 μs	-
GetFound	None	15.64 μs	0.041 μs	0.034 μs	-
GetNotFound	None	11.03 μs	0.023 μs	0.019 μs	-
Increment	None	20.76 μs	0.027 μs	0.023 μs	-
Decrement	None	21.87 μs	0.037 μs	0.033 μs	-
IncrementBy	None	27.58 μs	0.180 μs	0.160 μs	-
DecrementBy	None	27.71 μs	0.075 μs	0.066 μs	-

Method	Params	Mean	Error	StdDev	Allocated
Set	ACL	15.13 us	0.103 us	0.091 us	-
SetEx	ACL	20.49 us	0.064 us	0.060 us	-
SetNx	ACL	21.70 us	0.072 us	0.060 us	-
SetXx	ACL	23.91 us	0.058 us	0.052 us	-
GetFound	ACL	15.58 us	0.105 us	0.087 us	-
GetNotFound	ACL	10.92 us	0.016 us	0.014 us	-
Increment	ACL	21.11 us	0.049 us	0.043 us	-
Decrement	ACL	21.81 us	0.057 us	0.050 us	-
IncrementBy	ACL	26.21 us	0.114 us	0.101 us	-
DecrementBy	ACL	26.17 us	0.063 us	0.059 us	-
Set	AOF	19.36 us	0.077 us	0.068 us	-
SetEx	AOF	25.41 us	0.063 us	0.056 us	-
SetNx	AOF	27.82 us	0.081 us	0.072 us	-
SetXx	AOF	27.71 us	0.086 us	0.076 us	-
GetFound	AOF	15.55 us	0.029 us	0.026 us	-
GetNotFound	AOF	11.46 us	0.010 us	0.009 us	-
Increment	AOF	26.35 us	0.036 us	0.032 us	-
Decrement	AOF	27.86 us	0.043 us	0.040 us	-
IncrementBy	AOF	32.92 us	0.109 us	0.097 us	-
DecrementBy	AOF	33.06 us	0.090 us	0.085 us	-
Set	None	14.89 us	0.019 us	0.017 us	-
SetEx	None	20.96 us	0.080 us	0.071 us	-
SetNx	None	20.12 us	0.039 us	0.037 us	-
SetXx	None	23.88 us	0.045 us	0.040 us	-
GetFound	None	15.77 us	0.026 us	0.022 us	-
GetNotFound	None	10.59 us	0.020 us	0.018 us	-
Increment	None	22.16 us	0.206 us	0.183 us	-
Decrement	None	22.51 us	0.115 us	0.096 us	-
IncrementBy	None	26.93 us	0.138 us	0.122 us	-

Sessions A	Session B
`_users.TryRemove("foo", ...)` => true
Thread Suspends	AddUser called with "foo", populates `_users`, does not modify `_userLockObjects`
Thread Resumes
Because `userDeleted == true`, `_userLockObjects.TryRemove("foo", out _)` called

Improve ACL SETUSER Thread Handling #989

Are you sure you want to change the base?

Improve ACL SETUSER Thread Handling #989

Conversation

kevinmichaelbowersox commented Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

kevinmichaelbowersox Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinmichaelbowersox Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badrishc commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-montrose commented Feb 6, 2025

kevinmichaelbowersox commented Feb 6, 2025 • edited Loading

kevinmichaelbowersox commented Feb 1, 2025 •

edited

Loading

kevinmichaelbowersox Feb 3, 2025 •

edited

Loading

kevinmichaelbowersox Feb 3, 2025 •

edited

Loading

kevinmichaelbowersox commented Feb 6, 2025 •

edited

Loading