Skip to content
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

fix KillRoutineConnections panic #18726

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented Sep 12, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #https://github.com/matrixorigin/MO-Cloud/issues/4068 #18764

What this PR does / why we need it:

fix KillRoutineConnections panic


PR Type

Bug fix


Description

  • Fixed a potential panic in the KillRoutineConnections method by adding a check to ensure that the protocol is not nil before attempting to log and kill the connection.
  • This change prevents the application from crashing when trying to kill a connection with a nil protocol.

Changes walkthrough 📝

Relevant files
Bug fix
routine_manager.go
Fix panic by checking protocol before killing connection 

pkg/frontend/routine_manager.go

  • Added a check for rt.getProtocol() before logging and killing the
    connection.
  • Ensured that killConnection is only called if the protocol is not nil.

  • +4/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Null Check
    The PR adds a null check for rt.getProtocol() before logging and killing the connection. This prevents a potential panic, but it might be worth investigating why the protocol could be nil in the first place.

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Sep 12, 2024
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error logging for cases where the protocol is nil

    Consider logging an error or warning message when rt.getProtocol() is nil, to help
    with debugging and monitoring.

    pkg/frontend/routine_manager.go [588-591]

    -if rt.getProtocol() != nil {
    -  logutil.Infof("[kill connection] do kill connection account id %d, version %d, connection id %d, ", account, killRecord.version, rt.getConnectionID())
    +if protocol := rt.getProtocol(); protocol != nil {
    +  logutil.Infof("[kill connection] do kill connection account id %d, version %d, connection id %d", account, killRecord.version, rt.getConnectionID())
       rt.killConnection(false)
    +} else {
    +  logutil.Warnf("[kill connection] skipped for account id %d, version %d: nil protocol", account, killRecord.version)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances error handling by logging a warning when rt.getProtocol() is nil, which aids in debugging and monitoring. It is a valuable addition for understanding why certain connections are not being killed.

    8
    Performance
    Store the result of a method call in a temporary variable to avoid redundant calls

    Use a temporary variable to store the result of rt.getProtocol() to avoid multiple
    calls to the method.

    pkg/frontend/routine_manager.go [588-591]

    -if rt.getProtocol() != nil {
    -  logutil.Infof("[kill connection] do kill connection account id %d, version %d, connection id %d, ", account, killRecord.version, rt.getConnectionID())
    +if protocol := rt.getProtocol(); protocol != nil {
    +  logutil.Infof("[kill connection] do kill connection account id %d, version %d, connection id %d", account, killRecord.version, rt.getConnectionID())
       rt.killConnection(false)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a temporary variable to store the result of rt.getProtocol() improves performance by avoiding redundant method calls, making the code slightly more efficient.

    7
    Best practice
    Remove unnecessary trailing comma in a format string

    Remove the trailing comma in the logutil.Infof format string to improve code
    consistency and prevent potential issues.

    pkg/frontend/routine_manager.go [589]

    -logutil.Infof("[kill connection] do kill connection account id %d, version %d, connection id %d, ", account, killRecord.version, rt.getConnectionID())
    +logutil.Infof("[kill connection] do kill connection account id %d, version %d, connection id %d", account, killRecord.version, rt.getConnectionID())
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Removing the trailing comma in the format string is a minor improvement for code consistency and readability, but it does not significantly impact functionality or performance.

    5

    @mergify mergify bot requested a review from sukki37 September 12, 2024 03:12
    @mergify mergify bot added the kind/bug Something isn't working label Sep 12, 2024
    @YANGGMM
    Copy link
    Contributor Author

    YANGGMM commented Sep 12, 2024

    image image

    mysql> select connection_id();
    +-----------------+
    | connection_id() |
    +-----------------+
    | 201 |
    +-----------------+
    1 row in set (0.00 sec)

    mysql> select node_id, conn_id from processlist() a where conn_id = '201'
    -> ;
    +--------------------------------------+---------+
    | node_id | conn_id |
    +--------------------------------------+---------+
    | dd1dccb0-4d3c-41f8-b482-5251dc7a41bf | 201 |
    +--------------------------------------+---------+
    1 row in set (0.01 sec)

    mysql>
    mysql> select connection_id();
    ERROR 2013 (HY000): Lost connection to MySQL server during query
    No connection. Trying to reconnect...
    ERROR 1045 (28000): Access denied for user acc1:root. internal error: Account acc1 is suspended
    ERROR:
    Can't connect to the server

    mysql> select connection_id();
    +-----------------+
    | connection_id() |
    +-----------------+
    | 202 |
    +-----------------+
    1 row in set (0.01 sec)

    mysql> select connection_id();
    ERROR 2013 (HY000): Lost connection to MySQL server during query
    No connection. Trying to reconnect...
    ERROR 1045 (28000): Access denied for user acc1:root. internal error: Account acc1 is suspended
    ERROR:
    Can't connect to the server

    mysql>

    mysql> select connection_id();
    +-----------------+
    | connection_id() |
    +-----------------+
    | 203 |
    +-----------------+
    1 row in set (0.00 sec)

    mysql> select connection_id();
    ERROR 2013 (HY000): Lost connection to MySQL server during query
    No connection. Trying to reconnect...
    ERROR 1045 (28000): Access denied for user acc1:root. internal error: Account acc1 is suspended
    ERROR:
    Can't connect to the server

    mysql>

    改完以后测试正常

    @mergify mergify bot merged commit e821a3f into matrixorigin:1.2-dev Sep 12, 2024
    17 of 18 checks passed
    @YANGGMM YANGGMM deleted the fix-panic-1.2-dev branch October 15, 2024 07:40
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix kind/bug Something isn't working Review effort [1-5]: 2 size/XS Denotes a PR that changes [1, 9] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants