Introduces a separate thread in the UserManager. #21857
Conversation
arangod/Auth/UserManager.cpp
Outdated
| 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; | ||
| } |
There was a problem hiding this comment.
I assume this method might be called multiple times, but not concurrently? Otherwise this would not work.
arangod/Auth/UserManager.cpp
Outdated
| while (_internalVersion.load(std::memory_order_acquire) < globalVersion()) { | ||
| _internalVersion.wait(versionBeforeReload); | ||
| } | ||
| TRI_IF_FAILURE("UserManager::FailReload") { |
There was a problem hiding this comment.
I wanted a way to really tell if the reload did anything. So use this in a test to verify internal state.
There was a problem hiding this comment.
I also don't quite understand what the goal of this (in context of the test using the failure point) is. The test checks that this throws an exception, which is done if internalVersionWasUpdated && versionsAreEqual is true. internalVersionWasUpdated must always be true, so it basically checks versionsAreEqual.
In the context of the test, I think we assume noone else does any changes to either the users collection or the global version. In which case the previous loop also guarantees that. I think the way it currently is, the test with the failure point does not give any more value than such an assertion:
TRI_ASSERT(versionAfterReload <= _internalVersion.load(std::memory_order_relaxed));or
TRI_ASSERT(versionAfterReload <= internalVersion);, which is just the inverted loop condition, and thus quite obviously correct.
| } | ||
| // Internally it will do globalReload, setGlobalVersion, and blocks | ||
| // until the internal thread synchronizes the versions (internal and global). | ||
| auto const internalVersionBeforeReload = um->internalVersion(); |
There was a problem hiding this comment.
could we do an assertion here on the internal version, just to make more visible that the internal version changes after triggerCacheRevalidation?
arangod/Auth/UserManager.cpp
Outdated
| void auth::UserManager::triggerCacheRevalidation() { | ||
| uint64_t versionToReloadTo = globalVersion() + 1; | ||
| uint64_t const versionBeforeReload = globalVersion(); | ||
| triggerGlobalReload(); | ||
| setGlobalVersion(versionToReloadTo); | ||
| while (_internalVersion.load() < versionToReloadTo) { | ||
| triggerLocalReload(); | ||
| _internalVersion.wait(0); | ||
| setGlobalVersion(versionBeforeReload + 1); | ||
| while (_internalVersion.load(std::memory_order_acquire) < globalVersion()) { | ||
| _internalVersion.wait(versionBeforeReload); | ||
| } |
There was a problem hiding this comment.
I think this code warrants a comment that describes exactly what guarantees this gives, and why, by describing the order of events this imposes.
Changes the sleep in the update to be interruptible.
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
Checklist
Related Information
(Please reference tickets / specification / other PRs etc)