Skip to content

fix(transfer_engine): replace deprecated Json::Reader #938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 20, 2025
Merged

fix(transfer_engine): replace deprecated Json::Reader #938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d4b0dc
fix(transfer_engine): replace deprecated Json::Reader with thread-saf…
xiaguan Oct 17, 2025
930acb6
refactor(topology): simplify toJson() to avoid unnecessary serialization
xiaguan Oct 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions mooncake-common/src/default_config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,22 +45,31 @@ void DefaultConfig::loadFromYAML() {

void DefaultConfig::loadFromJSON() {
Json::Value root;
Json::Reader reader;
std::ifstream file;
file.open(path_);

if (!reader.parse(file, root, false)) {
file.close();
throw std::runtime_error("Failed to parse JSON file: " +
reader.getFormattedErrorMessages());
// Read entire file into string
std::string json_content((std::istreambuf_iterator<char>(file)),
std::istreambuf_iterator<char>());
file.close();

// Use thread-safe CharReaderBuilder instead of deprecated Reader
Json::CharReaderBuilder builder;
builder["collectComments"] = false;
std::unique_ptr<Json::CharReader> reader(builder.newCharReader());
std::string errs;

if (!reader->parse(json_content.data(),
json_content.data() + json_content.size(), &root,
&errs)) {
throw std::runtime_error("Failed to parse JSON file: " + errs);
}

try {
processNode(root, "");
} catch (const std::exception& e) {
file.close();
throw e;
}
Comment on lines 68 to 72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This try-catch block is redundant and potentially incorrect. It catches an exception and immediately re-throws it, which provides no value. The caller of loadFromJSON can handle any exceptions from processNode directly.

Furthermore, throw e; re-throws the exception by value, which can lead to object slicing if the caught exception is a derived class of std::exception. The correct way to re-throw the original exception is with a bare throw;.

Given the redundancy, the best solution is to remove the try-catch block entirely.

    processNode(root, "");

file.close();
}

void DefaultConfig::processNode(const YAML::Node& node, std::string key) {
Expand Down
1 change: 1 addition & 0 deletions mooncake-store/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ add_store_test(pybind_client_test pybind_client_test.cpp)
add_store_test(client_metrics_test client_metrics_test.cpp)
add_store_test(serializer_test serializer_test.cpp)
add_store_test(non_ha_reconnect_test non_ha_reconnect_test.cpp)

add_subdirectory(e2e)

add_executable(high_availability_test high_availability_test.cpp)
Expand Down
20 changes: 16 additions & 4 deletions mooncake-transfer-engine/src/topology.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,20 @@ int Topology::discover(const std::vector<std::string> &filter) {
int Topology::parse(const std::string &topology_json) {
std::set<std::string> rnic_set;
Json::Value root;
Json::Reader reader;

if (topology_json.empty() || !reader.parse(topology_json, root)) {
if (topology_json.empty()) {
return ERR_MALFORMED_JSON;
}

// Use thread-safe CharReaderBuilder instead of deprecated Reader
Json::CharReaderBuilder builder;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with other parts of the codebase (e.g., default_config.cpp) and for a minor performance improvement, it's good practice to explicitly disable comment collection when parsing JSON that is not expected to contain comments.

Suggested change
Json::CharReaderBuilder builder;
Json::CharReaderBuilder builder;
builder["collectComments"] = false;

std::unique_ptr<Json::CharReader> reader(builder.newCharReader());
std::string errs;

if (!reader->parse(topology_json.data(),
topology_json.data() + topology_json.size(), &root,
&errs)) {
LOG(ERROR) << "Topology::parse: JSON parse error: " << errs;
return ERR_MALFORMED_JSON;
}

Expand Down Expand Up @@ -303,8 +314,9 @@ std::string Topology::toString() const {

Json::Value Topology::toJson() const {
Json::Value root;
Json::Reader reader;
reader.parse(toString(), root);
for (const auto &pair : matrix_) {
root[pair.first] = pair.second.toJson();
}
return root;
}

Expand Down
2 changes: 0 additions & 2 deletions mooncake-transfer-engine/src/transfer_metadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ struct TransferNotifyUtil {
}

static int decode(Json::Value root, TransferMetadata::NotifyDesc &desc) {
Json::Reader reader;
desc.name = root["name"].asString();
desc.notify_msg = root["notify_msg"].asString();
return 0;
Expand All @@ -67,7 +66,6 @@ struct TransferHandshakeUtil {
}

static int decode(Json::Value root, TransferMetadata::HandShakeDesc &desc) {
Json::Reader reader;
desc.local_nic_path = root["local_nic_path"].asString();
desc.peer_nic_path = root["peer_nic_path"].asString();
for (const auto &qp : root["qp_num"])
Expand Down
75 changes: 51 additions & 24 deletions mooncake-transfer-engine/src/transfer_metadata_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,21 @@
#include "config.h"
#include "error.h"

// Helper function to parse JSON string using thread-safe CharReaderBuilder
static bool parseJsonString(const std::string &json_str, Json::Value &value,
std::string *error_msg = nullptr) {
Json::CharReaderBuilder builder;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with other parts of the codebase (e.g., default_config.cpp) and for a minor performance improvement, it's good practice to explicitly disable comment collection when parsing JSON that is not expected to contain comments.

Suggested change
Json::CharReaderBuilder builder;
Json::CharReaderBuilder builder;
builder["collectComments"] = false;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

To maintain consistency with other parts of the codebase (like in default_config.cpp) and the previous Json::Reader behavior, and for better performance, it's recommended to explicitly disable comment collection in the Json::CharReaderBuilder. The default for CharReaderBuilder is to collect comments, which might be an unnecessary overhead when parsing JSON that doesn't contain comments.

Suggested change
Json::CharReaderBuilder builder;
Json::CharReaderBuilder builder;
builder["collectComments"] = false;

std::unique_ptr<Json::CharReader> reader(builder.newCharReader());
std::string errs;

bool success = reader->parse(
json_str.data(), json_str.data() + json_str.size(), &value, &errs);
if (!success && error_msg) {
*error_msg = errs;
}
return success;
}

namespace mooncake {
#ifdef USE_REDIS
struct RedisStoragePlugin : public MetadataStoragePlugin {
Expand Down Expand Up @@ -118,7 +133,6 @@ struct RedisStoragePlugin : public MetadataStoragePlugin {
std::lock_guard<std::mutex> lock(access_client_mutex_);
if (!client_) return false;

Json::Reader reader;
redisReply *resp =
(redisReply *)redisCommand(client_, "GET %s", key.c_str());
if (!resp) {
Expand All @@ -135,7 +149,12 @@ struct RedisStoragePlugin : public MetadataStoragePlugin {

auto json_file = std::string(resp->str);
freeReplyObject(resp);
if (!reader.parse(json_file, value)) return false;

std::string errs;
if (!parseJsonString(json_file, value, &errs)) {
LOG(ERROR) << "RedisStoragePlugin: JSON parse error: " << errs;
return false;
}
return true;
}

Expand Down Expand Up @@ -374,15 +393,19 @@ struct EtcdStoragePlugin : public MetadataStoragePlugin {
virtual ~EtcdStoragePlugin() {}

virtual bool get(const std::string &key, Json::Value &value) {
Json::Reader reader;
auto resp = client_.get(key);
if (!resp.is_ok()) {
LOG(ERROR) << "EtcdStoragePlugin: unable to get " << key << " from "
<< metadata_uri_ << ": " << resp.error_message();
return false;
}
auto json_file = resp.value().as_string();
if (!reader.parse(json_file, value)) return false;

std::string errs;
if (!parseJsonString(json_file, value, &errs)) {
LOG(ERROR) << "EtcdStoragePlugin: JSON parse error: " << errs;
return false;
}
return true;
}

Expand Down Expand Up @@ -429,7 +452,6 @@ struct EtcdStoragePlugin : public MetadataStoragePlugin {
virtual ~EtcdStoragePlugin() { EtcdCloseWrapper(); }

virtual bool get(const std::string &key, Json::Value &value) {
Json::Reader reader;
char *json_data = nullptr;
auto ret = EtcdGetWrapper((char *)key.c_str(), &json_data, &err_msg_);
if (ret) {
Expand All @@ -446,7 +468,12 @@ struct EtcdStoragePlugin : public MetadataStoragePlugin {
auto json_file = std::string(json_data);
// free the memory allocated by EtcdGetWrapper
free(json_data);
if (!reader.parse(json_file, value)) return false;

std::string errs;
if (!parseJsonString(json_file, value, &errs)) {
LOG(ERROR) << "EtcdStoragePlugin: JSON parse error: " << errs;
return false;
}
return true;
}

Expand Down Expand Up @@ -716,16 +743,16 @@ struct SocketHandShakePlugin : public HandShakePlugin {
getNetworkAddress((struct sockaddr *)&addr);

Json::Value local, peer;
Json::Reader reader;

auto [type, json_str] = readString(conn_fd);
if (!reader.parse(json_str, peer)) {
LOG(ERROR) << "SocketHandShakePlugin: failed to receive "
"handshake message, "
"malformed json format:"
<< reader.getFormattedErrorMessages()
<< ", json string length: " << json_str.size()
<< ", json string content: " << json_str;
std::string errs;
if (!parseJsonString(json_str, peer, &errs)) {
LOG(ERROR)
<< "SocketHandShakePlugin: failed to receive "
"handshake message, "
"malformed json format: "
<< errs << ", json string length: " << json_str.size()
<< ", json string content: " << json_str;
close(conn_fd);
continue;
}
Expand Down Expand Up @@ -906,7 +933,6 @@ struct SocketHandShakePlugin : public HandShakePlugin {
return ret;
}

Json::Reader reader;
auto [type, json_str] = readString(conn_fd);
if (type != HandShakeRequestType::Connection) {
LOG(ERROR)
Expand All @@ -915,10 +941,11 @@ struct SocketHandShakePlugin : public HandShakePlugin {
return ERR_SOCKET;
}

if (!reader.parse(json_str, peer)) {
std::string errs;
if (!parseJsonString(json_str, peer, &errs)) {
LOG(ERROR) << "SocketHandShakePlugin: failed to receive handshake "
"message: "
"malformed json format, check tcp connection";
"message: malformed json format: "
<< errs;
close(conn_fd);
return ERR_MALFORMED_JSON;
}
Expand Down Expand Up @@ -981,7 +1008,6 @@ struct SocketHandShakePlugin : public HandShakePlugin {
return ret;
}

Json::Reader reader;
auto [type, json_str] = readString(conn_fd);
if (type != HandShakeRequestType::Notify) {
LOG(ERROR)
Expand All @@ -993,10 +1019,11 @@ struct SocketHandShakePlugin : public HandShakePlugin {
// LOG(INFO) << "SocketHandShakePlugin: received metadata message: "
// << json_str;

if (!reader.parse(json_str, peer_notify)) {
std::string errs;
if (!parseJsonString(json_str, peer_notify, &errs)) {
LOG(ERROR) << "SocketHandShakePlugin: failed to receive metadata "
"message, malformed json format: "
<< reader.getFormattedErrorMessages();
<< errs;
close(conn_fd);
return ERR_MALFORMED_JSON;
}
Expand All @@ -1023,7 +1050,6 @@ struct SocketHandShakePlugin : public HandShakePlugin {
return ret;
}

Json::Reader reader;
auto [type, json_str] = readString(conn_fd);
if (type != HandShakeRequestType::Metadata) {
LOG(ERROR)
Expand All @@ -1035,10 +1061,11 @@ struct SocketHandShakePlugin : public HandShakePlugin {
// LOG(INFO) << "SocketHandShakePlugin: received metadata message: "
// << json_str;

if (!reader.parse(json_str, peer_metadata)) {
std::string errs;
if (!parseJsonString(json_str, peer_metadata, &errs)) {
LOG(ERROR) << "SocketHandShakePlugin: failed to receive metadata "
"message, malformed json format: "
<< reader.getFormattedErrorMessages();
<< errs;
close(conn_fd);
return ERR_MALFORMED_JSON;
}
Expand Down
Loading