feat: add tcp keep alive #110
Conversation
|
Warning Rate limit exceeded@lankunGitHub has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis pull request introduces TCP keep-alive functionality to the KiwiDB system. The changes span multiple files, adding a new configuration option for TCP keep-alive with a default value of 300 seconds. A new Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration
participant NetOptions as NetOptions
participant EventServer as Event Server
participant BaseSocket as Base Socket
Config->>NetOptions: Set TCP Keep-Alive
NetOptions->>EventServer: Configure Keep-Alive
EventServer->>BaseSocket: Apply Keep-Alive Settings
BaseSocket-->>BaseSocket: Set Socket Options
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
| @@ -16,6 +16,9 @@ ip 127.0.0.1 | |||
| # Close the connection after a client is idle for N seconds (0 to disable) | |||
| timeout 0 | |||
|
|
|||
| # Enable the tcp keep-alive function (0 to disable) | |||
| tcp-keepalive 300 | |||
There was a problem hiding this comment.
我认为在注释里加上时间的单位会好一点，像16 line 一样
src/net/socket_addr.h
Outdated
| @@ -9,6 +9,7 @@ | |||
|
|
|||
| #include <arpa/inet.h> | |||
| #include <netinet/in.h> | |||
| #include <netinet/tcp.h> | |||
There was a problem hiding this comment.
我认为不要在这里加头文件，在需要它的地方再加
There was a problem hiding this comment.
这个keeplive 和我最开始设想的不一样, 这个只是操作系统提供的一个探活选项,但是不能超时自动断开
redis的keeplieve可以参考
int clientsCronHandleTimeout(client *c, mstime_t now_ms) {
不过可以先实现这个, 自动断开的keeplive后面再考虑
src/client.cc
Outdated
| @@ -288,6 +288,49 @@ bool PClient::SendPacket(UnboundedBuffer& data) { | |||
| return true; | |||
| } | |||
|
|
|||
| void PClient::SetTcpKeepAlive(int fd) { | |||
There was a problem hiding this comment.
这部分功能应该放到net目录中,对client来说是透明的
src/net/thread_manager.h
Outdated
| @@ -114,7 +114,8 @@ class ThreadManager { | |||
| }; | |||
|
|
|||
| template <typename T> | |||
| requires HasSetFdFunction<T> ThreadManager<T>::~ThreadManager() { Stop(); } | |||
| requires HasSetFdFunction<T> | |||
| ThreadManager<T>::~ThreadManager() { Stop(); } | |||
There was a problem hiding this comment.
这里恢复,要不然格式化不通过, 使用 clang-format-13这个版本的进行格式化,
src/net/base_socket.h
Outdated
| protected: | ||
| bool NoBlock() const { return noBlock_; } | ||
|
|
||
| private: | ||
| int type_ = SOCKET_NONE; // socket type (TCP/UDP) | ||
| bool noBlock_ = true; | ||
| uint32_t tcpKeepAlive_; // TCP keepalive |
There was a problem hiding this comment.
把变量名称改为 uint32_t tcp_keep_alive_ = 300;
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/net/net_options.h (2)
25-27: Add input validation and documentation for TCP keep-alive timeout.Consider adding validation for the timeout value and documenting the units and valid range.
- void SetOpTcpKeepAlive(uint32_t tcpKeepAlive) { tcpKeepAlive_ = tcpKeepAlive; } + /** + * Sets the TCP keep-alive timeout in seconds. + * @param tcpKeepAlive The timeout value in seconds. Set to 0 to disable keep-alive. + * @throws std::invalid_argument if tcpKeepAlive exceeds system limits + */ + void SetOpTcpKeepAlive(const uint32_t tcpKeepAlive) { + // Most systems limit TCP_KEEPIDLE to 32767 + if (tcpKeepAlive > 32767 && tcpKeepAlive != 0) { + throw std::invalid_argument("TCP keep-alive timeout exceeds maximum value"); + } + tcpKeepAlive_ = tcpKeepAlive; + }
34-34: Document the default keep-alive timeout value.Add a comment explaining why 300 seconds was chosen as the default value.
- uint32_t tcpKeepAlive_ = 300; // The timeout of the keepalive connection in seconds + uint32_t tcpKeepAlive_ = 300; // Default TCP keep-alive timeout (300s) based on common production settingssrc/net/base_socket.cc (1)
63-104: Enhance error handling and parameter validation.The implementation looks good but could benefit from:
- More descriptive error messages including the actual values being set
- System-specific parameter validation
- Documentation for the keep-alive parameters
void BaseSocket::SetTcpKeepAlive() { if (tcpKeepAlive_ == 0) { return; } int enabled = 1; uint32_t idle = tcpKeepAlive_; - uint32_t intvl = idle / 3; - int cnt = 3; + // Keep-alive probe interval (recommended: 1/3 of idle time) + uint32_t intvl = std::max(uint32_t(1), idle / 3); + // Number of unacknowledged probes before connection is dropped + int cnt = 3; if (setsockopt(Fd(), SOL_SOCKET, SO_KEEPALIVE, &enabled, sizeof(enabled)) == -1) { - WARN("SetTcpKeepAlive fd:{} error:{}", Fd(), errno); + WARN("Failed to enable TCP keep-alive on fd:{}, error:{} ({})", + Fd(), errno, strerror(errno)); return; } #ifdef TCP_KEEPIDLE if (setsockopt(Fd(), IPPROTO_TCP, TCP_KEEPIDLE, &idle, sizeof(idle))) { - WARN("SetTcpKeepAlive fd:{} error:{}", Fd(), errno); + WARN("Failed to set TCP_KEEPIDLE={} on fd:{}, error:{} ({})", + idle, Fd(), errno, strerror(errno)); return; } #elif defined(TCP_KEEPALIVE) /* support MacOS */ if (setsockopt(Fd(), IPPROTO_TCP, TCP_KEEPALIVE, &idle, sizeof(idle))) { - WARN("SetTcpKeepAlive fd:{} error:{}", Fd(), errno); + WARN("Failed to set TCP_KEEPALIVE={} on fd:{}, error:{} ({})", + idle, Fd(), errno, strerror(errno)); return; } #endif #ifdef TCP_KEEPINTVL if (setsockopt(Fd(), IPPROTO_TCP, TCP_KEEPINTVL, &intvl, sizeof(intvl))) { - WARN("SetTcpKeepAlive fd:{} error:{}", Fd(), errno); + WARN("Failed to set TCP_KEEPINTVL={} on fd:{}, error:{} ({})", + intvl, Fd(), errno, strerror(errno)); return; } #endif #ifdef TCP_KEEPCNT if (setsockopt(Fd(), IPPROTO_TCP, TCP_KEEPCNT, &cnt, sizeof(cnt))) { - WARN("SetTcpKeepAlive fd:{} error:{}", Fd(), errno); + WARN("Failed to set TCP_KEEPCNT={} on fd:{}, error:{} ({})", + cnt, Fd(), errno, strerror(errno)); return; } #endif }src/config.cc (1)
140-140: Add range validation for tcp-keepalive configuration.Consider using
AddNumberWithLimitto enforce valid range for the TCP keep-alive timeout.- AddNumber("tcp-keepalive", true, &tcp_keepalive); + // Most systems limit TCP_KEEPIDLE to 32767 + AddNumberWithLimit<uint32_t>("tcp-keepalive", true, &tcp_keepalive, 0, 32767);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
etc/conf/kiwi.conf(1 hunks)src/config.cc(1 hunks)src/config.h(1 hunks)src/kiwi.cc(1 hunks)src/net/base_socket.cc(2 hunks)src/net/base_socket.h(2 hunks)src/net/event_server.h(2 hunks)src/net/net_options.h(1 hunks)src/net/thread_manager.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/config.h
- src/net/event_server.h
- src/kiwi.cc
- etc/conf/kiwi.conf
- src/net/thread_manager.h
- src/net/base_socket.h
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (2)
src/net/base_socket.cc (1)
34-34: Consider adding error handling for TCP keep-alive initialization.The
OnCreatemethod should handle potential errors fromSetTcpKeepAlive.src/config.cc (1)
140-143:⚠️ Potential issueRemove duplicate configuration entries.
The following configuration entries are duplicated:
- db-path
- loglevel
- logfile
AddNumber("tcp-keepalive", true, &tcp_keepalive); AddString("db-path", false, {&db_path}); AddStringWithFunc("loglevel", &CheckLogLevel, false, {&log_level}); AddString("logfile", false, {&log_dir}); - AddString("db-path", false, &db_path); - AddStringWithFunc("loglevel", &CheckLogLevel, false, &log_level); - AddString("logfile", false, &log_dir);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/config.cc (1)
140-140: Add value limits for tcp-keepalive parameterConsider using
AddNumberWithLimitinstead ofAddNumberto enforce reasonable bounds for the TCP keep-alive timeout. This helps prevent extreme values that could impact system performance.- AddNumber("tcp-keepalive", true, &tcp_keepalive); + AddNumberWithLimit<uint32_t>("tcp-keepalive", true, &tcp_keepalive, 0, 3600); // 0 to disable, max 1 hour
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/config.cc(1 hunks)src/net/base_socket.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/net/base_socket.h
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_macos
🔇 Additional comments (1)
src/config.cc (1)
140-140: Verify TCP keep-alive implementation across the codebaseLet's verify the consistent usage of the tcp-keepalive configuration and proper error handling in socket operations.
Run the following script to analyze the implementation:
✅ Verification successful
TCP keep-alive implementation is properly integrated and robust
The verification confirms:
- Proper error handling for all socket operations including keep-alive setup
- Thread-safe socket resource management with atomic operations
- Consistent configuration usage from config through to socket setup
- Clean socket cleanup in all execution paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify TCP keep-alive implementation across the codebase # Check for proper error handling in socket operations echo "Checking error handling in socket operations..." rg -A 5 "SetTcpKeepAlive|setsockopt.*SO_KEEPALIVE" # Verify consistent usage of the configuration value echo "Checking usage of tcp_keepalive configuration..." rg -A 3 "tcp_keepalive" # Check for potential memory leaks in socket handling echo "Checking socket resource management..." ast-grep --pattern 'class $_Socket { $$$ ~$_Socket() { $$$ } $$$ }'Length of output: 3245
Script:
#!/bin/bash # Check socket cleanup implementation echo "Checking socket cleanup..." rg -A 5 "~BaseSocket|Close\(\)" src/net/ # Verify socket fd management echo "Checking socket fd handling..." rg "fd_\s*=|Fd\(\)" src/net/base_socketLength of output: 3247
| AddString("db-path", false, &db_path); | ||
| AddStringWithFunc("loglevel", &CheckLogLevel, false, &log_level); | ||
| AddString("logfile", false, &log_dir); |
There was a problem hiding this comment.
Remove duplicate configuration entries
The following configuration parameters are defined twice:
- db-path
- loglevel
- logfile
This appears to be a merge conflict or copy-paste error. Please remove the duplicate entries.
AddNumber("tcp-keepalive", true, &tcp_keepalive);
AddString("db-path", false, &db_path);
AddStringWithFunc("loglevel", &CheckLogLevel, false, &log_level);
AddString("logfile", false, &log_dir);
- AddString("db-path", false, &db_path);
- AddStringWithFunc("loglevel", &CheckLogLevel, false, &log_level);
- AddString("logfile", false, &log_dir);Also applies to: 144-146
Title
Add TCP KeepAlive Support #103
Description
tcp keepalive is enabled by default. You can use the tcp-keepalive option in the kiwi.conf file to change the value. If the value is set to 0, this function is disabled
Summary by CodeRabbit
New Features
Configuration Updates
tcp-keepalivedirective with a default 300-second interval.Performance Improvements