From 8b28561c5b2b240c3e9f9ee20bc6d74ed8edc57b Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 11 Dec 2018 15:32:21 +0100 Subject: [PATCH 1/4] fix invalid handling of `_lastValue` in case of multiple coordinators --- arangod/VocBase/KeyGenerator.cpp | 196 ++++++++++++++++++++----------- arangod/VocBase/KeyGenerator.h | 2 + 2 files changed, 131 insertions(+), 67 deletions(-) diff --git a/arangod/VocBase/KeyGenerator.cpp b/arangod/VocBase/KeyGenerator.cpp index 77190ad4e9b5..0e8a871c7f96 100644 --- a/arangod/VocBase/KeyGenerator.cpp +++ b/arangod/VocBase/KeyGenerator.cpp @@ -132,33 +132,23 @@ class TraditionalKeyGenerator : public KeyGenerator { public: /// @brief create the generator explicit TraditionalKeyGenerator(bool allowUserKeys) - : KeyGenerator(allowUserKeys), - _lastValue(0) {} + : KeyGenerator(allowUserKeys) {} bool hasDynamicState() const override { return true; } - + /// @brief generate a key std::string generate() override final { uint64_t tick = generateValue(); - { - MUTEX_LOCKER(mutexLocker, _lock); - - if (tick == UINT64_MAX || _lastValue >= UINT64_MAX - 1ULL) { - // oops, out of keys! - return std::string(); - } - - if (tick <= _lastValue) { - tick = ++_lastValue; - } else { - _lastValue = tick; - } + if (ADB_UNLIKELY(tick == 0)) { + // unlikely case we have run out of keys + // returning an empty string will trigger an error on the call site + return std::string(); } return arangodb::basics::StringUtils::itoa(tick); } - + /// @brief validate a key int validate(char const* p, size_t length, bool isRestore) override { int res = KeyGenerator::validate(p, length, isRestore); @@ -180,12 +170,7 @@ class TraditionalKeyGenerator : public KeyGenerator { uint64_t value = NumberUtils::atoi_zero(p, p + length); if (value > 0) { - MUTEX_LOCKER(mutexLocker, _lock); - - if (value > _lastValue) { - // and update our last value - _lastValue = value; - } + track(value); } } } @@ -194,18 +179,14 @@ class TraditionalKeyGenerator : public KeyGenerator { void toVelocyPack(arangodb::velocypack::Builder& builder) const override { KeyGenerator::toVelocyPack(builder); builder.add("type", VPackValue("traditional")); - - MUTEX_LOCKER(mutexLocker, _lock); - builder.add(StaticStrings::LastValue, VPackValue(_lastValue)); } protected: + /// @brief generate a new key value (internal) virtual uint64_t generateValue() = 0; - private: - mutable arangodb::Mutex _lock; - - uint64_t _lastValue; + /// @brief track a value (internal) + virtual void track(uint64_t value) = 0; }; /// @brief traditional key generator for a single server @@ -213,13 +194,61 @@ class TraditionalKeyGeneratorSingle final : public TraditionalKeyGenerator { public: /// @brief create the generator explicit TraditionalKeyGeneratorSingle(bool allowUserKeys) - : TraditionalKeyGenerator(allowUserKeys) {} + : TraditionalKeyGenerator(allowUserKeys), _lastValue(0) {} + + /// @brief build a VelocyPack representation of the generator in the builder + void toVelocyPack(arangodb::velocypack::Builder& builder) const override { + TraditionalKeyGenerator::toVelocyPack(builder); + + // add our specific stuff + MUTEX_LOCKER(mutexLocker, _lock); + builder.add(StaticStrings::LastValue, VPackValue(_lastValue)); + } private: - /// @brief generate a key + /// @brief generate a key value (internal) uint64_t generateValue() override { - return TRI_NewTickServer(); + uint64_t tick = TRI_NewTickServer(); + + if (ADB_UNLIKELY(tick == UINT64_MAX)) { + // out of keys + return 0; + } + + // keep track of last assigned value, and make sure the value + // we hand out is always higher than it + { + MUTEX_LOCKER(mutexLocker, _lock); + + if (ADB_UNLIKELY(_lastValue >= UINT64_MAX - 1ULL)) { + // oops, out of keys! + return 0; + } + + if (tick <= _lastValue) { + tick = ++_lastValue; + } else { + _lastValue = tick; + } + } + + return tick; + } + + /// @brief track a key value (internal) + void track(uint64_t value) override { + MUTEX_LOCKER(mutexLocker, _lock); + + if (value > _lastValue) { + // and update our last value + _lastValue = value; + } } + + private: + mutable arangodb::Mutex _lock; + + uint64_t _lastValue; }; /// @brief traditional key generator for a coordinator @@ -228,13 +257,16 @@ class TraditionalKeyGeneratorCluster final : public TraditionalKeyGenerator { /// @brief create the generator explicit TraditionalKeyGeneratorCluster(bool allowUserKeys) : TraditionalKeyGenerator(allowUserKeys) {} - + private: - /// @brief generate a key + /// @brief generate a key value (internal) uint64_t generateValue() override { ClusterInfo* ci = ClusterInfo::instance(); return ci->uniqid(); } + + /// @brief track a key value (internal) + void track(uint64_t /* value */) override {} }; /// @brief base class for padded key generators @@ -242,28 +274,18 @@ class PaddedKeyGenerator : public KeyGenerator { public: /// @brief create the generator explicit PaddedKeyGenerator(bool allowUserKeys) - : KeyGenerator(allowUserKeys), - _lastValue(0) {} + : KeyGenerator(allowUserKeys) {} bool hasDynamicState() const override { return true; } /// @brief generate a key std::string generate() override { uint64_t tick = generateValue(); - - { - MUTEX_LOCKER(mutexLocker, _lock); - - if (tick == UINT64_MAX || _lastValue >= UINT64_MAX - 1ULL) { - // oops, out of keys! - return std::string(); - } - if (tick <= _lastValue) { - tick = ++_lastValue; - } else { - _lastValue = tick; - } + if (ADB_UNLIKELY(tick == 0)) { + // unlikely case we have run out of keys + // returning an empty string will trigger an error on the call site + return std::string(); } return encode(tick); @@ -287,12 +309,7 @@ class PaddedKeyGenerator : public KeyGenerator { // check the numeric key part uint64_t value = decode(p, length); if (value > 0) { - MUTEX_LOCKER(mutexLocker, _lock); - - if (value > _lastValue) { - // and update our last value - _lastValue = value; - } + track(value); } } @@ -300,13 +317,14 @@ class PaddedKeyGenerator : public KeyGenerator { void toVelocyPack(arangodb::velocypack::Builder& builder) const override { KeyGenerator::toVelocyPack(builder); builder.add("type", VPackValue("padded")); - - MUTEX_LOCKER(mutexLocker, _lock); - builder.add(StaticStrings::LastValue, VPackValue(_lastValue)); } protected: + /// @brief generate a key value (internal) virtual uint64_t generateValue() = 0; + + /// @brief track a value (internal) + virtual void track(uint64_t value) = 0; private: uint64_t decode(char const* p, size_t length) { @@ -360,11 +378,6 @@ class PaddedKeyGenerator : public KeyGenerator { return std::string(&buffer[0], sizeof(uint64_t) * 2); } - - private: - mutable arangodb::Mutex _lock; - - uint64_t _lastValue; }; /// @brief padded key generator for a single server @@ -372,13 +385,59 @@ class PaddedKeyGeneratorSingle final : public PaddedKeyGenerator { public: /// @brief create the generator explicit PaddedKeyGeneratorSingle(bool allowUserKeys) - : PaddedKeyGenerator(allowUserKeys) {} + : PaddedKeyGenerator(allowUserKeys), _lastValue(0) {} + + /// @brief build a VelocyPack representation of the generator in the builder + void toVelocyPack(arangodb::velocypack::Builder& builder) const override { + PaddedKeyGenerator::toVelocyPack(builder); + + // add our own specific values + MUTEX_LOCKER(mutexLocker, _lock); + builder.add(StaticStrings::LastValue, VPackValue(_lastValue)); + } private: /// @brief generate a key uint64_t generateValue() override { - return TRI_NewTickServer(); + uint64_t tick = TRI_NewTickServer(); + + if (ADB_UNLIKELY(tick == UINT64_MAX)) { + // oops, out of keys! + return 0; + } + + { + MUTEX_LOCKER(mutexLocker, _lock); + + if (ADB_UNLIKELY(_lastValue >= UINT64_MAX - 1ULL)) { + // oops, out of keys! + return 0; + } + + if (tick <= _lastValue) { + tick = ++_lastValue; + } else { + _lastValue = tick; + } + } + + return tick; + } + + /// @brief generate a key value (internal) + void track(uint64_t value) override { + MUTEX_LOCKER(mutexLocker, _lock); + + if (value > _lastValue) { + // and update our last value + _lastValue = value; + } } + + private: + mutable arangodb::Mutex _lock; + + uint64_t _lastValue; }; /// @brief padded key generator for a coordinator @@ -389,11 +448,14 @@ class PaddedKeyGeneratorCluster final : public PaddedKeyGenerator { : PaddedKeyGenerator(allowUserKeys) {} private: - /// @brief generate a key + /// @brief generate a key value (internal) uint64_t generateValue() override { ClusterInfo* ci = ClusterInfo::instance(); return ci->uniqid(); } + + /// @brief generate a key value (internal) + void track(uint64_t /* value */) override {} }; /// @brief auto-increment key generator - not usable in cluster diff --git a/arangod/VocBase/KeyGenerator.h b/arangod/VocBase/KeyGenerator.h index c0f0af524771..e46aca45620f 100644 --- a/arangod/VocBase/KeyGenerator.h +++ b/arangod/VocBase/KeyGenerator.h @@ -55,6 +55,8 @@ class KeyGenerator { virtual bool hasDynamicState() const { return true; } /// @brief generate a key + /// if the returned string is empty, it means no proper key was + /// generated, and the caller must handle the situation virtual std::string generate() = 0; /// @brief validate a key From a802c634e017e267a8f08bf760ff995e07cf39a8 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 11 Dec 2018 19:15:40 +0100 Subject: [PATCH 2/4] attempt to fix tests --- arangod/VocBase/KeyGenerator.cpp | 28 +++++++++++++++++++++++---- arangod/VocBase/KeyGenerator.h | 8 ++++++++ arangod/VocBase/LogicalCollection.h | 10 +++++++++- tests/js/common/shell/shell-keygen.js | 10 +++++++--- 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/arangod/VocBase/KeyGenerator.cpp b/arangod/VocBase/KeyGenerator.cpp index 0e8a871c7f96..cc91d1acb033 100644 --- a/arangod/VocBase/KeyGenerator.cpp +++ b/arangod/VocBase/KeyGenerator.cpp @@ -194,7 +194,9 @@ class TraditionalKeyGeneratorSingle final : public TraditionalKeyGenerator { public: /// @brief create the generator explicit TraditionalKeyGeneratorSingle(bool allowUserKeys) - : TraditionalKeyGenerator(allowUserKeys), _lastValue(0) {} + : TraditionalKeyGenerator(allowUserKeys), _lastValue(0) { + TRI_ASSERT(!ServerState::instance()->isCoordinator()); + } /// @brief build a VelocyPack representation of the generator in the builder void toVelocyPack(arangodb::velocypack::Builder& builder) const override { @@ -252,11 +254,19 @@ class TraditionalKeyGeneratorSingle final : public TraditionalKeyGenerator { }; /// @brief traditional key generator for a coordinator +/// please note that coordinator-based key generators are frequently +/// created and discarded, so ctor & dtor need to be very efficient. +/// additionally, do not put any state into this object, as for the +/// same logical collection the ClusterInfo may create many different +/// temporary LogicalCollection objects one after the other, which +/// will also discard the collection's particular KeyGenerator object! class TraditionalKeyGeneratorCluster final : public TraditionalKeyGenerator { public: /// @brief create the generator explicit TraditionalKeyGeneratorCluster(bool allowUserKeys) - : TraditionalKeyGenerator(allowUserKeys) {} + : TraditionalKeyGenerator(allowUserKeys) { + TRI_ASSERT(ServerState::instance()->isCoordinator()); + } private: /// @brief generate a key value (internal) @@ -385,7 +395,9 @@ class PaddedKeyGeneratorSingle final : public PaddedKeyGenerator { public: /// @brief create the generator explicit PaddedKeyGeneratorSingle(bool allowUserKeys) - : PaddedKeyGenerator(allowUserKeys), _lastValue(0) {} + : PaddedKeyGenerator(allowUserKeys), _lastValue(0) { + TRI_ASSERT(!ServerState::instance()->isCoordinator()); + } /// @brief build a VelocyPack representation of the generator in the builder void toVelocyPack(arangodb::velocypack::Builder& builder) const override { @@ -441,11 +453,19 @@ class PaddedKeyGeneratorSingle final : public PaddedKeyGenerator { }; /// @brief padded key generator for a coordinator +/// please note that coordinator-based key generators are frequently +/// created and discarded, so ctor & dtor need to be very efficient. +/// additionally, do not put any state into this object, as for the +/// same logical collection the ClusterInfo may create many different +/// temporary LogicalCollection objects one after the other, which +/// will also discard the collection's particular KeyGenerator object! class PaddedKeyGeneratorCluster final : public PaddedKeyGenerator { public: /// @brief create the generator explicit PaddedKeyGeneratorCluster(bool allowUserKeys) - : PaddedKeyGenerator(allowUserKeys) {} + : PaddedKeyGenerator(allowUserKeys) { + TRI_ASSERT(ServerState::instance()->isCoordinator()); + } private: /// @brief generate a key value (internal) diff --git a/arangod/VocBase/KeyGenerator.h b/arangod/VocBase/KeyGenerator.h index e46aca45620f..3c45e39267e3 100644 --- a/arangod/VocBase/KeyGenerator.h +++ b/arangod/VocBase/KeyGenerator.h @@ -35,6 +35,14 @@ class Builder; class Slice; } +/// generic key generator interface +/// +/// please note that coordinator-based key generators are frequently +/// created and discarded, so ctor & dtor need to be very efficient. +/// additionally, do not put any state into this object, as for the +/// same logical collection the ClusterInfo may create many different +/// temporary LogicalCollection objects one after the other, which +/// will also discard the collection's particular KeyGenerator object! class KeyGenerator { KeyGenerator(KeyGenerator const&) = delete; KeyGenerator& operator=(KeyGenerator const&) = delete; diff --git a/arangod/VocBase/LogicalCollection.h b/arangod/VocBase/LogicalCollection.h index 663636c325a7..37b0c9647d40 100644 --- a/arangod/VocBase/LogicalCollection.h +++ b/arangod/VocBase/LogicalCollection.h @@ -75,6 +75,14 @@ class ChecksumResult : public Result { velocypack::Builder _builder; }; +/// please note that coordinator-based logical collections are frequently +/// created and discarded, so ctor & dtor need to be as efficient as possible. +/// additionally, do not put any volatile state into this object in the coordinator, +/// as the ClusterInfo may create many different temporary physical LogicalCollection +/// objects (one after the other) even for the same "logical" LogicalCollection. +/// this which will also discard the collection's volatile state each time! +/// all state of a LogicalCollection in the coordinator case needs to be derived +/// from the JSON info in the agency's plan entry for the collection... class LogicalCollection : public LogicalDataSource { friend struct ::TRI_vocbase_t; @@ -424,4 +432,4 @@ class LogicalCollection : public LogicalDataSource { } // namespace arangodb -#endif \ No newline at end of file +#endif diff --git a/tests/js/common/shell/shell-keygen.js b/tests/js/common/shell/shell-keygen.js index 0112e5e79bad..e7283ea1c147 100644 --- a/tests/js/common/shell/shell-keygen.js +++ b/tests/js/common/shell/shell-keygen.js @@ -273,7 +273,9 @@ function TraditionalSuite () { lastKey = key; } - assertEqual(lastKey, c.properties().keyOptions.lastValue); + if (!cluster || !cluster.isCluster || !cluster.isCluster()) { + assertEqual(lastKey, c.properties().keyOptions.lastValue); + } }, testPaddedTracking : function () { @@ -293,8 +295,10 @@ function TraditionalSuite () { lastKey = key; } - let hex = c.properties().keyOptions.lastValue.toString(16); - assertEqual(lastKey, Array(16 + 1 - hex.length).join("0") + hex); + if (!cluster || !cluster.isCluster || !cluster.isCluster()) { + let hex = c.properties().keyOptions.lastValue.toString(16); + assertEqual(lastKey, Array(16 + 1 - hex.length).join("0") + hex); + } } }; From addc07ddbbcb4ffb80915d8a7f0e8b18b886286b Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 11 Dec 2018 19:34:17 +0100 Subject: [PATCH 3/4] fix tests --- tests/js/common/shell/shell-keygen.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/js/common/shell/shell-keygen.js b/tests/js/common/shell/shell-keygen.js index e7283ea1c147..d7a264a4aa95 100644 --- a/tests/js/common/shell/shell-keygen.js +++ b/tests/js/common/shell/shell-keygen.js @@ -273,7 +273,7 @@ function TraditionalSuite () { lastKey = key; } - if (!cluster || !cluster.isCluster || !cluster.isCluster()) { + if (!cluster && !cluster.isCluster && !cluster.isCluster()) { assertEqual(lastKey, c.properties().keyOptions.lastValue); } }, @@ -295,7 +295,7 @@ function TraditionalSuite () { lastKey = key; } - if (!cluster || !cluster.isCluster || !cluster.isCluster()) { + if (!cluster && !cluster.isCluster && !cluster.isCluster()) { let hex = c.properties().keyOptions.lastValue.toString(16); assertEqual(lastKey, Array(16 + 1 - hex.length).join("0") + hex); } From 2492ef45ca2ea4da94e7c5cb7132c589cac4d999 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Wed, 12 Dec 2018 10:45:35 +0100 Subject: [PATCH 4/4] make JS conditions simpler to understand --- tests/js/common/shell/shell-keygen.js | 35 ++++++++++++++------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/tests/js/common/shell/shell-keygen.js b/tests/js/common/shell/shell-keygen.js index d7a264a4aa95..989b68777e3d 100644 --- a/tests/js/common/shell/shell-keygen.js +++ b/tests/js/common/shell/shell-keygen.js @@ -1,5 +1,5 @@ /*jshint globalstrict:false, strict:false */ -/*global assertEqual, assertTrue, assertEqual, assertMatch, fail, ArangoClusterComm */ +/*global arango, assertEqual, assertTrue, assertEqual, assertMatch, fail, ArangoClusterComm */ //////////////////////////////////////////////////////////////////////////////// /// @brief test the traditional key generators @@ -33,12 +33,17 @@ var jsunity = require("jsunity"); var arangodb = require("@arangodb"); var db = arangodb.db; var ERRORS = arangodb.errors; -let cluster; +let cluster = false; // default to false // quick hack to check if we are arangod or arangosh if (typeof ArangoClusterComm === "object") { - cluster = require("@arangodb/cluster"); + // arangod + cluster = require("@arangodb/cluster").isCluster(); } else { - cluster = {}; + // arangosh + if (arango) { + let role = arango.GET("/_admin/server/role").role; + cluster = (role === "COORDINATOR"); + } } //////////////////////////////////////////////////////////////////////////////// @@ -69,15 +74,14 @@ function TraditionalSuite () { try { c.save({ _key: "1234" }); // no user keys allowed fail(); - } - catch (err) { + } catch (err) { assertTrue(err.errorNum === ERRORS.ERROR_ARANGO_DOCUMENT_KEY_UNEXPECTED.code || err.errorNum === ERRORS.ERROR_CLUSTER_MUST_NOT_SPECIFY_KEY.code); } }, testCreateInvalidKeyNonDefaultSharding2 : function () { - if (!cluster || !cluster.isCluster || !cluster.isCluster()) { + if (!cluster) { return; } @@ -86,15 +90,14 @@ function TraditionalSuite () { try { c.save({ _key: "1234" }); // no user keys allowed fail(); - } - catch (err) { + } catch (err) { assertTrue(err.errorNum === ERRORS.ERROR_ARANGO_DOCUMENT_KEY_UNEXPECTED.code || err.errorNum === ERRORS.ERROR_CLUSTER_MUST_NOT_SPECIFY_KEY.code); } }, testCreateKeyNonDefaultSharding : function () { - if (!cluster || !cluster.isCluster || !cluster.isCluster()) { + if (!cluster) { return; } @@ -115,8 +118,7 @@ function TraditionalSuite () { try { c.save({ _key: "1234" }); // no user keys allowed fail(); - } - catch (err) { + } catch (err) { assertTrue(err.errorNum === ERRORS.ERROR_ARANGO_DOCUMENT_KEY_UNEXPECTED.code || err.errorNum === ERRORS.ERROR_CLUSTER_MUST_NOT_SPECIFY_KEY.code); } @@ -132,8 +134,7 @@ function TraditionalSuite () { try { c.save({ _key: "öä .mds 3 -6" }); // invalid key fail(); - } - catch (err) { + } catch (err) { assertEqual(ERRORS.ERROR_ARANGO_DOCUMENT_KEY_BAD.code, err.errorNum); } }, @@ -218,7 +219,7 @@ function TraditionalSuite () { }, testAutoincrementGeneratorInCluster : function () { - if (!cluster || !cluster.isCluster || !cluster.isCluster()) { + if (!cluster) { return; } @@ -273,7 +274,7 @@ function TraditionalSuite () { lastKey = key; } - if (!cluster && !cluster.isCluster && !cluster.isCluster()) { + if (!cluster) { assertEqual(lastKey, c.properties().keyOptions.lastValue); } }, @@ -295,7 +296,7 @@ function TraditionalSuite () { lastKey = key; } - if (!cluster && !cluster.isCluster && !cluster.isCluster()) { + if (!cluster) { let hex = c.properties().keyOptions.lastValue.toString(16); assertEqual(lastKey, Array(16 + 1 - hex.length).join("0") + hex); }