From b43b600a338bc6defe51d383eb1b51d90aecd27f Mon Sep 17 00:00:00 2001 From: Jan Date: Thu, 6 Dec 2018 16:36:05 +0100 Subject: [PATCH] try again in case of a conflict when updating users (#7615) --- arangod/Auth/UserManager.cpp | 6 ++- arangod/VocBase/Methods/Collections.cpp | 71 ++++++++++++++++++++----- 2 files changed, 61 insertions(+), 16 deletions(-) diff --git a/arangod/Auth/UserManager.cpp b/arangod/Auth/UserManager.cpp index 130623f849..ed54140ea7 100644 --- a/arangod/Auth/UserManager.cpp +++ b/arangod/Auth/UserManager.cpp @@ -334,7 +334,8 @@ Result auth::UserManager::storeUserInternal(auth::User const& entry, bool replac } #endif } else if (res.is(TRI_ERROR_ARANGO_CONFLICT)) { // user was outdated - _userCache.erase(entry.username()); + // we didn't succeed in updating the user, so we must not remove the + // user from the cache here. however, we should trigger a reload here triggerLocalReload(); LOG_TOPIC(WARN, Logger::AUTHENTICATION) << "Cannot update user due to conflict"; @@ -562,6 +563,7 @@ Result auth::UserManager::accessUser(std::string const& user, } loadFromDB(); + READ_LOCKER(readGuard, _userCacheLock); UserMap::iterator const& it = _userCache.find(user); if (it != _userCache.end()) { @@ -574,8 +576,8 @@ bool auth::UserManager::userExists(std::string const& user) { if (user.empty()) { return false; } + loadFromDB(); - READ_LOCKER(readGuard, _userCacheLock); UserMap::iterator const& it = _userCache.find(user); return it != _userCache.end(); diff --git a/arangod/VocBase/Methods/Collections.cpp b/arangod/VocBase/Methods/Collections.cpp index b82ee5d817..7541cf2baa 100644 --- a/arangod/VocBase/Methods/Collections.cpp +++ b/arangod/VocBase/Methods/Collections.cpp @@ -154,8 +154,7 @@ Result methods::Collections::lookup(TRI_vocbase_t* vocbase, func(coll); return Result(); - } - else { + } else { return Result(TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND); } } catch (basics::Exception const& ex) { @@ -267,10 +266,25 @@ Result Collections::create(TRI_vocbase_t* vocbase, std::string const& name, if (name[0] != '_' && um != nullptr && exe != nullptr && !exe->isSuperuser()) { // this should not fail, we can not get here without database RW access - um->updateUser(ExecContext::CURRENT->user(), [&](auth::User& entry) { - entry.grantCollection(vocbase->name(), name, auth::Level::RW); - return TRI_ERROR_NO_ERROR; - }); + // however, there may be races for updating the users account, so we try a + // few times in case of a conflict + int tries = 0; + while (true) { + Result r = um->updateUser(ExecContext::CURRENT->user(), [&](auth::User& entry) { + entry.grantCollection(vocbase->name(), name, auth::Level::RW); + return TRI_ERROR_NO_ERROR; + }); + if (r.ok() || r.is(TRI_ERROR_USER_NOT_FOUND)) { + // it seems to be allowed to created collections with an unknown user + break; + } + if (!r.is(TRI_ERROR_ARANGO_CONFLICT) || ++tries == 10) { + LOG_TOPIC(WARN, Logger::FIXME) << "Updating user failed with error: " << r.errorMessage() << ". giving up!"; + return r; + } + // try again in case of conflict + LOG_TOPIC(TRACE, Logger::FIXME) << "Updating user failed with error: " << r.errorMessage() << ". trying again"; + } } // reload otherwise collection might not be in yet @@ -286,10 +300,25 @@ Result Collections::create(TRI_vocbase_t* vocbase, std::string const& name, if (name[0] != '_' && um != nullptr && exe != nullptr && !exe->isSuperuser()) { // this should not fail, we can not get here without database RW access - um->updateUser(ExecContext::CURRENT->user(), [&](auth::User& u) { - u.grantCollection(vocbase->name(), name, auth::Level::RW); - return TRI_ERROR_NO_ERROR; - }); + // however, there may be races for updating the users account, so we try a + // few times in case of a conflict + int tries = 0; + while (true) { + Result r = um->updateUser(ExecContext::CURRENT->user(), [&](auth::User& entry) { + entry.grantCollection(vocbase->name(), name, auth::Level::RW); + return TRI_ERROR_NO_ERROR; + }); + if (r.ok() || r.is(TRI_ERROR_USER_NOT_FOUND)) { + // it seems to be allowed to created collections with an unknown user + break; + } + if (!r.is(TRI_ERROR_ARANGO_CONFLICT) || ++tries == 10) { + LOG_TOPIC(WARN, Logger::FIXME) << "Updating user failed with error: " << r.errorMessage() << ". giving up!"; + return r; + } + // try again in case of conflict + LOG_TOPIC(TRACE, Logger::FIXME) << "Updating user failed with error: " << r.errorMessage() << ". trying again"; + } } func(col); @@ -567,7 +596,7 @@ Result Collections::drop(TRI_vocbase_t* vocbase, LogicalCollection* coll, } TRI_ASSERT(coll); - auto& dbname = coll->vocbase().name(); + auto const& dbname = coll->vocbase().name(); std::string const collName = coll->name(); Result res; @@ -590,9 +619,23 @@ Result Collections::drop(TRI_vocbase_t* vocbase, LogicalCollection* coll, auth::UserManager* um = AuthenticationFeature::instance()->userManager(); if (res.ok() && um != nullptr) { - um->enumerateUsers([&](auth::User& entry) -> bool { - return entry.removeCollection(dbname, collName); - }); + int tries = 0; + while (true) { + res = um->enumerateUsers([&](auth::User& entry) -> bool { + return entry.removeCollection(dbname, collName); + }); + + if (res.ok() || !res.is(TRI_ERROR_ARANGO_CONFLICT)) { + break; + } + + if (++tries == 10) { + LOG_TOPIC(WARN, Logger::FIXME) << "Enumerating users failed with " << res.errorMessage() << ". giving up!"; + break; + } + // try again in case of conflict + LOG_TOPIC(TRACE, Logger::FIXME) << "Enumerating users failed with error: " << res.errorMessage() << ". trying again"; + } } return res; }