8000 [3.6] Bug fix/fix agency comm result swallowing errors (#12111) · RtiWeb/arangodb@c91819d · GitHub
[go: up one dir, main page]
Skip to content

Commit c91819d

Browse files
goedderzKVS85
goedderz
and
KVS85
authored
[3.6] Bug fix/fix agency comm result swallowing errors (arangodb#12111)
* Conservative backport of arangodb#11892 * Added a CHANGELOG entry for arangodb#11892 * Fixed compile errors * Compile fix for old MSVC Co-authored-by: Vadim <vadim@arangodb.com>
1 parent 08ea3eb commit c91819d

File tree

9 files changed

+100
-90
lines changed

9 files changed

+100
-90
lines changed

‎CHANGELOG

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
v3.6.5 (XXXX-XX-XX)
22
-------------------
33

4+
* Internal issue BTS-71: Fixed error handling regarding communication with the
5+
agency. This could in a specific case cause collection creation in a cluster
6+
report success when it failed.
7+
48
* Updated arangosync to 0.7.7.
59

610
* Fixed a document parsing bug in the Web UI. This issue occured in the

‎arangod/Agency/AgencyComm.cpp

Lines changed: 52 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -373,18 +373,8 @@ bool AgencyReadTransaction::validate(AgencyCommResult const& result) const {
373373
// --SECTION-- AgencyCommResult
374374
// -----------------------------------------------------------------------------
375375

376-
AgencyCommResult::AgencyCommResult()
377-
: _location(), _message(), _body(), _values(), _statusCode(0), _connected(false), _sent(false) {}
378-
379-
AgencyCommResult::AgencyCommResult(int code, std::string const& message,
380-
std::string const& clientId)
381-
: _location(),
382-
_message(message),
383-
_body(),
384-
_values(),
385-
_statusCode(code),
386-
_connected(false),
387-
_sent(false) {}
376+
AgencyCommResult::AgencyCommResult(int code, std::string message)
377+
: _message(std::move(message)), _statusCode(code) {}
388378

389379
AgencyCommResult::AgencyCommResult(AgencyCommResult&& other) noexcept
390380
: _location(std::move(other._location)),
@@ -418,8 +408,8 @@ AgencyCommResult& AgencyCommResult::operator=(AgencyCommResult&& other) noexcept
418408
return *this;
419409
}
420410

421-
void AgencyCommResult::set(int code, std::string const& message) {
422-
_message = message;
411+
void AgencyCommResult::set(int code, std::string message) {
412+
_message = std::move(message);
423413
_statusCode = code;
424414
_location.clear();
425415
_body.clear();
@@ -434,45 +424,39 @@ int AgencyCommResult::httpCode() const { return _statusCode; }
434424
bool AgencyCommResult::sent() const { return _sent; }
435425

436426
int AgencyCommResult::errorCode() const {
427+
return asResult().errorNumber();
428+
}
429+
430+
std::string AgencyCommResult::errorMessage() const {
431+
return asResult().errorMessage();
432+
}
433+
434+
std::optional<std::pair<int, std::string_view>> AgencyCommResult::parseBodyError() const {
435+
auto result = std::optional<std::pair<int, std::string_view>>{};
437436
try {
438437
if (!_body.empty()) {
439438
std::shared_ptr<VPackBuilder> bodyBuilder = VPackParser::fromJson(_body);
440439
VPackSlice body = bodyBuilder->slice();
441-
if (!body.isObject()) {
442-
return 0;
440+
if (body.isObject()) {
441+
// get "errorCode" attribute
442+
auto const errorCode = body.get(StaticStrings::ErrorCode).getNumber<int>();
443+
// Save error code if possible, set default error message first
444+
result = std::make_pair(errorCode, std::string_view(TRI_errno_string(errorCode)));
445+
// Now try to extract the message, too; but it's fine if that fails, we
446+
// already have the default one.
447+
if (auto const errMsg = body.get(StaticStrings::ErrorMessage); errMsg.isString()) {
448+
auto const strRef = errMsg.stringRef();
449+
result->second = {strRef.begin(), strRef.size()};
450+
} else if (auto const errMsg = body.get("message"); errMsg.isString()) {
451+
auto const strRef = errMsg.stringRef();
452+
result->second = {strRef.begin(), strRef.size()};
453+
}
443454
}
444-
// get "errorCode" attribute (0 if not exist)
445-
return basics::VelocyPackHelper::getNumericValue<int>(body, "errorCode", 0);
446455
}
447456
} catch (VPackException const&) {
448457
}
449-
return 0;
450-
}
451-
452-
std::string AgencyCommResult::errorMessage() const {
453-
if (!_message.empty()) {
454-
// return stored message first if set
455-
return _message;
456-
}
457-
458-
if (!_connected) {
459-
return std::string("unable to connect to agency");
460-
}
461458

462-
try {
463-
std::shared_ptr<VPackBuilder> bodyBuilder = VPackParser::fromJson(_body);
464-
465-
VPackSlice body = bodyBuilder->slice();
466-
if (!body.isObject()) {
467-
return "";
468-
}
469-
// get "message" attribute ("" if not exist)
470-
return arangodb::basics::VelocyPackHelper::getStringValue(body, "message",
471-
"");
472-
} catch (VPackException const& e) {
473-
std::string message("VPackException parsing body (" + _body + "): " + e.what());
474-
return std::string(message);
475-
}
459+
return result;
476460
}
477461

478462
std::string AgencyCommResult::errorDetails() const {
@@ -532,6 +516,24 @@ VPackBuilder AgencyCommResult::toVelocyPack() const {
532516
return builder;
533517
}
534518

519+
Result AgencyCommResult::asResult() const {
520+
if (successful()) {
521+
return Result{};
522+
} else if (auto const err = parseBodyError(); err.has_value()) {
523+
return Result{err->first, std::string{err->second}};
524+
} else if (_statusCode > 0) {
525+
if (!_message.empty()) {
526+
return Result{_statusCode, _message};
527+
} else if (!_connected) {
528+
return Result{_statusCode, "unable to connect to agency"};
529+
} else {
530+
return Result{_statusCode};
531+
}
532+
} else {
533+
return Result{TRI_ERROR_INTERNAL};
534+
}
535+
}
536+
535537
namespace std {
536538
ostream& operator<< (ostream& out, AgencyCommResult const& a) {
537539
out << a.toVelocyPack().toJson();
@@ -978,7 +980,7 @@ AgencyCommResult AgencyComm::getValues(std::string const& key) {
978980
}
979981

980982
try {
981-
result.setVPack(VPackParser::fromJson(result.bodyRef()));
983+
result.setVPack(VPackParser::fromJson(result.body()));
982984

983985
if (!result.slice().isArray()) {
984986
result.set(500, "got invalid result structure for getValues response");
@@ -1020,8 +1022,7 @@ AgencyCommResult AgencyComm::dump() {
10201022
}
10211023

10221024
try {
1023-
1024-
result.setVPack(VPackParser::fromJson(result.bodyRef()));
1025+
result.setVPack(VPackParser::fromJson(result.body()));
10251026
result._body.clear();
10261027
result._statusCode = 200;
10271028

@@ -1154,6 +1155,7 @@ uint64_t AgencyComm::uniqid(uint64_t count, double timeout) {
11541155
// The cas did not work, simply try again!
11551156
}
11561157

1158+
TRI_ASSERT(oldValue != 0);
11571159
return oldValue;
11581160
}
11591161

@@ -1252,14 +1254,14 @@ AgencyCommResult AgencyComm::sendTransactionWithFailover(AgencyTransaction const
12521254
}
12531255

12541256
try {
1255-
result.setVPack(VPackParser::fromJson(result.bodyRef()));
1257+
result.setVPack(VPackParser::fromJson(result.body()));
12561258

12571259
if (!transaction.validate(result)) {
12581260
result.set(500, std::string("validation failed for response to URL " + url));
12591261
LOG_TOPIC("f2083", DEBUG, Logger::AGENCYCOMM)
12601262
<< "validation failed for url: " << url
12611263
<< ", type: " << transaction.typeName()
1262-
<< ", sent: " << builder.toJson() << ", received: " << result.bodyRef();
1264+
<< ", sent: " << builder.toJson() << ", received: " << result.body();
12631265
return result;
12641266
}
12651267

@@ -1269,7 +1271,7 @@ AgencyCommResult AgencyComm::sendTransactionWithFailover(AgencyTransaction const
12691271
LOG_TOPIC("e13a5", ERR, Logger::AGENCYCOMM)
12701272
<< "Error transforming result: " << e.what()
12711273
<< ", status code: " << result._statusCode
1272-
<< ", incriminating body: " << result.bodyRef() << ", url: " << url
1274+
<< ", incriminating body: " << result.body() << ", url: " << url
12731275
<< ", timeout: " << timeout << ", data sent: " << builder.toJson();
12741276
result.clear();
12751277
} catch (...) {

‎arangod/Agency/AgencyComm.h

Lines changed: 29 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,15 @@
2828
#include <deque>
2929
#include <list>
3030
#include <memory>
31+
#include <optional>
3132
#include <string>
33+
#include <type_traits>
3234
#include <unordered_map>
35+
#include <utility>
3336

3437
#include <velocypack/Slice.h>
3538
#include <velocypack/Builder.h>
3639
#include <velocypack/velocypack-aliases.h>
37-
#include <type_traits>
3840

3941
#include "Basics/Mutex.h"
4042
#include "Basics/Result.h"
@@ -354,9 +356,8 @@ class AgencyOperation {
354356

355357
class AgencyCommResult {
356358
public:
357-
AgencyCommResult();
358-
AgencyCommResult(int code, std::string const& message,
359-
std::string const& transactionId = std::string());
359+
AgencyCommResult() = default;
360+
AgencyCommResult(int code, std::string message);
360361

361362
~AgencyCommResult() = default;
362363

@@ -367,54 +368,51 @@ class AgencyCommResult {
367368
AgencyCommResult& operator=(AgencyCommResult&& other) noexcept;
368369

369370
public:
370-
void set(int code, std::string const& message);
371+
void set(int code, std::string message);
371372

372-
bool successful() const { return (_statusCode >= 200 && _statusCode <= 299); }
373+
[[nodiscard]] bool successful() const { return (_statusCode >= 200 && _statusCode <= 299); }
373374

374-
bool connected() const;
375+
[[nodiscard]] bool connected() const;
375376

376-
int httpCode() const;
377+
[[nodiscard]] int httpCode() const;
377378

378-
int errorCode() const;
379+
[[nodiscard]] int errorCode() const;
379380

380-
std::string errorMessage() const;
381+
[[nodiscard]] std::string errorMessage() const;
381382

382-
std::string errorDetails() const;
383+
[[nodiscard]] std::string errorDetails() const;
383384

384-
std::string const location() const { return _location; }
385+
[[nodiscard]] std::string const& location() const { return _location; }
385386

386-
std::string const body() const { return _body; }
387-
std::string const& bodyRef() const { return _body; }
387+
[[nodiscard]] std::string const& body() const { return _body; }
388388

389-
bool sent() const;
389+
[[nodiscard]] bool sent() const;
390390

391391
void clear();
392392

393-
velocypack::Slice slice() const;
393+
[[nodiscard]] velocypack::Slice slice() const;
394+
394395
void setVPack(std::shared_ptr<velocypack::Builder> const& vpack) {
395396
_vpack = vpack;
396397
}
397398

398-
Result asResult() {
399-
if (successful()) {
400-
return Result{};
401-
}
402-
return Result{errorCode(), errorMessage()};
403-
}
399+
[[nodiscard]] Result asResult() const;
404400

405401
void toVelocyPack(VPackBuilder& builder) const;
406402

407-
VPackBuilder toVelocyPack() const;
403+
[[nodiscard]] VPackBuilder toVelocyPack() const;
404+
405+
[[nodiscard]] std::optional<std::pair<int, std::string_view>> parseBodyError() const;
408406

409407
public:
410-
std::string _location;
411-
std::string _message;
412-
std::string _body;
413-
414-
std::unordered_map<std::string, AgencyCommResultEntry> _values;
415-
int _statusCode;
416-
bool _connected;
417-
bool _sent;
408+
std::string _location = "";
409+
std::string _message = "";
410+
std::string _body = "";
411+
412+
std::unordered_map<std::string, AgencyCommResultEntry> _values = {};
413+
int _statusCode = 0;
414+
bool _connected = false;
415+
bool _sent = false;
418416

419417
private:
420418
std::shared_ptr<velocypack::Builder> _vpack;

‎arangod/Cluster/ClusterInfo.cpp

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,8 @@ uint64_t ClusterInfo::uniqid(uint64_t count) {
298298
if (_uniqid._currentValue + count - 1 <= _uniqid._upperValue) {
299299
uint64_t result = _uniqid._currentValue;
300300
_uniqid._currentValue += count;
301+
302+
TRI_ASSERT(result != 0);
301303
return result;
302304
}
303305

@@ -308,6 +310,7 @@ uint64_t ClusterInfo::uniqid(uint64_t count) {
308310
_uniqid._upperValue = _uniqid._nextUpperValue;
309311
triggerBackgroundGetIds();
310312

313+
TRI_ASSERT(result != 0);
311314
return result;
312315
}
313316

@@ -327,6 +330,7 @@ uint64_t ClusterInfo::uniqid(uint64_t count) {
327330
_uniqid._nextBatchStart = _uniqid._upperValue + 1;
328331
_uniqid._nextUpperValue = _uniqid._upperValue + fetch - 1;
329332

333+
TRI_ASSERT(result != 0);
330334
return result;
331335
}
332336

@@ -2343,7 +2347,7 @@ Result ClusterInfo::createCollectionsCoordinator(
23432347
events::CreateCollection(databaseName, info.name, res.errorCode());
23442348
}
23452349
loadCurrent();
2346-
return res.errorCode();
2350+
return res.asResult();
23472351
}
23482352
if (tmpRes > TRI_ERROR_NO_ERROR) {
23492353
// We do not need to lock all condition variables
@@ -4535,7 +4539,7 @@ arangodb::Result ClusterInfo::agencyHotBackupLock(std::string const& backupId,
45354539
std::chrono::system_clock::now() + std::chrono::seconds(timeouti))));
45364540
}
45374541

4538-
// Prevonditions
4542+
// Preconditions
45394543
{
45404544
VPackObjectBuilder precs(&builder);
45414545
builder.add(VPackValue(backupKey)); // Backup key empty
@@ -4581,7 +4585,7 @@ arangodb::Result ClusterInfo::agencyHotBackupLock(std::string const& backupId,
45814585
"failed to acquire backup lock in agency");
45824586
}
45834587

4584-
auto rv = VPackParser::fromJson(result.bodyRef());
4588+
auto rv = VPackParser::fromJson(result.body());
45854589

45864590
LOG_TOPIC("a94d5", DEBUG, Logger::BACKUP)
45874591
<< "agency lock response for backup id " << backupId << ": " << rv->toJson();
@@ -4692,7 +4696,7 @@ arangodb::Result ClusterInfo::agencyHotBackupUnlock(std::string const& backupId,
46924696
"failed to release backup lock in agency");
46934697
}
46944698

4695-
auto rv = VPackParser::fromJson(result.bodyRef());
4699+
auto rv = VPackParser::fromJson(result.body());
46964700

46974701
if (!rv->slice().isObject() || !rv->slice().hasKey("results") ||
46984702
!rv->slice().get("results").isArray()) {

‎arangod/Cluster/FollowerInfo.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ Result FollowerInfo::remove(ServerID const& sid) {
201201
}
202202
// we are finished
203203
LOG_TOPIC("be0cb", DEBUG, Logger::CLUSTER)
204-
<< "Removing follower " << sid << " from " << _docColl->name() << "succeeded";
204+
<< "Removing follower " << sid << " from " << _docColl->name() << " succeeded";
205205
return agencyRes;
206206
}
207207
if (agencyRes.is(TRI_ERROR_CLUSTER_NOT_LEADER)) {

‎arangod/Cluster/HeartbeatThread.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -575,7 +575,7 @@ void HeartbeatThread::getNewsFromAgencyForCoordinator() {
575575
if (!_server.isStopping()) {
576576
LOG_TOPIC("539fc", WARN, Logger::HEARTBEAT)
577577
<< "Heartbeat: Could not read from agency! status code: " << result._statusCode
578-
<< ", incriminating body: " << result.bodyRef() << ", timeout: " << timeout;
578+
<< ", incriminating body: " << result.body() << ", timeout: " << timeout;
579579
}
580580
} else {
581581
VPackSlice agentPool = result.slice()[0].get(".agency");
@@ -803,7 +803,7 @@ void HeartbeatThread::runSingleServer() {
803803
if (!_server.isStopping()) {
804804
LOG_TOPIC("229fd", WARN, Logger::HEARTBEAT)
805805
<< "Heartbeat: Could not read from agency! status code: "
806-
<< result._statusCode << ", incriminating body: " << result.bodyRef()
806+
<< result._statusCode << ", incriminating body: " << result.body()
807807
<< ", timeout: " << timeout;
808808
}
809809

0 commit comments

Comments
 (0)
0