Permanent FF sessions + check_data_flow docs#5245
Conversation
Allow permanent fast-forward sessions (SESSION_FORWARD_TYPE_PERMANENT) to continue processing when bidirectional data flow is detected, instead of treating it as a fatal error. This prevents unnecessary session termination in these specific cases while maintaining the original strict validation for all other session types.
Summary of ChangesHello @renecannao, 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 introduces a crucial behavioral change for permanent fast-forward sessions, allowing them to maintain uninterrupted operation by gracefully handling bidirectional data flow with a warning instead of a fatal error. Concurrently, it significantly improves the clarity and maintainability of the codebase by adding detailed Doxygen documentation for the 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.
Code Review
This pull request introduces a special handling for permanent fast-forward sessions to allow bidirectional data flow, which is a useful feature for specific use cases. The implementation is correct, and it also fixes a pre-existing bug in the logging argument order. Additionally, the new Doxygen documentation for check_data_flow() is clear and greatly improves the code's understandability.
I have one suggestion to refactor the new logic slightly to improve readability and maintainability by extracting the condition into a boolean variable and deduplicating the log message format string.
| if ( (PSarrayIN->len || queue_data(queueIN) ) && ( PSarrayOUT->len || queue_data(queueOUT) ) ){ | ||
| // there is data at both sides of the data stream: this is considered a fatal error | ||
| proxy_error("Session=%p, DataStream=%p -- Data at both ends of a MySQL data stream: IN <%d bytes %d packets> , OUT <%d bytes %d packets>\n", sess, this, PSarrayIN->len , queue_data(queueIN) , PSarrayOUT->len , queue_data(queueOUT)); | ||
| shut_soft(); | ||
| generate_coredump(); | ||
| if (sess && sess->status == FAST_FORWARD && sess->session_fast_forward == SESSION_FORWARD_TYPE_PERMANENT) { | ||
| // Permanent fast-forward sessions: log warning but continue | ||
| proxy_warning("Session=%p, DataStream=%p -- Data at both ends of a MySQL data stream: IN <%d bytes %d packets> , OUT <%d bytes %d packets>\n", sess, this, queue_data(queueIN), PSarrayIN->len, queue_data(queueOUT), PSarrayOUT->len); | ||
| } else { | ||
| // All other sessions: treat as fatal error | ||
| proxy_error("Session=%p, DataStream=%p -- Data at both ends of a MySQL data stream: IN <%d bytes %d packets> , OUT <%d bytes %d packets>\n", sess, this, queue_data(queueIN), PSarrayIN->len, queue_data(queueOUT), PSarrayOUT->len); | ||
| shut_soft(); | ||
| generate_coredump(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling permanent fast-forward sessions is correct. However, the implementation can be improved for better readability and maintainability.
- The condition to check for a permanent fast-forward session is a bit long and makes the
ifstatement less readable. Extracting it into a boolean variable would clarify the intent. - The format string for logging is duplicated in both the
proxy_warningandproxy_errorcalls. This could lead to inconsistencies if one is updated and the other is not. It's better to define it as a constant.
Here is a suggested refactoring that addresses these points:
if ( (PSarrayIN->len || queue_data(queueIN) ) && ( PSarrayOUT->len || queue_data(queueOUT) ) ){
const bool is_permanent_ff = sess && sess->status == FAST_FORWARD && sess->session_fast_forward == SESSION_FORWARD_TYPE_PERMANENT;
const char* BIDI_DATA_LOG_MSG = "Session=%p, DataStream=%p -- Data at both ends of a MySQL data stream: IN <%d bytes %d packets> , OUT <%d bytes %d packets>\n";
if (is_permanent_ff) {
// Permanent fast-forward sessions: log warning but continue
proxy_warning(BIDI_DATA_LOG_MSG, sess, this, queue_data(queueIN), PSarrayIN->len, queue_data(queueOUT), PSarrayOUT->len);
} else {
// All other sessions: treat as fatal error
proxy_error(BIDI_DATA_LOG_MSG, sess, this, queue_data(queueIN), PSarrayIN->len, queue_data(queueOUT), PSarrayOUT->len);
shut_soft();
generate_coredump();
}
}
Features
Documentation
Closes #5244