(DAL) Possible fix for memory leak #1921
Conversation
|
Warning Rate limit exceeded@nick-bisonai has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 18 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. WalkthroughWalkthroughThe recent changes enhance the WebSocket functionality in the application. Key improvements include better logging for traceability, refactoring connection handling for clarity, and introducing batch processing for subscriptions. A new WebSocket controller facilitates real-time communication, while a performance testing script supports efficient testing with k6. Overall, these updates improve maintainability, performance, and error handling across the WebSocket-related codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebSocketController
participant Controller
participant Stats
Client->>WebSocketController: Connect
WebSocketController->>Controller: Retrieve instance
WebSocketController-->>Client: Connection established
Client->>WebSocketController: Send subscription request
WebSocketController->>Controller: Handle connection
Controller->>Stats: Insert subscription
Stats-->>Controller: Confirmation
Controller-->>WebSocketController: Subscription successful
WebSocketController-->>Client: Confirmation message
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (1)
node/pkg/dal/k6/script.js (1)
4-4: Ensure proper error handling for the ws import.The
wsmodule is used innode/pkg/dal/k6/script.jswithout any error handling mechanisms. It is important to add error handling around thews.connectcall and within the event handlers to manage potential issues during WebSocket communication.
node/pkg/dal/k6/script.js: Add try-catch blocks or other error handling mechanisms around thews.connectcall and within the event handlers.Analysis chain
Ensure proper error handling for the ws import.
The imported module should be checked for availability and proper error handling.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the availability of the ws module. # Test: Search for the module usage. Expect: Only occurrences of the correct import. rg --type js -A 5 $'import ws from "k6/ws"'Length of output: 2654
Script:
#!/bin/bash # Description: Check for error handling around the usage of the ws module in script.js. # Search for the usage of the ws module in script.js and include context lines to identify error handling mechanisms. rg --type js 'ws' -A 10 -B 10 node/pkg/dal/k6/script.jsLength of output: 4777
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- node/pkg/dal/api/controller.go (5 hunks)
- node/pkg/dal/api/websocketcontroller.go (1 hunks)
- node/pkg/dal/k6/script.js (1 hunks)
- node/pkg/dal/tests/api_test.go (1 hunks)
- node/pkg/dal/utils/stats/stats.go (1 hunks)
- node/pkg/wss/utils.go (1 hunks)
Additional context used
Learnings (2)
node/pkg/dal/api/websocketcontroller.go (2)
Learnt from: nick-bisonai PR: Bisonai/orakl#1791 File: node/pkg/dal/api/controller.go:0-0 Timestamp: 2024-07-12T02:42:57.258Z Learning: In the `node/pkg/dal/api/controller.go`, the `handleWebsocket` function no longer requires explicit validation for the `X-Api-Key` header, as this is handled by the `gofiber keyauth` middleware in `initializer.go`.Learnt from: nick-bisonai PR: Bisonai/orakl#1791 File: node/pkg/dal/api/controller.go:109-113 Timestamp: 2024-07-12T01:57:43.211Z Learning: In the `node/pkg/dal/api/controller.go`, the `handleWebsocket` function includes logic to extract and validate the `X-Api-Key` header.node/pkg/wss/utils.go (1)
Learnt from: nick-bisonai PR: Bisonai/orakl#1524 File: node/pkg/wss/utils.go:0-0 Timestamp: 2024-05-28T05:35:40.840Z Learning: The `Run` method in `WebsocketHelper` now includes a context-based mechanism to exit the loop and a flag (`IsRunning`) to prevent multiple instances from running simultaneously.
Additional comments not posted (15)
node/pkg/dal/api/websocketcontroller.go (2)
19-24: LGTM! Ensure the custom close handler is thoroughly tested.The custom close handler correctly unregisters the connection and logs the event.
68-93: LGTM! Ensure subscription handling is thoroughly tested.The subscription handling logic appears correct, with proper locking and insertion of subscription statistics.
node/pkg/dal/utils/stats/stats.go (1)
74-80: LGTM! Ensure bulk insertion is thoroughly tested.The bulk insertion logic appears correct and should improve performance for multiple subscriptions.
node/pkg/dal/k6/script.js (2)
64-81: LGTM! Ensure WebSocket stress test is thoroughly tested.The WebSocket stress test logic appears correct and should help identify memory leaks.
1-3: Ensure proper error handling for the randomIntBetween import.The imported function should be checked for availability and proper error handling.
Verification successful
The
randomIntBetweenfunction is imported and used correctly.The function is imported from a reliable external library and used appropriately in the script.
- The import statement is correct.
- The function is used to set the
sessionDurationvariable.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the availability of the randomIntBetween function. # Test: Search for the function usage. Expect: Only occurrences of the correct import. rg --type js -A 5 $'randomIntBetween'Length of output: 2871
node/pkg/dal/tests/api_test.go (1)
166-220: Improved Readability and OrganizationThe restructuring of the
TestApiWebsocketfunction into a sub-test "test subscription" enhances readability and organization. This change makes the test's intent clearer and the sequence of operations more understandable.node/pkg/dal/api/controller.go (8)
22-22: Enhanced Logging for Config RetrievalThe logging statement adds context by including the "Player" field, which improves traceability.
32-32: Enhanced Logging for Collector CreationThe logging statement adds context by including the "Player" field, which improves traceability.
56-58: Enhanced Logging for Collector and Connection Handler StartThe logging statements add context by including the "Player" field, which improves traceability.
62-86: Refactored Connection HandlingThe refactored
handleConnectionmethod improves readability and maintainability. The use ofsync.RWMutexensures safe concurrent access to theclientsmap.
88-98: New Method for Broadcasting DataThe
startBroadCastmethod improves the clarity of the broadcast setup process by encapsulating the logic in a dedicated method.
117-125: Simplified Function SignatureThe
castSubmissionDatamethod now accepts a string for the symbol instead of a pointer. This change simplifies the function signature and eliminates unnecessary pointer dereferencing, improving performance.
49-49: Removal of Unused CodeThe removal of commented-out sections related to websocket connection handling streamlines the file and focuses on the essential logic.
Line range hint
105-111: Correct Mapping of Configuration ID to SymbolThe
configIdToSymbolmethod correctly maps a configuration ID to a symbol, ensuring the correct symbol is used for broadcasting data.node/pkg/wss/utils.go (1)
284-296: New Method for Checking WebSocket Connection StatusThe
IsAlivemethod enhances the functionality by checking the status of the WebSocket connection. It correctly handles the connection status and logs errors if the ping fails.
Description
CloseHandler
Custom closeHandler which overwrites default closeHandler so that it can automatically signal unregister on disconnection.
Refactoring
websocketcontrollerfromcontrollerhandleMessagefromhandleWebsocketfunctionController.Startmethod for readabilityMinor update
InsertWebsocketSubscriptionsfor bulk insertion of subscription statsisAlivefunc to be later used in test codes so that it can check conn live from client sideType of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment