fix(dapi): getStatus cache invalidation #2155
Conversation
WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant WebSocketClient
participant BlockchainListener
WebSocketClient->>BlockchainListener: Connect
BlockchainListener->>BlockchainListener: #subscribe()
BlockchainListener->>WebSocketClient: Subscribe to TX_QUERY
BlockchainListener->>WebSocketClient: Subscribe to NEW_BLOCK_QUERY
WebSocketClient->>BlockchainListener: NEW_BLOCK event
BlockchainListener->>WebSocketClient: Emit EVENTS.NEW_BLOCK
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
packages/dapi/lib/externalApis/tenderdash/BlockchainListener.js (1)
72-74: Good addition of the#subscribemethod for centralized subscription logic.The new
#subscribemethod effectively encapsulates the subscription logic for both new blocks and transactions. This improves code organization and reusability, aligning well with the PR objectives.Consider adding basic error handling to make the method more robust:
#subscribe() { - this.wsClient.subscribe(NEW_BLOCK_QUERY); - this.wsClient.subscribe(TX_QUERY); + try { + this.wsClient.subscribe(NEW_BLOCK_QUERY); + this.wsClient.subscribe(TX_QUERY); + } catch (error) { + logger.error('Failed to subscribe to blockchain events', { error: error.message }); + } }This change will ensure that any subscription errors are logged, making it easier to diagnose issues in production.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/dapi/lib/externalApis/tenderdash/BlockchainListener.js (1 hunks)
- packages/dapi/lib/grpcServer/handlers/platform/getStatusHandlerFactory.js (3 hunks)
Additional comments not posted (6)
packages/dapi/lib/externalApis/tenderdash/BlockchainListener.js (2)
61-69: Excellent implementation of block subscription and connection handling!The changes in the
startmethod effectively address the PR objectives:
- Subscribing to new blocks (line 61-62) enables cache invalidation based on new block events.
- The 'connect' event listener (lines 63-65) ensures resubscription upon WebSocket reconnection.
- The immediate subscription check (lines 67-69) guarantees that subscriptions are set up if the client is already connected when
startis called.These improvements enhance the robustness of the blockchain listening mechanism and should resolve the caching issues mentioned in the PR objectives.
Line range hint
1-87: Overall, excellent implementation addressing the PR objectives.The changes to the
BlockchainListenerclass effectively solve the caching and subscription issues mentioned in the PR objectives. The code is well-structured, with clear separation of concerns and improved error handling. The new#subscribemethod and the enhancedstartmethod work together to ensure robust subscription management and event handling.Key improvements:
- Subscription to new blocks for cache invalidation.
- Resubscription logic on WebSocket reconnection.
- Immediate subscription when the client is already connected.
The suggested enhancements for error handling and logging will further improve the robustness and observability of the system. Great job on this implementation!
packages/dapi/lib/grpcServer/handlers/platform/getStatusHandlerFactory.js (4)
12-12: Logger import added correctlyThe addition of the logger import is appropriate for the new logging functionality.
22-22: 'cleanCacheTimeout' variable initializedInitializing
cleanCacheTimeouttonullsets up the variable for managing cache cleanup scheduling.
24-34: Cache cleanup function is well-implementedThe
cleanCachefunction effectively clears the cached response and cancels any existing scheduled cache cleanup. The logging statement enhances traceability.
37-37: Subscribed to 'NEW_BLOCK' event for cache invalidationSubscribing to the
NEW_BLOCKevent ensures the cache is invalidated upon new block detection, aligning with the cache invalidation strategy.
| this.wsClient.on(NEW_BLOCK_QUERY, (message) => this.emit(EVENTS.NEW_BLOCK, message)); | ||
|
|
||
| this.wsClient.on('connect', () => { | ||
| this.#subscribe(); | ||
| }); | ||
|
|
||
| if (this.wsClient.isConnected) { | ||
| this.#subscribe(); | ||
| } | ||
| } | ||
|
|
||
| #subscribe() { | ||
| this.wsClient.subscribe(NEW_BLOCK_QUERY); | ||
| this.wsClient.subscribe(TX_QUERY); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing logging for better traceability of the subscription process.
The changes effectively implement the required improvements to the blockchain listening mechanism. To further enhance the system's observability, consider adding more detailed logging:
start() {
const processLogger = logger.child({
process: 'BlockchainListener',
});
- processLogger.info('Subscribed to state transition results');
+ processLogger.info('Starting BlockchainListener');
// ... (existing code)
this.wsClient.on(NEW_BLOCK_QUERY, (message) => {
+ processLogger.debug('Received new block', { height: message.block.header.height });
this.emit(EVENTS.NEW_BLOCK, message)
});
this.wsClient.on('connect', () => {
+ processLogger.info('WebSocket connected, subscribing to events');
this.#subscribe();
});
if (this.wsClient.isConnected) {
+ processLogger.info('WebSocket already connected, subscribing to events');
this.#subscribe();
}
}
#subscribe() {
+ processLogger.debug('Subscribing to blockchain events');
try {
this.wsClient.subscribe(NEW_BLOCK_QUERY);
this.wsClient.subscribe(TX_QUERY);
+ processLogger.info('Successfully subscribed to blockchain events');
} catch (error) {
logger.error('Failed to subscribe to blockchain events', { error: error.message });
}
}These logging enhancements will provide better visibility into the subscription process, making it easier to track the system's behavior and diagnose any issues that may arise.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.wsClient.on(NEW_BLOCK_QUERY, (message) => this.emit(EVENTS.NEW_BLOCK, message)); | |
| this.wsClient.on('connect', () => { | |
| this.#subscribe(); | |
| }); | |
| if (this.wsClient.isConnected) { | |
| this.#subscribe(); | |
| } | |
| } | |
| #subscribe() { | |
| this.wsClient.subscribe(NEW_BLOCK_QUERY); | |
| this.wsClient.subscribe(TX_QUERY); | |
| this.wsClient.on(NEW_BLOCK_QUERY, (message) => { | |
| processLogger.debug('Received new block', { height: message.block.header.height }); | |
| this.emit(EVENTS.NEW_BLOCK, message) | |
| }); | |
| this.wsClient.on('connect', () => { | |
| processLogger.info('WebSocket connected, subscribing to events'); | |
| this.#subscribe(); | |
| }); | |
| if (this.wsClient.isConnected) { | |
| processLogger.info('WebSocket already connected, subscribing to events'); | |
| this.#subscribe(); | |
| } | |
| } | |
| #subscribe() { | |
| processLogger.debug('Subscribing to blockchain events'); | |
| try { | |
| this.wsClient.subscribe(NEW_BLOCK_QUERY); | |
| this.wsClient.subscribe(TX_QUERY); | |
| processLogger.info('Successfully subscribed to blockchain events'); | |
| } catch (error) { | |
| logger.error('Failed to subscribe to blockchain events', { error: error.message }); | |
| } | |
| } |
| // Clean cache in 3 minutes | ||
| cleanCacheTimeout = setTimeout(cleanCache, 3 * 60 * 1000); | ||
|
|
There was a problem hiding this comment.
Potential for multiple timeouts—ensure a single scheduled cache cleanup
Each time getStatusHandler generates a new cachedResponse, a new cleanCacheTimeout is set without clearing any existing timeout. This could lead to multiple timeouts running concurrently, causing redundant executions of cleanCache.
To prevent multiple scheduled timeouts, clear any existing timeout before setting a new one:
+ // Cancel any existing scheduled cache cleanup
+ if (cleanCacheTimeout !== null) {
+ clearTimeout(cleanCacheTimeout);
+ cleanCacheTimeout = null;
+ }
// Clean cache in 3 minutes
cleanCacheTimeout = setTimeout(cleanCache, 3 * 60 * 1000);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Clean cache in 3 minutes | |
| cleanCacheTimeout = setTimeout(cleanCache, 3 * 60 * 1000); | |
| // Cancel any existing scheduled cache cleanup | |
| if (cleanCacheTimeout !== null) { | |
| clearTimeout(cleanCacheTimeout); | |
| cleanCacheTimeout = null; | |
| } | |
| // Clean cache in 3 minutes | |
| cleanCacheTimeout = setTimeout(cleanCache, 3 * 60 * 1000); |
🛠️ Refactor suggestion
Consider using 'setInterval' for consistent cache cleanup
Using setInterval can simplify the cache cleanup mechanism by consistently invalidating the cache every three minutes, regardless of cache usage patterns.
Implement setInterval to schedule regular cache cleanups:
- // Clean cache in 3 minutes
- cleanCacheTimeout = setTimeout(cleanCache, 3 * 60 * 1000);
+ // Schedule cache cleanup every 3 minutes if not already scheduled
+ if (cleanCacheTimeout === null) {
+ cleanCacheTimeout = setInterval(cleanCache, 3 * 60 * 1000);
+ }Modify the cleanCache function to retain the interval:
function cleanCache() {
cachedResponse = null;
- // cancel scheduled cache cleanup
- if (cleanCacheTimeout !== null) {
- clearTimeout(cleanCacheTimeout);
- cleanCacheTimeout = null;
- }
logger.trace({ endpoint: 'getStatus' }, 'cleanup cache');
}This approach ensures the cache is invalidated regularly without the need to manage timeouts manually.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Rabbit misunderstood logic. We cleanup cache by block or in 3 mintues after it was generated by a user request. Interval doesn't work for our case.
There was a problem hiding this comment.
@shumkov, thank you for the clarification. I apologize for the misunderstanding. I now understand that the cache is cleaned up by block or in 3 minutes after it was generated by a user request, so using an interval doesn't fit your requirements.
（・・？）
✏️ Learnings added
Learnt from: shumkov
PR: dashpay/platform#2155
File: packages/dapi/lib/grpcServer/handlers/platform/getStatusHandlerFactory.js:231-233
Timestamp: 2024-09-26T14:33:28.944Z
Learning: In this codebase, the cache is cleaned up by block or in 3 minutes after it was generated by a user request. Using 'setInterval' does not work for this case.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/dapi/lib/externalApis/tenderdash/BlockchainListener.js (2)
54-64: LGTM: Improved event handling and logging.The changes enhance the visibility of transaction processing and simplify the
NEW_BLOCK_QUERYevent handling. These improvements align well with the PR objectives.Consider adding more detailed logging for the new block event:
this.wsClient.on(NEW_BLOCK_QUERY, (message) => { - this.processLogger.trace('Received new platform block'); + this.processLogger.trace('Received new platform block', { height: message.block.header.height }); this.emit(EVENTS.NEW_BLOCK, message) });This will provide more context about each new block received.
75-78: LGTM: Well-organized subscription logic.The new
#subscribe()method effectively encapsulates the subscription logic, improving code organization and maintainability. It correctly subscribes to both new blocks and transactions, which is consistent with the class's purpose.Consider adding error handling to make the subscription process more robust:
#subscribe() { - this.wsClient.subscribe(NEW_BLOCK_QUERY); - this.wsClient.subscribe(TX_QUERY); - this.processLogger.debug('Subscribed to platform blockchain events'); + try { + this.wsClient.subscribe(NEW_BLOCK_QUERY); + this.wsClient.subscribe(TX_QUERY); + this.processLogger.debug('Subscribed to platform blockchain events'); + } catch (error) { + this.processLogger.error('Failed to subscribe to blockchain events', { error: error.message }); + } }This will ensure that any subscription errors are properly logged and handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/dapi/lib/externalApis/tenderdash/BlockchainListener.js (2 hunks)
🔇 Additional comments not posted (3)
packages/dapi/lib/externalApis/tenderdash/BlockchainListener.js (3)
16-21: LGTM: Improved logging with dedicated process logger.The addition of a dedicated
processLoggerusinglogger.child()is a good practice. It allows for contextualized logging, which will enhance debugging and monitoring capabilities for theBlockchainListenerclass.
66-72: LGTM: Robust subscription handling implemented.The addition of the 'connect' event listener and the check for an existing connection ensure that the client always subscribes to events, both on new connections and reconnections. This directly addresses the PR objective of ensuring the websocket client resubscribes upon reconnection.
The use of the private
#subscribe()method improves code organization and maintainability.
Line range hint
1-78: Verify cache invalidation implementation.The changes effectively implement subscription to new blocks and resubscription on reconnection. However, the PR objectives mention clearing the cache every three minutes to account for scenarios where a drive or tenderdash may become unavailable. This cache invalidation logic is not present in the current file.
Could you please clarify where the cache invalidation every three minutes is implemented? If it's not yet implemented, consider adding it to ensure all PR objectives are met.
To help locate the cache invalidation implementation, you can run the following script:
This script will search for potential cache invalidation logic and timer setups that might be used for the 3-minute cache clearing mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/dapi/test/integration/externalApis/tenderdash/BlockchainListener.spec.js (1)
87-98: Improved test case for subscription eventsThe changes to this test case are well-implemented and align with the PR objectives. By setting
wsClientMock.isConnected = trueand explicitly callingblockchainListener.start(), the test now more accurately reflects the expected behavior of the BlockchainListener.The verification of both transaction and new block subscriptions is a good addition, ensuring that the listener is properly set up for all required events.
Consider adding a test case for when the WebSocket client is not connected to ensure the listener behaves correctly in that scenario as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/dapi/lib/externalApis/tenderdash/BlockchainListener.js (2 hunks)
- packages/dapi/test/integration/externalApis/tenderdash/BlockchainListener.spec.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dapi/lib/externalApis/tenderdash/BlockchainListener.js
🔇 Additional comments (3)
packages/dapi/test/integration/externalApis/tenderdash/BlockchainListener.spec.js (3)
101-103: Re-enabled and improved test for new block emissionGreat job on re-enabling this test case. The addition of
blockchainListener.start()at the beginning of the test ensures that the listener is properly initialized before testing the new block emission. This change improves the reliability of the test and aligns well with the PR objectives.
114-115: Improved test initialization for transaction emissionThe addition of
blockchainListener.start()at the beginning of this test case is a good improvement. It ensures that the BlockchainListener is properly initialized before testing the transaction emission, which increases the reliability of the test. This change maintains consistency with the other test cases in the file.
Line range hint
1-126: Overall assessment of changesThe modifications to this test file are well-implemented and align closely with the PR objectives. The changes improve the test coverage, particularly for the
startmethod of the BlockchainListener class. By ensuring that the WebSocket client is connected and explicitly callingblockchainListener.start()in each test, the reliability of the tests has been enhanced.The re-enabled test for new block emission and the improved checks for both transaction and new block subscriptions contribute to a more comprehensive test suite. These changes will help ensure the proper functioning of the cache invalidation logic and the handling of new blocks as described in the PR objectives.
Issue being fixed or feature implemented
The
getStatusresults is caches in DAPI memory and cache invalidation logic doesn't workWhat was done?
How Has This Been Tested?
Starting and stopping containers locally
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes