rpc: answer z_getoperationstatus without cs_main and cs_wallet
z_getoperationstatus took LOCK2(cs_main, cs_wallet) for a pure read of the async operation queue, so a GUI polling it every few seconds queued behind every wallet scan and block connect. It also sorted UniValues with a by-value comparator. - z_getoperationstatus_IMPL takes no wallet locks. z_getoperationresult takes LOCK2 itself, so the removing path stays serialised with the wallet's swap of automated operations (cancel then remove, under cs_wallet). - Sort (creation_time, index) pairs comparing the time only. libstdc++'s sort depends only on the comparator results, so the order, ties included, is the same as before. - AsyncRPCQueue::popOperationForId finds and erases under one lock. - z_listoperationids takes no wallet locks; the status-name map is const and read with at(). Output is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -32,7 +32,7 @@ using namespace std;
|
|||||||
|
|
||||||
static boost::uuids::random_generator uuidgen;
|
static boost::uuids::random_generator uuidgen;
|
||||||
|
|
||||||
static std::map<OperationStatus, std::string> OperationStatusMap = {
|
static const std::map<OperationStatus, std::string> OperationStatusMap = {
|
||||||
{OperationStatus::READY, "queued"},
|
{OperationStatus::READY, "queued"},
|
||||||
{OperationStatus::EXECUTING, "executing"},
|
{OperationStatus::EXECUTING, "executing"},
|
||||||
{OperationStatus::CANCELLED, "cancelled"},
|
{OperationStatus::CANCELLED, "cancelled"},
|
||||||
@@ -194,7 +194,7 @@ UniValue AsyncRPCOperation::getStatus() const {
|
|||||||
OperationStatus status = this->getState();
|
OperationStatus status = this->getState();
|
||||||
UniValue obj(UniValue::VOBJ);
|
UniValue obj(UniValue::VOBJ);
|
||||||
obj.push_back(Pair("id", this->id_));
|
obj.push_back(Pair("id", this->id_));
|
||||||
obj.push_back(Pair("status", OperationStatusMap[status]));
|
obj.push_back(Pair("status", OperationStatusMap.at(status)));
|
||||||
obj.push_back(Pair("creation_time", this->creation_time_));
|
obj.push_back(Pair("creation_time", this->creation_time_));
|
||||||
// TODO: Issue #1354: There may be other useful metadata to return to the user.
|
// TODO: Issue #1354: There may be other useful metadata to return to the user.
|
||||||
UniValue err = this->getError();
|
UniValue err = this->getError();
|
||||||
@@ -218,5 +218,5 @@ UniValue AsyncRPCOperation::getStatus() const {
|
|||||||
*/
|
*/
|
||||||
std::string AsyncRPCOperation::getStateAsString() const {
|
std::string AsyncRPCOperation::getStateAsString() const {
|
||||||
OperationStatus status = this->getState();
|
OperationStatus status = this->getState();
|
||||||
return OperationStatusMap[status];
|
return OperationStatusMap.at(status);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -129,14 +129,18 @@ std::shared_ptr<AsyncRPCOperation> AsyncRPCQueue::getOperationForId(AsyncRPCOper
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Return the operation for a given operation id and then remove the operation from internal storage.
|
* Return the operation for a given operation id and then remove the operation from internal storage.
|
||||||
|
* The lookup and the erase happen under one hold of lock_, so when several threads pop the same id
|
||||||
|
* exactly one of them receives the operation and the others get a null pointer.
|
||||||
*/
|
*/
|
||||||
std::shared_ptr<AsyncRPCOperation> AsyncRPCQueue::popOperationForId(AsyncRPCOperationId id) {
|
std::shared_ptr<AsyncRPCOperation> AsyncRPCQueue::popOperationForId(AsyncRPCOperationId id) {
|
||||||
std::shared_ptr<AsyncRPCOperation> ptr = getOperationForId(id);
|
std::shared_ptr<AsyncRPCOperation> ptr; // declared before guard, so it is released after unlock
|
||||||
if (ptr) {
|
|
||||||
std::lock_guard<std::mutex> guard(lock_);
|
std::lock_guard<std::mutex> guard(lock_);
|
||||||
|
AsyncRPCOperationMap::iterator iter = operation_map_.find(id);
|
||||||
|
if (iter != operation_map_.end()) {
|
||||||
|
ptr = iter->second;
|
||||||
// Note: if the id still exists in the operationIdQueue, when it gets processed by a worker
|
// Note: if the id still exists in the operationIdQueue, when it gets processed by a worker
|
||||||
// there will no operation in the map to execute, so nothing will happen.
|
// there will no operation in the map to execute, so nothing will happen.
|
||||||
operation_map_.erase(id);
|
operation_map_.erase(iter);
|
||||||
}
|
}
|
||||||
return ptr;
|
return ptr;
|
||||||
}
|
}
|
||||||
|
|||||||
+42
-19
@@ -5079,7 +5079,12 @@ UniValue z_getoperationresult(const UniValue& params, bool fHelp, const CPubKey&
|
|||||||
+ HelpExampleRpc("z_getoperationresult", "'[\"operationid\", ... ]'")
|
+ HelpExampleRpc("z_getoperationresult", "'[\"operationid\", ... ]'")
|
||||||
);
|
);
|
||||||
|
|
||||||
// This call will remove finished operations
|
// This call will remove finished operations. Unlike z_getoperationstatus it keeps cs_main +
|
||||||
|
// cs_wallet: CWallet::RunSaplingSweep/RunSaplingConsolidation/RunAutoShieldCoinbase cancel and
|
||||||
|
// remove the previous automated operation inside one cs_wallet critical section, and holding
|
||||||
|
// cs_wallet here keeps this removal serialised with that and with other z_getoperationresult
|
||||||
|
// callers, exactly as before.
|
||||||
|
LOCK2(cs_main, pwalletMain->cs_wallet);
|
||||||
return z_getoperationstatus_IMPL(params, true);
|
return z_getoperationstatus_IMPL(params, true);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -5108,7 +5113,12 @@ UniValue z_getoperationstatus(const UniValue& params, bool fHelp, const CPubKey&
|
|||||||
|
|
||||||
UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedOperations=false)
|
UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedOperations=false)
|
||||||
{
|
{
|
||||||
LOCK2(cs_main, pwalletMain->cs_wallet);
|
// No cs_main/cs_wallet here: nothing below reads wallet or chain state. AsyncRPCQueue guards
|
||||||
|
// its operation map with its own mutex, and AsyncRPCOperation::getStatus() (and every
|
||||||
|
// override) reads only the atomic state_, members fixed at construction (id_, creation_time_,
|
||||||
|
// contextinfo_, targetHeight_) and result/error under the operation's own lock_. Holding the
|
||||||
|
// wallet locks only made this poll wait behind wallet scans and block connection. The removing
|
||||||
|
// form is serialised by its caller, z_getoperationresult, which still holds cs_main + cs_wallet.
|
||||||
|
|
||||||
std::set<AsyncRPCOperationId> filter;
|
std::set<AsyncRPCOperationId> filter;
|
||||||
if (params.size()==1) {
|
if (params.size()==1) {
|
||||||
@@ -5119,11 +5129,18 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO
|
|||||||
}
|
}
|
||||||
bool useFilter = (filter.size()>0);
|
bool useFilter = (filter.size()>0);
|
||||||
|
|
||||||
UniValue ret(UniValue::VARR);
|
|
||||||
std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
|
std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
|
||||||
std::vector<AsyncRPCOperationId> ids = q->getAllOperationIds();
|
std::vector<AsyncRPCOperationId> ids = q->getAllOperationIds();
|
||||||
|
|
||||||
for (auto id : ids) {
|
// Status objects in enumeration order, and one (creation_time, index) key per object for the
|
||||||
|
// sort. UniValue has no move operations, so sorting the objects themselves deep-copied them on
|
||||||
|
// every comparison and every element move.
|
||||||
|
std::vector<UniValue> objs;
|
||||||
|
objs.reserve(ids.size());
|
||||||
|
std::vector<std::pair<int64_t, size_t> > order;
|
||||||
|
order.reserve(ids.size());
|
||||||
|
|
||||||
|
for (const AsyncRPCOperationId& id : ids) {
|
||||||
if (useFilter && !filter.count(id))
|
if (useFilter && !filter.count(id))
|
||||||
continue;
|
continue;
|
||||||
|
|
||||||
@@ -5139,26 +5156,32 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO
|
|||||||
if (fRemoveFinishedOperations) {
|
if (fRemoveFinishedOperations) {
|
||||||
// Caller is only interested in retrieving finished results
|
// Caller is only interested in retrieving finished results
|
||||||
if ("success"==s || "failed"==s || "cancelled"==s) {
|
if ("success"==s || "failed"==s || "cancelled"==s) {
|
||||||
ret.push_back(obj);
|
objs.push_back(obj);
|
||||||
q->popOperationForId(id);
|
// Report the operation only if this call is the one that removed it.
|
||||||
|
if (q->popOperationForId(id)) {
|
||||||
|
order.push_back(std::make_pair(operation->getCreationTime(), objs.size() - 1));
|
||||||
|
} else {
|
||||||
|
objs.pop_back();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
ret.push_back(obj);
|
objs.push_back(obj);
|
||||||
|
order.push_back(std::make_pair(operation->getCreationTime(), objs.size() - 1));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
std::vector<UniValue> arrTmp = ret.getValues();
|
// sort results chronologically by creation_time. Compare creation_time only, never the index:
|
||||||
|
// std::sort then makes exactly the comparisons and moves it made on the status objects before,
|
||||||
// sort results chronologically by creation_time
|
// so operations created in the same second keep the order earlier releases returned.
|
||||||
std::sort(arrTmp.begin(), arrTmp.end(), [](UniValue a, UniValue b) -> bool {
|
std::sort(order.begin(), order.end(),
|
||||||
const int64_t t1 = find_value(a.get_obj(), "creation_time").get_int64();
|
[](const std::pair<int64_t, size_t>& a, const std::pair<int64_t, size_t>& b) -> bool {
|
||||||
const int64_t t2 = find_value(b.get_obj(), "creation_time").get_int64();
|
return a.first < b.first;
|
||||||
return t1 < t2;
|
|
||||||
});
|
});
|
||||||
|
|
||||||
ret.clear();
|
UniValue ret(UniValue::VARR);
|
||||||
ret.setArray();
|
for (size_t i = 0; i < order.size(); i++) {
|
||||||
ret.push_backV(arrTmp);
|
ret.push_back(objs[order[i].second]);
|
||||||
|
}
|
||||||
|
|
||||||
return ret;
|
return ret;
|
||||||
}
|
}
|
||||||
@@ -6245,8 +6268,8 @@ UniValue z_listoperationids(const UniValue& params, bool fHelp, const CPubKey& m
|
|||||||
+ HelpExampleRpc("z_listoperationids", "")
|
+ HelpExampleRpc("z_listoperationids", "")
|
||||||
);
|
);
|
||||||
|
|
||||||
LOCK2(cs_main, pwalletMain->cs_wallet);
|
// No cs_main/cs_wallet: like z_getoperationstatus this only reads the self-locking async
|
||||||
|
// operation queue and each operation's atomic state.
|
||||||
std::string filter;
|
std::string filter;
|
||||||
bool useFilter = false;
|
bool useFilter = false;
|
||||||
if (params.size()==1) {
|
if (params.size()==1) {
|
||||||
|
|||||||
Reference in New Issue
Block a user