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] concurrent problem of sessionMgr #15210

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

ck89119
Copy link
Contributor

@ck89119 ck89119 commented Mar 28, 2024

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #15206

What this PR does / why we need it:

[Fix] concurrent problem of sessionMgr

@mergify mergify bot added the kind/bug Something isn't working label Mar 28, 2024
@mergify mergify bot mentioned this pull request Mar 28, 2024
7 tasks
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Mar 28, 2024
@matrix-meow
Copy link
Contributor

@ck89119 Thanks for your contributions!

Pull Request Review:

Title: [Fix] concurrent problem of sessionMgr

Problem 1: Inconsistent Naming Convention

  • The method GetAllStatusSessions introduced in SessionManager does not follow consistent naming conventions with the existing methods. It would be better to maintain uniformity in naming to improve code readability and maintainability. Consider renaming it to GetAllStatusSessions for consistency.

Problem 2: Inefficient Code

  • In the GetStatusSessionsByTenant method, the loop to iterate over sessions belonging to a specific tenant is incorrect. The loop should iterate over the values of the map, not just the keys. Update the loop to for _, session := range sm.mu.sessionsByTenant[tenant] to correctly append status sessions.

Problem 3: Potential Null Pointer Dereference

  • In the GetAllStatusSessions and GetStatusSessionsByTenant methods, there is a check for sm == nil, but the methods continue execution without returning an error or handling this case properly. It's essential to handle this scenario more gracefully, either by returning an error or logging a message before proceeding.

Problem 4: Lack of Error Handling

  • The methods GetAllStatusSessions and GetStatusSessionsByTenant do not handle potential errors that may occur during the execution, such as map access errors or other unexpected issues. It's crucial to add proper error handling mechanisms to provide more robust and reliable behavior.

Problem 5: Missing Unit Tests

  • The changes made in this pull request lack corresponding unit tests to validate the functionality and ensure that the modifications work as expected. It's recommended to include unit tests for the new methods introduced to maintain code quality and prevent regressions.

Optimization Suggestion:

  • Consider consolidating the common logic for retrieving status sessions into a shared private method to avoid code duplication and improve maintainability. This approach can help reduce redundancy and make future modifications easier to implement.

Security Concern:

  • Ensure that the concurrent access to shared data structures, such as sm.mu.sessionsByID and sm.mu.sessionsByTenant, is properly synchronized to prevent race conditions and data corruption. Verify that the read and write operations are appropriately protected using locks to maintain data integrity.

By addressing the identified issues and incorporating the suggested improvements, the codebase can be enhanced in terms of consistency, efficiency, error handling, and overall code quality. Additionally, implementing unit tests will contribute to the reliability and stability of the system.

@sukki37 sukki37 merged commit 0fa3ef6 into matrixorigin:1.1-dev Mar 28, 2024
15 of 18 checks passed
@ck89119 ck89119 deleted the fix/session_mgr_concurrent_1.1 branch March 28, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants