Introduces a separate thread in the UserManager. #21857

johann-listunov · 2025-07-15T07:45:25Z

Add a separate thread to the UserManger to handle the load of the user-cache asynchronously.
This pre-loads the user-cache as fast as possible (as soon as the global version changes)
Also, compared to the old version, this allows the read-only operation to work on the user-cache
for longer and not block each other while trying to load a new version. On a high load environment this also
means a more responsive UserManger.

Scope & Purpose

💩 Bugfix
🍕 New feature
🔥 Performance improvement
🔨 Refactoring/simplification

Checklist

Related Information

(Please reference tickets / specification / other PRs etc)

Docs PR:
Enterprise PR:
GitHub issue / Jira ticket:
Design document:

…e when needed.

johann-listunov · 2025-07-15T09:45:09Z

arangod/Auth/UserManager.cpp


-  std::lock_guard guard{_loadFromDBLock};    // must be first
-  WRITE_LOCKER(writeGuard, _userCacheLock);  // must be second
+  READ_LOCKER(readGuard, _userCacheLock);  // must be second


Remove comment.

johann-listunov · 2025-07-15T09:45:37Z

arangod/Auth/UserManager.cpp

@@ -379,27 +409,34 @@ VPackBuilder auth::UserManager::allUsers() {
 }

 void auth::UserManager::triggerCacheRevalidation() {
-  triggerLocalReload();
+  uint64_t versionToReloadTo = globalVersion() + 1;


Can be const.

johann-listunov · 2025-07-15T09:49:17Z

tests/Auth/UserManagerClusterTest.cpp

-  }
-}
+// This test is not really applicable anymore.
+// Neither triggerGlobalReload or triggerLocal reload are incrementing the


Suggested change

// Neither triggerGlobalReload or triggerLocal reload are incrementing the

// Neither triggerGlobalReload or triggerLocalReload are incrementing the

johann-listunov · 2025-07-15T09:49:52Z

tests/js/client/restart/test-restart-cluster.js

@@ -229,7 +229,7 @@ function testSuite() {
        }
        let aliveStatus = waitForAlive(30, coordinator.url, { auth: { bearer: jwt } });
        // note: this should actually work, but currently doesn't TODO


Remove comment?

I'd think yes.

goedderz · 2025-07-16T08:15:32Z

CHANGELOG

@@ -1,6 +1,12 @@
 devel
 -----

+* Add a separate thread to the UserManger to handle the load of the user-cache asynchronously.


Suggested change

* Add a separate thread to the UserManger to handle the load of the user-cache asynchronously.

* Add a separate thread to the UserManager to handle the load of the user-cache asynchronously.

goedderz · 2025-07-16T08:15:39Z

CHANGELOG

+  This pre-loads the user-cache as fast as possible (as soon as the global version changes)
+  Also, compared to the old version, this allows the read-only operation to work on the user-cache 
+  for longer and not block each other while trying to load a new version. On a high load environment this also 
+  means a more responsive UserManger. 


Suggested change

means a more responsive UserManger.

means a more responsive UserManager.

goedderz · 2025-07-16T11:47:34Z

CHANGELOG

+* Add a separate thread to the UserManger to handle the load of the user-cache asynchronously.
+  This pre-loads the user-cache as fast as possible (as soon as the global version changes)
+  Also, compared to the old version, this allows the read-only operation to work on the user-cache 
+  for longer and not block each other while trying to load a new version. On a high load environment this also 
+  means a more responsive UserManger. 


I think it's valuable to note the motivation for this, namely that this avoids the HTTP server threads getting blocked while the user-cache is being updated.

goedderz · 2025-07-16T12:14:40Z

arangod/Auth/UserManager.cpp

-void auth::UserManager::loadFromDB() {
-  TRI_ASSERT(ServerState::instance()->isSingleServerOrCoordinator());
-
-  if (_internalVersion.load(std::memory_order_acquire) == globalVersion()) {
+void auth::UserManager::loadUserCacheAndStartUpdateThread() noexcept {
+  if (_usersInitialized.load(std::memory_order_relaxed) == true) {
    return;
  }


I assume this method might be called multiple times, but not concurrently? Otherwise this would not work.

goedderz · 2025-07-16T12:16:28Z

arangod/Auth/UserManager.cpp

+  LOG_TOPIC("ef78c", INFO, Logger::AUTHENTICATION) << "Preloading user cache";
+  auto const start = chrono::system_clock::now();
+  int64_t lastLog = 0;
+  int64_t constexpr LogEverySeconds = 3;


Suggested change

int64_t constexpr LogEverySeconds = 3;

int64_t constexpr logEverySeconds = 3;

goedderz · 2025-07-17T07:30:28Z

tests/Auth/UserManagerClusterTest.cpp

+//  } catch (...) {
+//    FAIL();
+//  }
+//}

 TEST_F(UserManagerClusterTest, cacheRevalidationShouldKeepVersionsInLine) {


The changes to the used failure point defeat the purpose of this test. It's arguable whether the failure point should be changed (e.g., move it to the user manager thread, but instead of throwing an exception set a special value that can be observed here); and/or whether the test should be rewritten instead, as it doesn't quite match up any longer with the way the user manager works now.

goedderz · 2025-07-17T07:50:17Z

tests/Auth/UserManagerClusterTest.cpp

@@ -166,10 +172,6 @@ TEST_F(UserManagerClusterTest, cacheRevalidationShouldKeepVersionsInLine) {

 TEST_F(UserManagerClusterTest,
       triggerLocalReloadShouldNotUpdateClusterVersion) {


As above, the changes to the failure point defeat the purpose of this test, at least of the second part. Again, it's arguable whether the test should be written differently.

goedderz · 2025-07-17T08:14:50Z

tests/Auth/UserManagerClusterTest.cpp

@@ -203,10 +205,6 @@ TEST_F(UserManagerClusterTest,
 }

 TEST_F(UserManagerClusterTest, triggerGlobalReloadShouldUpdateClusterVersion) {


Again, the second part of the test at least must be done differently now to still make sense.

goedderz · 2025-07-17T09:16:13Z

tests/js/client/restart/test-restart-cluster.js

@@ -229,7 +229,7 @@ function testSuite() {
        }
        let aliveStatus = waitForAlive(30, coordinator.url, { auth: { bearer: jwt } });
        // note: this should actually work, but currently doesn't TODO


I'd think yes.

goedderz · 2025-07-17T09:17:22Z

tests/IResearch/IResearchAnalyzerFeatureTest.cpp

@@ -468,22 +473,27 @@ class IResearchAnalyzerFeatureTest
    auto* userManager = authFeature.userManager();
    if (userManager != nullptr) {
      userManager->removeAllUsers();
+      userManager->shutdown();
    }


You might not need the user manager thread at all, and same for the failure points. Same for the other tests.

jvolmer

We already discussed some stuff about trying to get rid of the failure points and possibly waiting for a version change in the tests, so here just some small comments I noticed.

jvolmer · 2025-07-16T11:20:58Z

arangod/Auth/UserManager.h

+  // this is only needed in unittest this
+  // will shutdown the running thread on demand
+  // its needed because the failure point can be deactivated before the thread
+  // is finished and can lead to calls on the server that are not initialized
+  // properly in the unit-test environment


Suggested change

// this is only needed in unittest this

// will shutdown the running thread on demand

// its needed because the failure point can be deactivated before the thread

// is finished and can lead to calls on the server that are not initialized

// properly in the unit-test environment

// This is only needed in unittest:

// This will shutdown the running thread on demand. It's needed because the

// failure point can be deactivated before the thread is finished and can lead

// to calls on the server that are not initialized properly in the unit-test

// environment.

jvolmer · 2025-07-16T11:22:52Z

arangod/Auth/UserManager.h

+  // for test that was previously in the loadFromDB call, that is now in
+  // a seperate thread


Suggested change

// for test that was previously in the loadFromDB call, that is now in

// a seperate thread

// for a test that was previously in the loadFromDB call which is now executed in

// a separate thread

jvolmer · 2025-07-16T16:31:00Z

arangod/Auth/UserManager.cpp

  }

+  TRI_ASSERT(ServerState::instance()->isSingleServerOrCoordinator());


it seems weird to me to do the assertion here, this feels random, would be move this directly to the start of the function. And I would also like to have this assertion at the start of loadUserCacheAndStartUpdateThread.

Introduces a separate thread in the UserManager to load the user-cach…

261d08f

…e when needed.

cla-bot bot added the cla-signed label Jul 15, 2025

Fixed changelog and reverted a test-change I missed to revert.

704f2ff

johann-listunov commented Jul 15, 2025

View reviewed changes

goedderz reviewed Jul 17, 2025

View reviewed changes

jvolmer reviewed Jul 17, 2025

View reviewed changes

	// Neither triggerGlobalReload or triggerLocal reload are incrementing the
	// Neither triggerGlobalReload or triggerLocalReload are incrementing the

	* Add a separate thread to the UserManger to handle the load of the user-cache asynchronously.
	* Add a separate thread to the UserManager to handle the load of the user-cache asynchronously.

	means a more responsive UserManger.
	means a more responsive UserManager.

	int64_t constexpr LogEverySeconds = 3;
	int64_t constexpr logEverySeconds = 3;

		@@ -166,10 +172,6 @@ TEST_F(UserManagerClusterTest, cacheRevalidationShouldKeepVersionsInLine) {

		TEST_F(UserManagerClusterTest,
		triggerLocalReloadShouldNotUpdateClusterVersion) {

		@@ -203,10 +205,6 @@ TEST_F(UserManagerClusterTest,
		}

		TEST_F(UserManagerClusterTest, triggerGlobalReloadShouldUpdateClusterVersion) {

		// for test that was previously in the loadFromDB call, that is now in
		// a seperate thread

		}

		TRI_ASSERT(ServerState::instance()->isSingleServerOrCoordinator());

Introduces a separate thread in the UserManager. #21857

Are you sure you want to change the base?

Introduces a separate thread in the UserManager. #21857

Uh oh!

Conversation

johann-listunov commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Scope & Purpose

Checklist

Related Information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvolmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johann-listunov commented Jul 15, 2025 •

edited

Loading