- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25
 
Add configurable limit on consecutive client response timeouts #166
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
Conversation
…ber of failed requests
          Summary of ChangesHello @jadamcrain, 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 enhances the Modbus client's resilience by introducing a configurable mechanism to detect and recover from unresponsive connections. By allowing users to specify a limit on consecutive response timeouts, the system can proactively reset a session, preventing indefinite hangs and improving overall reliability, especially in scenarios where remote devices might silently fail or network issues lead to dropped packets without explicit connection termination signals. 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
  | 
    
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.
Code Review
This pull request introduces a configurable limit on consecutive client response timeouts to enhance connection stability. The implementation for the TCP client is solid and includes good test coverage. However, the feature is currently not functional for serial clients as the configuration is hardcoded to be disabled. Additionally, there's a minor logic issue in the timeout counter that could cause problems in an extreme edge case. The review includes suggestions to address these points.
| FrameWriter::rtu(), | ||
| FramedReader::rtu_response(), | ||
| decode, | ||
| None, | 
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.
The max_timeouts option is hardcoded to None for the serial client, which disables the new consecutive timeout limit feature. The PR description and the addition of SessionError::MaxTimeouts handling for serial connections suggest this feature should be available for serial clients as well. To fix this, the max_timeouts value should be configurable, likely by passing ClientOptions to SerialChannelTask::new similarly to how it's done for the TCP client.
        
          
                rodbus/src/client/task.rs
              
                Outdated
          
        
      | match &mut self.state { | ||
| TimeoutCounterState::Disabled => Ok(()), | ||
| TimeoutCounterState::Enabled { current, max } => { | ||
| *current = current.wrapping_add(1); | 
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.
Using wrapping_add here could lead to unexpected behavior in an extreme edge case. If current reaches usize::MAX, it will wrap around to 0, effectively resetting the timeout counter instead of keeping it at its maximum value. This would prevent the MaxTimeouts error from being triggered if the number of timeouts is close to usize::MAX.
Using saturating_add would be safer and more correctly reflect the intent of capping the counter at a maximum value.
| *current = current.wrapping_add(1); | |
| *current = current.saturating_add(1); | 
• PR Title
Add configurable limit on consecutive client response timeouts
PR Description
Summary
Fixes #144.