fix(transfer_engine): replace deprecated Json::Reader #938
Conversation
…e CharReaderBuilder The deprecated Json::Reader class uses static global variables internally, causing race conditions and crashes in multi-threaded scenarios. This fix replaces all Json::Reader usage with the thread-safe Json::CharReaderBuilder API across the transfer engine and common modules. Changes: - mooncake-transfer-engine: Updated JSON parsing in transfer_metadata_plugin, transfer_metadata, and topology modules - mooncake-common: Fixed JSON parsing in default_config - All JSON parsing now uses CharReaderBuilder for thread safety This resolves intermittent CI crashes in concurrent tests like ConcurrentPutGetWithLeaseTimeOut.
xiaguan
requested review from
alogfans and
doujiang24
and removed request for
alogfans
Summary of ChangesHello @xiaguan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances system stability by systematically replacing the deprecated and thread-unsafe Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully replaces the deprecated and thread-unsafe Json::Reader with the modern Json::CharReaderBuilder across the codebase, addressing potential race conditions. The changes are well-executed, and the introduction of a parseJsonString helper function is a good pattern for centralizing the new parsing logic. I've added a few suggestions to further improve the code by enhancing error handling, removing redundant code, and optimizing a JSON conversion function for better performance and robustness.
| Json::Value root; | ||
| Json::Reader reader; | ||
| reader.parse(toString(), root); | ||
| std::string json_str = toString(); | ||
|
|
||
| // Use thread-safe CharReaderBuilder instead of deprecated Reader | ||
| Json::CharReaderBuilder builder; | ||
| std::unique_ptr<Json::CharReader> reader(builder.newCharReader()); | ||
| std::string errs; | ||
|
|
||
| if (!reader->parse(json_str.data(), json_str.data() + json_str.size(), | ||
| &root, &errs)) { | ||
| LOG(ERROR) << "Topology::toJson: JSON parse error: " << errs; | ||
| } | ||
| return root; |
There was a problem hiding this comment.
This implementation serializes the topology to a string via toString() and then immediately parses it back into a Json::Value. This is inefficient and creates a tight coupling with toString(), which could lead to issues like infinite recursion if toString() were to call toJson().
You can construct the Json::Value directly from the matrix_ member variable. This is more efficient and robust.
| Json::Value root; | |
| Json::Reader reader; | |
| reader.parse(toString(), root); | |
| std::string json_str = toString(); | |
| // Use thread-safe CharReaderBuilder instead of deprecated Reader | |
| Json::CharReaderBuilder builder; | |
| std::unique_ptr<Json::CharReader> reader(builder.newCharReader()); | |
| std::string errs; | |
| if (!reader->parse(json_str.data(), json_str.data() + json_str.size(), | |
| &root, &errs)) { | |
| LOG(ERROR) << "Topology::toJson: JSON parse error: " << errs; | |
| } | |
| return root; | |
| Json::Value root; | |
| for (const auto &pair : matrix_) { | |
| root[pair.first] = pair.second.toJson(); | |
| } | |
| return root; |
|
/gemini |
|
It looks like you tried to invoke a command, but you just typed Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully replaces the deprecated and thread-unsafe Json::Reader with the modern Json::CharReaderBuilder API, addressing potential race conditions. The changes are well-implemented across multiple files. My review includes a few suggestions for improvement:
- Refactoring an inefficient JSON serialization-then-deserialization pattern in one function.
- Removing a redundant and potentially incorrect
try-catchblock. - Consistently applying parser configurations for minor performance gains and code uniformity.
Overall, this is a solid fix that improves the thread safety and robustness of JSON handling.
| try { | ||
| processNode(root, ""); | ||
| } catch (const std::exception& e) { | ||
| file.close(); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
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, "");| // 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; |
There was a problem hiding this comment.
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.
| Json::CharReaderBuilder builder; | |
| Json::CharReaderBuilder builder; | |
| builder["collectComments"] = false; |
| } | ||
|
|
||
| // Use thread-safe CharReaderBuilder instead of deprecated Reader | ||
| Json::CharReaderBuilder builder; |
There was a problem hiding this comment.
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.
| Json::CharReaderBuilder builder; | |
| Json::CharReaderBuilder builder; | |
| builder["collectComments"] = false; |
| Json::Value root; | ||
| Json::Reader reader; | ||
| reader.parse(toString(), root); | ||
| std::string json_str = toString(); | ||
|
|
||
| // Use thread-safe CharReaderBuilder instead of deprecated Reader | ||
| Json::CharReaderBuilder builder; | ||
| std::unique_ptr<Json::CharReader> reader(builder.newCharReader()); | ||
| std::string errs; | ||
|
|
||
| if (!reader->parse(json_str.data(), json_str.data() + json_str.size(), | ||
| &root, &errs)) { | ||
| LOG(ERROR) << "Topology::toJson: JSON parse error: " << errs; | ||
| } |
There was a problem hiding this comment.
This function is inefficient as it calls toString() to serialize a Json::Value to a string, and then immediately parses that string back into a Json::Value. The toString() function itself builds the Json::Value that is needed. You can avoid the unnecessary serialization and parsing by directly constructing the Json::Value here, which is both more performant and easier to read.
| Json::Value root; | |
| Json::Reader reader; | |
| reader.parse(toString(), root); | |
| std::string json_str = toString(); | |
| // Use thread-safe CharReaderBuilder instead of deprecated Reader | |
| Json::CharReaderBuilder builder; | |
| std::unique_ptr<Json::CharReader> reader(builder.newCharReader()); | |
| std::string errs; | |
| if (!reader->parse(json_str.data(), json_str.data() + json_str.size(), | |
| &root, &errs)) { | |
| LOG(ERROR) << "Topology::toJson: JSON parse error: " << errs; | |
| } | |
| Json::Value root(Json::objectValue); | |
| for (const auto& entry : matrix_) { | |
| root[entry.first] = entry.second.toJson(); | |
| } |
Instead of serializing to string and parsing back, directly construct Json::Value from matrix_. This is more efficient and eliminates the need for JSON parsing entirely in this method.
xiaguan
force-pushed
the
fix-json-thread-safety
branch
from
1959d80 to
930acb6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the thread-safety issue of Json::Reader by migrating to the Json::CharReaderBuilder API. The changes are applied consistently across the codebase, and the introduction of a helper function for parsing is a good practice. I have a few suggestions to further improve the implementation, mainly concerning error handling in file operations, removing redundant code, and ensuring consistent behavior in the new JSON parsing logic for better performance.
| // 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; |
There was a problem hiding this comment.
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.
| Json::CharReaderBuilder builder; | |
| Json::CharReaderBuilder builder; | |
| builder["collectComments"] = false; |
2 participants
Problem
The deprecated
Json::Readerclass uses static global variables internally, causing race conditions and memory corruption in multi-threaded scenarios. This led to intermittent CI crashes with symptoms including:Solution
Replaced all
Json::Readerusage with the thread-safeJson::CharReaderBuilderAPI across:mooncake-transfer-engine: transfer_metadata_plugin, transfer_metadata, topologymooncake-common: default_configTesting
Verified the fix resolves the issue:
ConcurrentPutGetWithLeaseTimeOuttest passes consistently (3/3 runs)This should eliminate the small probability CI crashes in concurrent scenarios.