From 7a991c82ccd22b6aa3190f8df550292e810db1ee Mon Sep 17 00:00:00 2001 From: Markus Pfeiffer Date: Thu, 19 Mar 2020 08:08:50 +0000 Subject: [PATCH 1/7] Reset traversal on construction --- arangod/Aql/TraversalExecutor.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/arangod/Aql/TraversalExecutor.cpp b/arangod/Aql/TraversalExecutor.cpp index 58e917dde61f..b02ea9e81a1f 100644 --- a/arangod/Aql/TraversalExecutor.cpp +++ b/arangod/Aql/TraversalExecutor.cpp @@ -145,7 +145,14 @@ std::vector> const& TraversalExecutorInfo } TraversalExecutor::TraversalExecutor(Fetcher& fetcher, Infos& infos) - : _infos(infos), _inputRow{CreateInvalidInputRowHint{}}, _traverser(infos.traverser()) {} + : _infos(infos), _inputRow{CreateInvalidInputRowHint{}}, _traverser(infos.traverser()) { + + // reset the traverser, so that no residual state is left in it. This is + // important because the TraversalExecutor is sometimes reconstructed (in place) + // with the same TraversalExecutorInfos as before. Those infos contain the traverser + // which might contain state from a previous run. + _traverser.done(); +} TraversalExecutor::~TraversalExecutor() { auto opts = _traverser.options(); @@ -176,7 +183,6 @@ std::pair TraversalExecutor::produceRows(OutputA } auto TraversalExecutor::doOutput(OutputAqlItemRow& output) -> void { - // TODO check whether _traverser.hasMore is obsolete here while (!output.isFull() && _traverser.hasMore() && _traverser.next()) { TRI_ASSERT(_inputRow.isInitialized()); @@ -226,7 +232,6 @@ auto TraversalExecutor::produceRows(AqlItemBlockInputRange& input, OutputAqlItem while (true) { if (_traverser.hasMore()) { - TRI_ASSERT(_inputRow.isInitialized()); doOutput(output); if (output.isFull()) { From 6c742438955657275c16079b962f27755e498286 Mon Sep 17 00:00:00 2001 From: Markus Pfeiffer Date: Thu, 19 Mar 2020 11:56:43 +0000 Subject: [PATCH 2/7] Reset path enumerator properly --- arangod/Graph/PathEnumerator.cpp | 34 ++++++++++++++++---------------- arangod/Graph/PathEnumerator.h | 18 ++++++++--------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/arangod/Graph/PathEnumerator.cpp b/arangod/Graph/PathEnumerator.cpp index c1ce3543d934..654fe65558d0 100644 --- a/arangod/Graph/PathEnumerator.cpp +++ b/arangod/Graph/PathEnumerator.cpp @@ -38,27 +38,24 @@ using Traverser = arangodb::traverser::Traverser; using TraverserOptions = arangodb::traverser::TraverserOptions; PathEnumerator::PathEnumerator(Traverser* traverser, TraverserOptions* opts) - : _traverser(traverser), - _isFirst(true), - _opts(opts), - _httpRequests(0) {} + : _traverser(traverser), _isFirst(true), _opts(opts), _httpRequests(0) {} PathEnumerator::~PathEnumerator() = default; void PathEnumerator::setStartVertex(arangodb::velocypack::StringRef startVertex) { _enumeratedPath.edges.clear(); _enumeratedPath.vertices.clear(); - _isFirst = true; + _isFirst = true; _httpRequests = 0; - + _enumeratedPath.vertices.push_back(startVertex); TRI_ASSERT(_enumeratedPath.vertices.size() == 1); } bool PathEnumerator::keepEdge(arangodb::graph::EdgeDocumentToken& eid, arangodb::velocypack::Slice edge, - arangodb::velocypack::StringRef sourceVertex, size_t depth, - size_t cursorId) { + arangodb::velocypack::StringRef sourceVertex, + size_t depth, size_t cursorId) { if (_opts->hasEdgeFilter(depth, cursorId)) { VPackSlice e = edge; if (edge.isString()) { @@ -73,7 +70,8 @@ bool PathEnumerator::keepEdge(arangodb::graph::EdgeDocumentToken& eid, return _opts->destinationCollectionAllowed(edge, sourceVertex); } -graph::EdgeCursor* PathEnumerator::getCursor(arangodb::velocypack::StringRef nextVertex, uint64_t currentDepth) { +graph::EdgeCursor* PathEnumerator::getCursor(arangodb::velocypack::StringRef nextVertex, + uint64_t currentDepth) { if (currentDepth >= _cursors.size()) { _cursors.emplace_back(_opts->buildCursor(currentDepth)); } @@ -83,15 +81,16 @@ graph::EdgeCursor* PathEnumerator::getCursor(arangodb::velocypack::StringRef nex } DepthFirstEnumerator::DepthFirstEnumerator(Traverser* traverser, TraverserOptions* opts) - : PathEnumerator(traverser, opts), - _activeCursors(0), - _pruneNext(false) {} + : PathEnumerator(traverser, opts), _activeCursors(0), _pruneNext(false) {} DepthFirstEnumerator::~DepthFirstEnumerator() = default; void DepthFirstEnumerator::setStartVertex(arangodb::velocypack::StringRef startVertex) { PathEnumerator::setStartVertex(startVertex); + _enumeratedPath.edges.clear(); + _enumeratedPath.vertices.clear(); + _activeCursors = 0; _pruneNext = false; } @@ -114,7 +113,9 @@ bool DepthFirstEnumerator::next() { if (_enumeratedPath.edges.size() < _opts->maxDepth && !_pruneNext) { // We are not done with this path, so // we reserve the cursor for next depth - graph::EdgeCursor* cursor = getCursor(arangodb::velocypack::StringRef(_enumeratedPath.vertices.back()), _enumeratedPath.edges.size()); + graph::EdgeCursor* cursor = + getCursor(arangodb::velocypack::StringRef(_enumeratedPath.vertices.back()), + _enumeratedPath.edges.size()); incHttpRequests(cursor->httpRequests()); ++_activeCursors; } else { @@ -131,7 +132,7 @@ bool DepthFirstEnumerator::next() { auto callback = [&](graph::EdgeDocumentToken&& eid, VPackSlice const& edge, size_t cursorId) { if (!keepEdge(eid, edge, arangodb::velocypack::StringRef(_enumeratedPath.vertices.back()), - _enumeratedPath.edges.size(), cursorId)) { + _enumeratedPath.edges.size(), cursorId)) { return; } @@ -257,13 +258,13 @@ bool DepthFirstEnumerator::shouldPrune() { if (!_opts->usesPrune()) { return false; } - + transaction::BuilderLeaser pathBuilder(_opts->trx()); // evaluator->evaluate() might access these, so they have to live long // enough aql::AqlValue vertex, edge; aql::AqlValueGuard vertexGuard{vertex, true}, edgeGuard{edge, true}; - + aql::PruneExpressionEvaluator* evaluator = _opts->getPruneEvaluator(); TRI_ASSERT(evaluator != nullptr); @@ -281,4 +282,3 @@ bool DepthFirstEnumerator::shouldPrune() { } return evaluator->evaluate(); } - diff --git a/arangod/Graph/PathEnumerator.h b/arangod/Graph/PathEnumerator.h index 09ae14d864f4..bddb40740908 100644 --- a/arangod/Graph/PathEnumerator.h +++ b/arangod/Graph/PathEnumerator.h @@ -82,7 +82,7 @@ class PathEnumerator { ////////////////////////////////////////////////////////////////////////////// EnumeratedPath _enumeratedPath; - + ////////////////////////////////////////////////////////////////////////////// /// @brief Number of HTTP requests made ////////////////////////////////////////////////////////////////////////////// @@ -96,7 +96,7 @@ class PathEnumerator { PathEnumerator(Traverser* traverser, TraverserOptions* opts); virtual ~PathEnumerator(); - + /// @brief set start vertex and reset /// note that the caller *must* guarantee that the string data pointed to by /// startVertex remains valid even after the call to reset()!! @@ -113,19 +113,19 @@ class PathEnumerator { virtual aql::AqlValue lastVertexToAqlValue() = 0; virtual aql::AqlValue lastEdgeToAqlValue() = 0; virtual aql::AqlValue pathToAqlValue(arangodb::velocypack::Builder&) = 0; - + /// @brief return number of HTTP requests made, and reset it to 0 - size_t getAndResetHttpRequests() { + size_t getAndResetHttpRequests() { size_t value = _httpRequests; _httpRequests = 0; return value; } - + void incHttpRequests(size_t requests) { _httpRequests += requests; } - + protected: graph::EdgeCursor* getCursor(arangodb::velocypack::StringRef nextVertex, uint64_t currentDepth); - + /// @brief The vector of EdgeCursors to walk through. std::vector> _cursors; }; @@ -144,7 +144,7 @@ class DepthFirstEnumerator final : public PathEnumerator { DepthFirstEnumerator(Traverser* traverser, TraverserOptions* opts); ~DepthFirstEnumerator(); - + /// @brief set start vertex and reset void setStartVertex(arangodb::velocypack::StringRef startVertex) override; @@ -159,7 +159,7 @@ class DepthFirstEnumerator final : public PathEnumerator { private: bool shouldPrune(); - + velocypack::Slice pathToSlice(arangodb::velocypack::Builder& result); }; } // namespace traverser From 7eb114c090aa21d86ef53c6cf81adbedf52e93c0 Mon Sep 17 00:00:00 2001 From: Markus Pfeiffer Date: Thu, 19 Mar 2020 13:29:06 +0000 Subject: [PATCH 3/7] Cleanup --- arangod/Graph/PathEnumerator.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/arangod/Graph/PathEnumerator.cpp b/arangod/Graph/PathEnumerator.cpp index 654fe65558d0..d15a04bb7fd1 100644 --- a/arangod/Graph/PathEnumerator.cpp +++ b/arangod/Graph/PathEnumerator.cpp @@ -88,8 +88,6 @@ DepthFirstEnumerator::~DepthFirstEnumerator() = default; void DepthFirstEnumerator::setStartVertex(arangodb::velocypack::StringRef startVertex) { PathEnumerator::setStartVertex(startVertex); - _enumeratedPath.edges.clear(); - _enumeratedPath.vertices.clear(); _activeCursors = 0; _pruneNext = false; } From 291b40f5c7dad1159c86c6badd1a35b85acc923f Mon Sep 17 00:00:00 2001 From: Markus Pfeiffer Date: Thu, 19 Mar 2020 13:29:47 +0000 Subject: [PATCH 4/7] Add regression test for TraversalExecutor failure --- .../aql/aql-traversal-reset-regression.js | 283 ++++++++++++++++++ 1 file changed, 283 insertions(+) create mode 100644 tests/js/server/aql/aql-traversal-reset-regression.js diff --git a/tests/js/server/aql/aql-traversal-reset-regression.js b/tests/js/server/aql/aql-traversal-reset-regression.js new file mode 100644 index 000000000000..a59b5da6e975 --- /dev/null +++ b/tests/js/server/aql/aql-traversal-reset-regression.js @@ -0,0 +1,283 @@ +/*jshint globalstrict:false, strict:false, maxlen: 500 */ +/*global assertEqual, AQL_EXECUTE, assertTrue, fail */ + +//////////////////////////////////////////////////////////////////////////////// +/// @brief tests for regression returning blocks to the manager +/// +/// @file +/// +/// DISCLAIMER +/// +/// Copyright 2010-2014 triagens GmbH, Cologne, Germany +/// +/// Licensed under the Apache License, Version 2.0 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// http://www.apache.org/licenses/LICENSE-2.0 +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is triAGENS GmbH, Cologne, Germany +/// +/// @author Markus Pfeiffer +/// @author Copyright 2020, triAGENS GmbH, Cologne, Germany +//////////////////////////////////////////////////////////////////////////////// + +var jsunity = require("jsunity"); +var internal = require("internal"); +var errors = internal.errors; +var db = require("@arangodb").db, + indexId; + +// This example was produced by Jan Steeman to reproduce a +// crash in the TraversalExecutor code +const productCollectionName = "RegressionProduct"; +const customerCollectionName = "RegressionCustomer"; +const ownsEdgeCollectionName = "RegressionOwns"; + +var cleanup = function() { + db._drop(productCollectionName); + db._drop(customerCollectionName); + db._drop(ownsEdgeCollectionName); +}; + +var createBaseGraph = function() { + db._create(customerCollectionName, { + cacheEnabled: false, + globallyUniqueId: "h43A585CA1081/168", + isSmartChild: false, + isSystem: false, + keyOptions: { allowUserKeys: true, type: "traditional", lastValue: 0 }, + minRevision: 1661521530573553700, + objectId: "167", + statusString: "loaded", + usesRevisionsAsDocumentIds: true, + validation: null, + waitForSync: false, + writeConcern: 1 + }); + + db[customerCollectionName].ensureIndex({ + bestIndexedLevel: 17, + fields: ["location"], + geoJson: false, + id: "customer/173", + maxNumCoverCells: 8, + name: "idx_1661521530578796544", + sparse: true, + type: "geo", + unique: false, + worstIndexedLevel: 4 + }); + + db._create(productCollectionName, { + cacheEnabled: false, + globallyUniqueId: "h43A585CA1081/194", + isSmartChild: false, + isSystem: false, + keyOptions: { allowUserKeys: true, type: "traditional", lastValue: 0 }, + minRevision: 1661521530656391200, + objectId: "193", + statusString: "loaded", + usesRevisionsAsDocumentIds: true, + validation: null, + waitForSync: false, + writeConcern: 1 + }); + db[productCollectionName].ensureIndex({ + bestIndexedLevel: 17, + fields: ["location"], + geoJson: false, + id: "product/199", + maxNumCoverCells: 8, + name: "idx_1661521530657439744", + sparse: true, + type: "geo", + unique: false, + worstIndexedLevel: 4 + }); + + db._createEdgeCollection(ownsEdgeCollectionName, { + cacheEnabled: false, + globallyUniqueId: "h43A585CA1081/218", + isSmartChild: false, + isSystem: false, + keyOptions: { allowUserKeys: true, type: "traditional", lastValue: 0 }, + minRevision: 1661521530664779800, + objectId: "217", + statusString: "loaded", + usesRevisionsAsDocumentIds: true, + validation: null, + waitForSync: false, + writeConcern: 1 + }); + + db[customerCollectionName].insert([ + { + _key: "396", + _id: customerCollectionName + "/396", + _rev: "_aM4ZTRW---", + name: "John", + surname: "Smith", + age: 52, + alive: false, + _class: "com.arangodb.springframework.testdata.Customer" + }, + { + _key: "397", + _id: customerCollectionName + "/397", + _rev: "_aM4ZTRW--_", + name: "Matt", + surname: "Smith", + age: 34, + alive: false, + _class: "com.arangodb.springframework.testdata.Customer" + }, + { + _key: "398", + _id: customerCollectionName + "/398", + _rev: "_aM4ZTRW--A", + name: "Adam", + surname: "Smith", + age: 294, + alive: false, + _class: "com.arangodb.springframework.testdata.Customer" + } + ]); + db[ownsEdgeCollectionName].insert([ + { + _key: "400", + _id: ownsEdgeCollectionName + "/400", + _from: customerCollectionName + "/396", + _to: productCollectionName + "/390", + _rev: "_aM4ZTR2---", + _class: "com.arangodb.springframework.testdata.Owns" + }, + { + _key: "402", + _id: ownsEdgeCollectionName + "/402", + _from: customerCollectionName + "/396", + _to: productCollectionName + "/392", + _rev: "_aM4ZTR6---", + _class: "com.arangodb.springframework.testdata.Owns" + }, + { + _key: "404", + _id: ownsEdgeCollectionName + "/404", + _from: customerCollectionName + "/398", + _to: productCollectionName + "/394", + _rev: "_aM4ZTS----", + _class: "com.arangodb.springframework.testdata.Owns" + }, + { + _key: "406", + _id: ownsEdgeCollectionName + "/406", + _from: customerCollectionName + "/397", + _to: productCollectionName + "/390", + _rev: "_aM4ZTSC---", + _class: "com.arangodb.springframework.testdata.Owns" + }, + { + _key: "408", + _id: ownsEdgeCollectionName + "/408", + _from: customerCollectionName + "/397", + _to: productCollectionName + "/392", + _rev: "_aM4ZTSG---", + _class: "com.arangodb.springframework.testdata.Owns" + }, + { + _key: "410", + _id: ownsEdgeCollectionName + "/410", + _from: customerCollectionName + "/397", + _to: productCollectionName + "/394", + _rev: "_aM4ZTSG--_", + _class: "com.arangodb.springframework.testdata.Owns" + } + ]); + db[productCollectionName].insert([ + { + _key: "390", + _id: productCollectionName + "/390", + _rev: "_aM4ZTPy---", + name: "phone", + _class: "com.arangodb.springframework.testdata.Product" + }, + { + _key: "392", + _id: productCollectionName + "/392", + _rev: "_aM4ZTQG---", + name: "car", + _class: "com.arangodb.springframework.testdata.Product" + }, + { + _key: "394", + _id: productCollectionName + "/394", + _rev: "_aM4ZTQK---", + name: "chair", + _class: "com.arangodb.springframework.testdata.Product" + } + ]); +}; + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test suite +//////////////////////////////////////////////////////////////////////////////// + +function traversalResetRegressionSuite() { + return { + //////////////////////////////////////////////////////////////////////////////// + /// @brief set up + //////////////////////////////////////////////////////////////////////////////// + + setUpAll: function() { + cleanup(); + createBaseGraph(); + }, + + //////////////////////////////////////////////////////////////////////////////// + /// @brief tear down + //////////////////////////////////////////////////////////////////////////////// + + tearDownAll: function() { + cleanup(); + }, + + //////////////////////////////////////////////////////////////////////////////// + /// @brief test object access for path object + //////////////////////////////////////////////////////////////////////////////// + testTraversalResetCrashes: function() { + const query = `WITH @@product + FOR e IN @@customer + FILTER (FOR e1 IN 1..1 ANY e._id @@owns + FILTER e1.name == @0 + RETURN 1)[0] == 1 AND + (FOR e1 IN 1..1 ANY e._id @@owns + FILTER e1.name == @1 + RETURN 1)[0] == 1 + RETURN e`; + const bindVars = { + "0": "phone", + "1": "phone", + "@product": productCollectionName, + "@customer": customerCollectionName, + "@owns": ownsEdgeCollectionName + }; + + // var plan = db._explain(query, bindVars); + var actual = db._query(query, bindVars); + } + }; +} + +//////////////////////////////////////////////////////////////////////////////// +/// @brief executes the test suite +//////////////////////////////////////////////////////////////////////////////// + +jsunity.run(traversalResetRegressionSuite); + +return jsunity.done(); From 07f94474a12b09a25eb4e9a72824619b6cf41ef8 Mon Sep 17 00:00:00 2001 From: Markus Pfeiffer Date: Thu, 19 Mar 2020 13:50:08 +0000 Subject: [PATCH 5/7] Add back some asserts --- arangod/Aql/TraversalExecutor.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arangod/Aql/TraversalExecutor.cpp b/arangod/Aql/TraversalExecutor.cpp index b02ea9e81a1f..71cc7e4cd1a8 100644 --- a/arangod/Aql/TraversalExecutor.cpp +++ b/arangod/Aql/TraversalExecutor.cpp @@ -218,6 +218,7 @@ auto TraversalExecutor::doSkip(AqlCall& call) -> size_t { auto skip = size_t{0}; while (call.shouldSkip() && _traverser.hasMore() && _traverser.next()) { + TRI_ASSERT(_inputRow.isInitialized()); skip++; call.didSkip(1); } @@ -232,6 +233,7 @@ auto TraversalExecutor::produceRows(AqlItemBlockInputRange& input, OutputAqlItem while (true) { if (_traverser.hasMore()) { + TRI_ASSERT(_inputRow.isInitialized()); doOutput(output); if (output.isFull()) { From 298299615a7cf030f0d15dcc1cd94de0f2817d63 Mon Sep 17 00:00:00 2001 From: Markus Pfeiffer Date: Thu, 19 Mar 2020 14:17:04 +0000 Subject: [PATCH 6/7] Remove old produceRows --- arangod/Aql/TraversalExecutor.cpp | 6 ------ arangod/Aql/TraversalExecutor.h | 8 -------- 2 files changed, 14 deletions(-) diff --git a/arangod/Aql/TraversalExecutor.cpp b/arangod/Aql/TraversalExecutor.cpp index 71cc7e4cd1a8..ae3948715130 100644 --- a/arangod/Aql/TraversalExecutor.cpp +++ b/arangod/Aql/TraversalExecutor.cpp @@ -176,12 +176,6 @@ std::pair TraversalExecutor::shutdown(int errorCode) { return {ExecutionState::DONE, TRI_ERROR_NO_ERROR}; } -std::pair TraversalExecutor::produceRows(OutputAqlItemRow& output) { - // TODO: Remove me! - TRI_ASSERT(false); - return {ExecutionState::DONE, TraversalStats{}}; -} - auto TraversalExecutor::doOutput(OutputAqlItemRow& output) -> void { while (!output.isFull() && _traverser.hasMore() && _traverser.next()) { TRI_ASSERT(_inputRow.isInitialized()); diff --git a/arangod/Aql/TraversalExecutor.h b/arangod/Aql/TraversalExecutor.h index a614fa0d5e56..ef04e1b38d04 100644 --- a/arangod/Aql/TraversalExecutor.h +++ b/arangod/Aql/TraversalExecutor.h @@ -133,14 +133,6 @@ class TraversalExecutor { */ std::pair shutdown(int errorCode); - /** - * @brief produce the next Row of Aql Values. - * - * @return ExecutionState, and if successful exactly one new Row of AqlItems. - */ - [[nodiscard]] auto produceRows(OutputAqlItemRow& output) - -> std::pair; - [[nodiscard]] auto produceRows(AqlItemBlockInputRange& input, OutputAqlItemRow& output) -> std::tuple; [[nodiscard]] auto skipRowsRange(AqlItemBlockInputRange& input, AqlCall& call) From 1ae3309b3b6dbf2183fd6be696da952626449bd5 Mon Sep 17 00:00:00 2001 From: Markus Pfeiffer Date: Thu, 19 Mar 2020 15:53:39 +0000 Subject: [PATCH 7/7] Add assertEqual to regression test to validate traversal result --- .../aql/aql-traversal-reset-regression.js | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/tests/js/server/aql/aql-traversal-reset-regression.js b/tests/js/server/aql/aql-traversal-reset-regression.js index a59b5da6e975..ca1ab70addb8 100644 --- a/tests/js/server/aql/aql-traversal-reset-regression.js +++ b/tests/js/server/aql/aql-traversal-reset-regression.js @@ -40,6 +40,27 @@ const productCollectionName = "RegressionProduct"; const customerCollectionName = "RegressionCustomer"; const ownsEdgeCollectionName = "RegressionOwns"; +const expectedResult = [ + { + _key: "396", + _id: customerCollectionName + "/396", + name: "John", + surname: "Smith", + age: 52, + alive: false, + _class: "com.arangodb.springframework.testdata.Customer" + }, + { + _key: "397", + _id: customerCollectionName + "/397", + name: "Matt", + surname: "Smith", + age: 34, + alive: false, + _class: "com.arangodb.springframework.testdata.Customer" + } +]; + var cleanup = function() { db._drop(productCollectionName); db._drop(customerCollectionName); @@ -259,7 +280,8 @@ function traversalResetRegressionSuite() { (FOR e1 IN 1..1 ANY e._id @@owns FILTER e1.name == @1 RETURN 1)[0] == 1 - RETURN e`; + RETURN UNSET(e, "_rev")`; + const bindVars = { "0": "phone", "1": "phone", @@ -268,8 +290,8 @@ function traversalResetRegressionSuite() { "@owns": ownsEdgeCollectionName }; - // var plan = db._explain(query, bindVars); var actual = db._query(query, bindVars); + assertEqual(actual.toArray(), expectedResult); } }; }