fix: Remove dangling reference to msgpack::object in task executor; Increase test value sizes to cover this case. - #90
Conversation
WalkthroughThis pull request updates the SQL schema definitions by increasing the maximum lengths of VARCHAR and VARBINARY fields for several tables. It also introduces a new class, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor as Task Executor
participant Parser as TaskExecutorRequestParser
Client->>Executor: Send msgpack::sbuffer request
Executor->>Parser: Create parser instance with buffer
Parser-->>Executor: get_type() returns request type
Parser-->>Executor: get_body() returns message body
Executor->>Executor: Process request based on parsed data
Possibly related PRs
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/client/client-test.cpp (1)
208-209: Remove the unnecessary semicolon.There's an extraneous semicolon on line 209 that should be removed.
constexpr size_t cLargeInputSize = 300; -;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/spider/storage/mysql/mysql_stmt.hpp(4 hunks)src/spider/worker/TaskExecutorMessage.hpp(1 hunks)src/spider/worker/task_executor.cpp(1 hunks)tests/client/client-test.cpp(3 hunks)tests/worker/test-TaskExecutor.cpp(1 hunks)tests/worker/worker-test.cpp(3 hunks)tests/worker/worker-test.hpp(2 hunks)tools/scripts/storage/init_db.sql(4 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
tests/worker/test-TaskExecutor.cpp (3)
src/spider/worker/worker.cpp (2)
get_environment_variable(83-102)get_environment_variable(83-85)src/spider/core/Data.hpp (1)
gen(40-43)tests/storage/StorageTestHelper.hpp (2)
get_storage_url(27-29)get_storage_url(27-27)
tests/client/client-test.cpp (2)
tests/worker/worker-test.hpp (1)
join_string_test(24-28)tests/worker/worker-test.cpp (2)
join_string_test(65-71)join_string_test(65-69)
tests/worker/worker-test.cpp (1)
tests/worker/worker-test.hpp (1)
join_string_test(24-28)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (15)
tests/worker/worker-test.hpp (2)
4-4: Good addition of the string headerThis is necessary to support the new function declaration that uses
std::string.
24-28: Function signature looks goodThe new function declaration is well-formed with proper parameter types and return type. The trailing return type syntax
-> std::stringis consistent with the codebase style.tests/worker/test-TaskExecutor.cpp (2)
211-211: Well-defined constantGood practice using a named constant for the input size rather than hardcoding the value in multiple places.
213-246: Well-structured test for large I/OThis test case effectively validates that the task executor can handle large input and output strings. It creates two 300-character strings and verifies that their concatenation works correctly through the task execution framework.
The test aligns with the PR's goal of improving memory management for message objects by ensuring large string handling works properly.
tests/worker/worker-test.cpp (3)
6-6: Necessary header inclusionGood addition of the string header to support the function implementation.
65-71: Simple and effective implementationThe function implementation is straightforward and correctly concatenates the two input strings. The
/*context*/comment on the unused parameter is a good practice to prevent compiler warnings.
81-81: Task registration looks goodProperly registering the new function as a task makes it available to the task execution framework.
src/spider/worker/task_executor.cpp (2)
145-146: Good use of TaskExecutorRequestParserThis change addresses the core issue mentioned in the PR - replacing direct calls to free functions with a class that can properly manage the lifetime of the msgpack object handle. This prevents dangling references when the object handle goes out of scope.
150-150: Fixed dangling reference issueUsing
request_parser.get_body()instead of directly calling a free function ensures that the msgpack object reference remains valid as long as the parser object exists. This directly addresses the memory management issue described in the PR objective.tests/client/client-test.cpp (2)
183-206: Comprehensive test for large input/output handling.The test covers a scenario to ensure the system correctly handles large string inputs and outputs, which aligns with the increased VARCHAR and VARBINARY field sizes in the database schema. The implementation is clean and follows the pattern of existing tests.
275-278: LGTM!Good integration of the new test into the main test runner.
src/spider/worker/TaskExecutorMessage.hpp (2)
40-69: Good encapsulation of request parsing.The new
TaskExecutorRequestParserclass properly addresses the dangling reference issue mentioned in the PR objectives by holding themsgpack::object_handleas a member variable.
76-79: LGTM!The private member variable correctly stores the object handle to prevent dangling references.
tools/scripts/storage/init_db.sql (1)
61-61:Details
✅ Verification successful
Consistent increase of field sizes matches application requirements.
All the VARCHAR and VARBINARY fields have been consistently increased to 999 characters/bytes to support the larger input/output strings being tested in the new test case.
Also applies to: 70-71, 81-82, 84-85, 138-138, 145-145
🏁 Script executed:
#!/bin/bash # Verify that the field size changes are consistent between the SQL script and the C++ header file echo "Comparing field sizes in SQL script and C++ header:" echo "======================================================" # Check SQL script for field sizes echo "Field sizes in SQL script (init_db.sql):" grep -E "(VARCHAR|VARBINARY)\([0-9]+\)" tools/scripts/storage/init_db.sql | grep -E "type|value" echo "----------------------------------------------------" # Check C++ header for field sizes echo "Field sizes in C++ header (mysql_stmt.hpp):" grep -E "(VARCHAR|VARBINARY)\([0-9]+\)" src/spider/storage/mysql/mysql_stmt.hpp | grep -E "type|value"Length of output: 1340
Verified Field Size Increase Consistency
After executing the verification script, the output confirms that all instances of VARCHAR and VARBINARY fields in bothtools/scripts/storage/init_db.sqlandsrc/spider/storage/mysql/mysql_stmt.hpphave been updated uniformly to 999. The grep results clearly show consistent changes for both thetypeandvaluecolumns.
- The SQL script (
init_db.sql) contains the updated field sizes as required.- The C++ header (
mysql_stmt.hpp) mirrors these updates for consistent behaviour.Great work on aligning these field sizes with the application requirements.
src/spider/storage/mysql/mysql_stmt.hpp (1)
70-70:Details
✅ Verification successful
Database schema field sizes consistently increased.
The field size increases in this file match those in the initialization SQL script, which is good for consistency. The size of 999 should be sufficient for handling larger input/output strings being tested.
Also applies to: 73-73, 84-85, 111-111, 144-144, 151-151
🌐 Web query:
What is the maximum recommended size for VARCHAR and VARBINARY fields in MySQL?💡 Result:
In MySQL, the maximum lengths for
VARCHARandVARBINARYfields are constrained by row-size limits and practical performance considerations:VARCHAR Recommendations
- Maximum theoretical length:
Up to 65,535 characters, but this is shared across all columns in a row and depends on the character set:
- For
utf8mb4(4 bytes per character): ~16,383 characters (65,535 / 4).- For
latin1(1 byte per character): 65,535 characters[1][7].- Recommended practice:
Declare the smallest size needed for your data (e.g.,VARCHAR(255)for emails). Oversized columns waste memory during query processing and can lead to row-size errors[3][6].VARBINARY Recommendations
- Maximum theoretical length:
Up to 65,535 bytes per column, subject to the same 65,535-byte row-size limit[5][8][9].- Recommended practice:
Use exact lengths for fixed-size binary data (e.g.,VARBINARY(16)for UUIDs,VARBINARY(64)for SHA-256 hashes). Avoid unnecessary large declarations[5][8].Key Considerations
- Row-Size Limit:
Total bytes for all columns in a row cannot exceed 65,535 bytes, including overhead (e.g., 1–2 bytes forVARCHARlength prefixes)[7].- Performance Implications:
- Over-declaring sizes forces MySQL to allocate larger memory buffers for sorting/grouping operations[3][6].
- For indexed columns, prefer smaller lengths to avoid hitting InnoDB’s 1,073,741,824-byte index prefix limit[3].
- Character Set Impact:
Multi-byte character sets likeutf8mb4reduce usableVARCHARlength (e.g.,utf8mb4allows ~16,383 characters vs.latin1’s 65,535)[7].Example Declarations
-- Recommended for short strings (e.g., usernames): username VARCHAR(50) CHARACTER SET utf8mb4, -- Fixed-length binary data (e.g., UUID): user_token VARBINARY(16), -- Avoid unnecessarily large declarations: description VARCHAR(2000) -- Use 2000 if data rarely exceeds thisIn summary, prioritize minimal viable sizes for
VARCHAR/VARBINARYto optimize storage, memory usage, and query performance.Citations:
- 1: https://dev.mysql.com/doc/refman/8.1/en/string-type-syntax.html
- 2: https://www.dofactory.com/sql/varbinarymax
- 3: https://www.sqlservercentral.com/forums/topic/varchar1000-vs-varcharmax
- 4: https://docs.oracle.com/cd/E19078-01/mysql/mysql-refman-5.1/data-types.html
- 5: https://www.mysqltutorial.org/mysql-basics/mysql-varbinary/
- 6: https://planetscale.com/blog/mysql-data-types-varchar-and-char
- 7: https://dev.mysql.com/doc/en/char.html
- 8: https://wiki.ispirer.com/sqlways/mysql/data-types/varbinary
- 9: https://www.w3schools.com/mysql/mysql_datatypes.asp
- 10: https://learn.microsoft.com/en-us/sql/t-sql/data-types/binary-and-varbinary-transact-sql?view=sql-server-ver16
Field size update verified; changes are consistent with initialization scripts.
The increased field sizes, including the use ofVARCHAR(999)on line 70 (and similarly on lines 73, 84-85, 111, 144, and 151), have been confirmed to align with the initialization SQL. Although MySQL permits much larger sizes theoretically, using 999 for handling larger I/O strings in tests is both practical and consistent.
| auto get_body() const -> msgpack::object { | ||
| // NOLINTBEGIN(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic) | ||
| msgpack::object const object = m_obj.get(); | ||
| return object.via.array.ptr[1]; | ||
| // NOLINTEND(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic) | ||
| } |
There was a problem hiding this comment.
Add bounds checking in get_body method.
The get_body() method accesses object.via.array.ptr[1] without checking if the array has at least 2 elements, which could lead to out-of-bounds access if called independently of get_type().
auto get_body() const -> msgpack::object {
// NOLINTBEGIN(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic)
msgpack::object const object = m_obj.get();
+ if (object.type != msgpack::type::ARRAY || object.via.array.size < 2) {
+ throw msgpack::type_error();
+ }
return object.via.array.ptr[1];
// NOLINTEND(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto get_body() const -> msgpack::object { | |
| // NOLINTBEGIN(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic) | |
| msgpack::object const object = m_obj.get(); | |
| return object.via.array.ptr[1]; | |
| // NOLINTEND(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic) | |
| } | |
| auto get_body() const -> msgpack::object { | |
| // NOLINTBEGIN(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic) | |
| msgpack::object const object = m_obj.get(); | |
| if (object.type != msgpack::type::ARRAY || object.via.array.size < 2) { | |
| throw msgpack::type_error(); | |
| } | |
| return object.via.array.ptr[1]; | |
| // NOLINTEND(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/spider/worker/TaskExecutorMessage.hpp (1)
70-74:⚠️ Potential issueAdd bounds checking in get_body method.
The
get_body()method accessesobject.via.array.ptr[1]without checking if the array has at least 2 elements, which could lead to out-of-bounds access if called independently ofget_type().auto get_body() const -> msgpack::object { // NOLINTBEGIN(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic) msgpack::object const object = m_obj.get(); + if (object.type != msgpack::type::ARRAY || object.via.array.size < 2) { + throw msgpack::type_error(); + } return object.via.array.ptr[1]; // NOLINTEND(cppcoreguidelines-pro-type-union-access,cppcoreguidelines-pro-bounds-pointer-arithmetic) }
🧹 Nitpick comments (5)
src/spider/worker/TaskExecutorMessage.hpp (3)
67-69: Improve documentation for get_body method.The current documentation notes that the return value "Cannot outlive the
TaskExecutorRequestParserobject," which is good. However, it would be helpful to also document the expected structure of the object and potential exceptions that might be thrown, especially after adding the bounds checking suggested in the other comment./** * @return The body of the message. Cannot outlive the `TaskExecutorRequestParser` object. + * @throw msgpack::type_error if the underlying object does not have the expected structure. */
46-47: Documentation mismatch in constructor.The constructor's documentation mentions a potential
std::bad_castexception, but this doesn't appear to match whatmsgpack::unpackwould throw. According to msgpack documentation, it would more likely throwmsgpack::insufficient_bytes,msgpack::parse_error, orstd::bad_alloc. Consider updating the documentation to reflect the actual exceptions that could be thrown.
52-65: Consider simplifying error handling.The
get_type()method has good error handling, checking both array type and size before accessing elements. However, you're returningTaskExecutorRequestType::Unknownfor both invalid array structure and invalid type conversion. For debugging purposes, it might be useful to distinguish between these different error cases, perhaps by adding more specific enum values or by logging.tests/client/client-test.cpp (2)
208-208: Consider if 300 characters is sufficient for a "large" input test.The constant
cLargeInputSizeis set to 300, which might not be large enough to thoroughly test the system's handling of large inputs, especially if the fix addresses dangling references in memory management. If the purpose is to stress test the system, a larger value might be more appropriate.
183-206: Consider adding documentation explaining test purpose.Adding a brief comment explaining the purpose of this test function, particularly its relation to testing memory management with large strings, would improve code readability and maintenance.
+/** + * Tests the system's ability to properly handle large string inputs and outputs + * to verify that memory management functions correctly, especially regarding + * serialization and deserialization of large data. + */ auto test_large_input_output( spider::Driver& driver, size_t const input_size_1, size_t const input_size_2 ) -> int {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/spider/worker/TaskExecutorMessage.hpp(1 hunks)tests/client/client-test.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (3)
src/spider/worker/TaskExecutorMessage.hpp (1)
40-79: Good approach using RAII to prevent dangling references.The introduction of
TaskExecutorRequestParserclass that holds themsgpack::object_handleis an effective solution to the dangling reference problem mentioned in the PR objectives. This follows the RAII (Resource Acquisition Is Initialization) pattern, ensuring the unpacked object remains valid as long as the parser exists.tests/client/client-test.cpp (2)
183-206: Test function looks correct and well implemented.This test function follows the pattern of other test functions in the file and properly validates the system's ability to handle larger string inputs and outputs. It creates two input strings of specified sizes, executes a job that joins them, and verifies both job success and correct result concatenation.
274-277: Integration with main function follows established pattern.The addition of the large input test to the main function follows the same pattern as other tests, making it a clean integration into the existing test suite.
davidlion
changed the title
msgpack::object in task executormsgpack::object in task executor; Increase test value sizes to cover this case.
davidlion
changed the title
msgpack::object in task executor; Increase test value sizes to cover this case.msgpack::object in task executor; Increase test value sizes to cover this case.
2 participants
Description
In
TaskExecutorMessage.hpp, theget_message_bodycreates amsgpack::object_handle, gets the message body object and returns it. However, when themsgpack::object_handlegoes out of scope and destructed, the underlying memory storing the object is freed, the return object is a dangling reference to invalid memory.This pr fixes the problem by introducing a class to hold the
msgpack::object_handleso it does not goes out of scope.This pr adds a unit test and an integration test for large task input and output, which expose the dangling reference bug. To support the tests, this pr increases the size of task input/output type and value, and the size of data. This fixes #68.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit