-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[Benchmark] Fix client benchmark module and improve stability & coverage #630
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
JueBaby seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
It looks wonderful. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes critical compilation errors in the client benchmark module and enhances its stability, security, and functionality. The changes enable the previously non-functional benchmark code to compile and run properly with improved error handling, thread-safety, and resource management.
Key changes include:
- Fixed compilation errors (RC::SUCESS → RC::SUCCESS typo, missing return values, removed
#if 0wrapper) - Enhanced thread-safety by replacing gethostbyname with getaddrinfo
- Added comprehensive input validation and buffer overflow protection
- Implemented complete benchmark infrastructure with SetUp/TearDown lifecycle management and example test case
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 校验参数合法性(UNIX_PATH_MAX 为系统定义的最大路径长度) | ||
| if (unix_socket.empty() || unix_socket.size() >= UNIX_PATH_MAX) { | ||
| LOG_WARN("invalid unix socket path: %s (length=%zu, max=%d)", | ||
| unix_socket.c_str(), unix_socket.size(), UNIX_PATH_MAX); |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNIX_PATH_MAX is not defined. Use sizeof(sockaddr_un::sun_path) instead, which is the portable way to check the Unix socket path length limit.
| // 校验参数合法性(UNIX_PATH_MAX 为系统定义的最大路径长度) | |
| if (unix_socket.empty() || unix_socket.size() >= UNIX_PATH_MAX) { | |
| LOG_WARN("invalid unix socket path: %s (length=%zu, max=%d)", | |
| unix_socket.c_str(), unix_socket.size(), UNIX_PATH_MAX); | |
| // 校验参数合法性(使用 sizeof(addr.sun_path) 作为最大路径长度) | |
| if (unix_socket.empty() || unix_socket.size() >= sizeof(addr.sun_path)) { | |
| LOG_WARN("invalid unix socket path: %s (length=%zu, max=%zu)", | |
| unix_socket.c_str(), unix_socket.size(), sizeof(addr.sun_path)); |
| int writen(int fd, const void *buf, size_t n) | ||
| { | ||
| size_t nleft = n; |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function signature mismatch. The existing writen in src/common/io/io.h:68 uses 'int size' not 'size_t n'. This will cause linking errors or duplicate symbol issues. Change to match: int writen(int fd, const void *buf, int size).
| int writen(int fd, const void *buf, size_t n) | |
| { | |
| size_t nleft = n; | |
| int writen(int fd, const void *buf, int size) | |
| { | |
| int nleft = size; |
| Client client_; // 基准测试使用的客户端实例 | ||
| // 可配置参数(通过 benchmark 命令行传递或硬编码默认值) | ||
| string host_ = "127.0.0.1"; // 服务器主机 | ||
| int port_ = 1024; // 服务器端口 |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Port 1024 is the first non-privileged port but may conflict with system services. Consider using a higher default port like 6789 or obtaining it from configuration.
| int port_ = 1024; // 服务器端口 | |
| int port_ = 6789; // 服务器端口 |
| if (sql == nullptr || strlen(sql) == 0) { | ||
| LOG_WARN("send_sql failed: sql is empty"); | ||
| return RC::INVALID_ARGUMENT; | ||
| } | ||
|
|
||
| // 调用 miniob 封装的 writen 函数(保证数据完整发送) | ||
| int ret = writen(socket_fd_, sql, strlen(sql) + 1); // 包含 '\0' 终止符 |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling strlen(sql) after checking sql == nullptr will cause undefined behavior. Check for nullptr first, then check length in a separate condition or use strlen only if sql is not null.
| if (sql == nullptr || strlen(sql) == 0) { | |
| LOG_WARN("send_sql failed: sql is empty"); | |
| return RC::INVALID_ARGUMENT; | |
| } | |
| // 调用 miniob 封装的 writen 函数(保证数据完整发送) | |
| int ret = writen(socket_fd_, sql, strlen(sql) + 1); // 包含 '\0' 终止符 | |
| if (sql == nullptr) { | |
| LOG_WARN("send_sql failed: sql is nullptr"); | |
| return RC::INVALID_ARGUMENT; | |
| } | |
| size_t sql_len = strlen(sql); | |
| if (sql_len == 0) { | |
| LOG_WARN("send_sql failed: sql is empty"); | |
| return RC::INVALID_ARGUMENT; | |
| } | |
| // 调用 miniob 封装的 writen 函数(保证数据完整发送) | |
| int ret = writen(socket_fd_, sql, sql_len + 1); // 包含 '\0' 终止符 |
| // 校验参数合法性(UNIX_PATH_MAX 为系统定义的最大路径长度) | ||
| if (unix_socket.empty() || unix_socket.size() >= UNIX_PATH_MAX) { | ||
| LOG_WARN("invalid unix socket path: %s (length=%zu, max=%d)", | ||
| unix_socket.c_str(), unix_socket.size(), UNIX_PATH_MAX); |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UNIX_PATH_MAX is used but not defined. This will cause a compilation error. Use sizeof(sockaddr_un::sun_path) instead.
| // 校验参数合法性(UNIX_PATH_MAX 为系统定义的最大路径长度) | |
| if (unix_socket.empty() || unix_socket.size() >= UNIX_PATH_MAX) { | |
| LOG_WARN("invalid unix socket path: %s (length=%zu, max=%d)", | |
| unix_socket.c_str(), unix_socket.size(), UNIX_PATH_MAX); | |
| // 校验参数合法性(sun_path 为系统定义的最大路径长度) | |
| if (unix_socket.empty() || unix_socket.size() >= sizeof(sockaddr_un::sun_path)) { | |
| LOG_WARN("invalid unix socket path: %s (length=%zu, max=%zu)", | |
| unix_socket.c_str(), unix_socket.size(), sizeof(sockaddr_un::sun_path)); |
Problem:
The client benchmark module of miniob had critical compilation errors, including misspelled RC::SUCESS (should be RC::SUCCESS), missing return values in receive_result function, and unresolved dependency on writen utility function, leading to complete code inoperability.
There were severe stability and security risks: using thread-unsafe gethostbyname (crashes in multi-threaded benchmark scenarios), no validation for Unix Socket path length (risk of buffer overflow), and lack of checks for duplicate socket closure.
Functional incompleteness: the base class BenchmarkBase lacked core logic for client initialization and resource cleanup, no practical benchmark test cases, and inefficient small buffer (256B) causing frequent system calls.
Poor code readability and maintainability: overloaded init functions (confusing for TCP/Unix Socket), missing key comments, and inconsistent error handling (mixed RC enums and errno).
What is changed and how it works?
Fix Compilation Errors
Corrected the misspelling RC::SUCESS to RC::SUCCESS; added return value logic for receive_result to ensure consistent RC return type.
Built-in writen function implementation (controlled by HAVE_WRITEN macro) to resolve dependency issues, with automatic compatibility if the function already exists in the project.
Enhance Stability & Compatibility
Replaced thread-unsafe gethostbyname with getaddrinfo (POSIX standard, thread-safe, supports IPv4/IPv6 adaptation) to fix multi-threaded crash risks.
Added parameter validation: checked TCP port range (1-65535), Unix Socket path length (≤ UNIX_PATH_MAX), and null pointers for input SQL to prevent invalid operations.
Optimized socket resource management: used socket_fd_ = -1 to mark disconnected state, avoiding duplicate closure; added is_connected() method for connection status checks.
Improve Functional Completeness
Implemented SetUp/TearDown in BenchmarkBase: automatically initializes client connections (TCP/Unix Socket configurable via use_tcp_) before tests and closes connections after tests to avoid resource leaks.
Added SimpleQueryBenchmark example test case (tests SELECT 1 execution performance) and registered parameterized ranges (1-10000 executions) to support flexible performance testing.
Increased receive buffer from 256B to 4096B to reduce system call frequency and improve data transfer efficiency.
Optimize Code Quality
Split overloaded init into init_tcp and init_unix for clear distinction between connection types, eliminating ambiguity.
Added detailed comments for functions, parameters, and core logic; unified error handling to use RC enums for consistent error reporting.
How it works:
The Client class now supports both TCP and Unix Socket connections with validations and thread-safe address resolution.
The BenchmarkBase base class manages client lifecycle automatically, allowing subclasses to focus on test logic.
Benchmark test cases run in loops (controlled by Google Benchmark), executing SQL and collecting performance metrics (e.g., latency per execution) while ensuring resource cleanup after tests.
Other information
Dependencies: Relies on existing common/log/log.h (logging module) and rc.h (error code definition) in miniob; no new third-party dependencies.
Compatibility: The built-in writen function is guarded by HAVE_WRITEN macro—if the project already has common/net/io.h with writen, the built-in implementation will be automatically skipped to avoid duplicate definitions.