chore(dashmate): report port check errors #2245
Conversation
WalkthroughThe changes in this pull request focus on enhancing error handling in various components related to port status checks within the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Learnings (1)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/dashmate/src/status/scopes/platform.js (1)
97-97: LGTM: Consistent error handling in result processingThe updated destructuring of
portStatusResultcorrectly handles both successful and failed port checks. This change ensures that the code continues execution even if a port check fails, which is consistent with the error handling improvements made earlier.For improved clarity, consider using a more explicit null check:
const [httpPortState, p2pPortState] = portStatusResult.map((result) => (result.status === 'fulfilled' ? result.value : PortStateEnum.ERROR));This makes it clear that failed checks result in
PortStateEnum.ERRORrather thannull.packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js (1)
Line range hint
170-192: Overall improvement in error handling for port status checksThe changes consistently improve error handling across all port status checks (Core P2P, Gateway HTTP, and Tenderdash P2P). This enhancement aligns well with the PR objectives and previous learnings about error handling in the
checkPortStatusfunction.To further improve code readability and maintainability, consider extracting the common error handling logic into a separate function. This would reduce code duplication and make future updates easier. For example:
const checkPortStatusWithErrorHandling = async (port, externalIp) => { try { return await providers.mnowatch.checkPortStatus(port, externalIp); } catch (e) { return e.toString(); } };Then, you could use this function in all three port check tasks:
const response = await checkPortStatusWithErrorHandling(port, config.get('externalIp'));This refactoring would make the code more DRY (Don't Repeat Yourself) and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js (3 hunks)
- packages/dashmate/src/status/providers.js (4 hunks)
- packages/dashmate/src/status/scopes/core.js (2 hunks)
- packages/dashmate/src/status/scopes/platform.js (2 hunks)
- packages/dashmate/test/unit/status/scopes/core.spec.js (3 hunks)
🧰 Additional context used
📓 Learnings (4)
📓 Common learnings
Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-10-09T00:22:57.778Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-09-26T14:28:54.892Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.packages/dashmate/src/status/providers.js (2)
Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-10-09T00:22:57.778Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-09-26T14:28:54.892Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.packages/dashmate/src/status/scopes/core.js (2)
Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-10-09T00:22:57.778Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-09-26T14:28:54.892Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.packages/dashmate/src/status/scopes/platform.js (2)
Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-10-09T00:22:57.778Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-09-26T14:28:54.892Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.
🔇 Additional comments (14)
packages/dashmate/src/status/scopes/core.js (3)
2-2: LGTM: New import statement is correctly added.The import of
PortStateEnumis properly placed and will be used for error handling in thecheckPortStatuscall.
120-121: Great improvement in error handling forcheckPortStatus.This change aligns perfectly with the previous learnings about how
mnowatch.checkPortStatusshould handle failures. By catching potential errors and resolving withPortStateEnum.ERROR, we ensure that the Promise chain doesn't break and that errors are properly represented in the port state.
Line range hint
1-150: Overall, excellent improvements to error handling.The changes in this file enhance the robustness of the
getCoreScopefunction by properly handling potential errors in thecheckPortStatuscall. These modifications align well with previous learnings and best practices. The code remains clean and maintainable, with no apparent negative impacts on existing functionality.packages/dashmate/src/status/scopes/platform.js (3)
2-2: LGTM: New import for PortStateEnumThe new import for
PortStateEnumis correctly placed and necessary for the changes in thegetTenderdashInfofunction.
93-96: LGTM: Improved error handling for port status checksThe changes enhance the robustness of port status checks by:
- Using
Promise.allSettledto attempt both checks regardless of individual failures.- Catching errors and returning
PortStateEnum.ERRORinstead of rejecting the promise.This implementation aligns with the previous feedback from shumkov in PR #2162, where it was suggested that failures should resolve with
PortStateEnum.ERRORinstead of rejecting the promise.
Line range hint
1-324: Summary: Improved error handling in port status checksThe changes in this file significantly enhance the robustness of port status checks in the
getTenderdashInfofunction. Key improvements include:
- Proper error handling using
PortStateEnum.ERROR.- Use of
Promise.allSettledto ensure all checks are attempted.- Consistent handling of failed checks in the result processing.
These changes align well with previous feedback and improve the overall reliability of the platform status reporting. The code is now better equipped to handle potential failures in port status checks without disrupting the entire process.
packages/dashmate/test/unit/status/scopes/core.spec.js (4)
81-81: LGTM: Improved asynchronous behavior simulationThe change from a synchronous return to
mockMNOWatchProvider.resolves('OPEN');is a good improvement. It better simulates the asynchronous nature of network operations, which is more realistic for port status checks. This change aligns well with the PR's objective of enhancing error handling in port checks.
222-222: LGTM: Consistent asynchronous behaviorThis change is consistent with the previous modification, ensuring that all test cases are updated to work with the new asynchronous implementation of
mockMNOWatchProvider. This consistency is crucial for maintaining the integrity of the test suite.
274-274: LGTM: Improved error handling simulationThe change to
mockMNOWatchProvider.rejects(PortStateEnum.ERROR);is an excellent improvement. It properly simulates an asynchronous error scenario in port status checks, which is crucial for testing error handling. This change aligns well with the PR's objective and the best practices for handling port status check failures.
Line range hint
81-274: Verify corresponding implementation changesThe changes in this test file consistently improve the simulation of asynchronous behavior in
mockMNOWatchProvider, which aligns well with the PR's objective of enhancing error handling in port checks. These modifications will lead to more realistic and robust tests.To ensure full compatibility:
Please run the following script to verify that the corresponding implementation in the
dashmatepackage has been updated to handle these asynchronous operations correctly:This script will help ensure that the implementation matches the new test expectations for asynchronous port status checks.
✅ Verification successful
Asynchronous Handling Verified Successfully
The
dashmatepackage correctly implements asynchronous handling forcheckPortStatususingasync/await, aligning with the test enhancements incore.spec.js. No further action is required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the dashmate package handles asynchronous port checks # Test: Search for asynchronous handling of port checks in the dashmate package rg --type javascript -g '!test' 'checkPortStatus.*\.(then|catch)' packages/dashmate/srcLength of output: 124
Script:
#!/bin/bash # Description: Verify that the dashmate package handles asynchronous port checks # Test: Search for asynchronous handling of port checks in the dashmate package rg --type js -g '!test' 'checkPortStatus.*\.(then|catch)' packages/dashmate/srcLength of output: 81
Script:
#!/bin/bash # Description: Search for all instances of `checkPortStatus` in the dashmate package # Search for any usage of `checkPortStatus` regardless of async handling rg 'checkPortStatus' packages/dashmate/src # Additionally, search for async/await usage related to `checkPortStatus` rg -A2 'async' packages/dashmate/src | rg 'checkPortStatus'Length of output: 1846
packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js (3)
170-171: Improved error handling for Core P2P port status checkThis change enhances the error handling for the Core P2P port status check. By adding a
.catch()block and converting any errors to strings, we ensure that all errors are captured and stored in a consistent format. This aligns with the PR objective of improving error reporting in the dashmate doctor report.
181-182: Consistent error handling improvement for Gateway HTTP port status checkThis change applies the same error handling improvement to the Gateway HTTP port status check as was done for the Core P2P port. It ensures consistent error reporting across different port checks, which enhances the overall reliability of the dashmate doctor report.
191-192: Consistent error handling across all port status checksThis change completes the implementation of improved error handling for all port status checks (Core P2P, Gateway HTTP, and Tenderdash P2P). The consistent approach ensures that all errors are captured and stored as strings, providing a uniform and more detailed error reporting mechanism in the dashmate doctor report.
packages/dashmate/src/status/providers.js (1)
Line range hint
88-135: Change in error handling may introduce unhandled promise rejectionsThe
checkPortStatusfunction now rejects the promise with anErrorobject when an error occurs (lines 100, 112, 135), whereas previously it resolved withPortStateEnum.ERROR. According to previous learnings, failures should resolve withPortStateEnum.ERRORinstead of rejecting the promise. This change could lead to unhandled promise rejections in code that expects a resolved value, potentially causing unexpected behavior.Consider revising the error handling to resolve with
PortStateEnum.ERRORas per previous conventions, or ensure that all calling code properly handles rejected promises.Run the following script to verify how the
checkPortStatusfunction is used throughout the codebase and ensure that promise rejections are handled appropriately:#!/bin/bash # Description: Find all usages of `checkPortStatus` and check if promise rejections are handled. # Search for all JavaScript files containing `checkPortStatus` calls. rg --type js -C 5 'checkPortStatus\('This script will identify all instances where
checkPortStatusis called. Please review these occurrences to confirm that promise rejections are properly handled to prevent potential issues.
shumkov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/dashmate/src/status/providers.js (1)
Line range hint
86-142: Overall improvements in error handling, with some concerns.The changes to the
checkPortStatusfunction generally improve error reporting and handling, aligning with the PR objectives. However, there are a few points to consider:
- The removal of the default value for the
ipparameter might affect backward compatibility.- The change to reject promises instead of resolving with
PortStateEnum.ERRORcontradicts previous learnings. This inconsistency needs to be addressed or explained.- The timeout handling is a good addition to prevent hanging requests.
Please review these points and ensure that the changes are consistent with the overall error handling strategy in the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/dashmate/src/status/providers.js (4 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-10-09T00:22:57.778Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-09-26T14:28:54.892Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.packages/dashmate/src/status/providers.js (2)
Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-10-09T00:22:57.778Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.Learnt from: shumkov PR: dashpay/platform#2162 File: packages/dashmate/src/status/providers.js:121-127 Timestamp: 2024-09-26T14:28:54.892Z Learning: In the `mnowatch.checkPortStatus` function, failures should resolve with `PortStateEnum.ERROR` instead of rejecting the promise.
🔇 Additional comments (3)
packages/dashmate/src/status/providers.js (3)
97-101: Improved error handling for non-200 status codes.The new implementation rejects the promise with a detailed error message for non-200 status codes, which aligns with the PR objectives of providing more informative error reporting.
118-120: Enhanced response size limit handling.The code now rejects the promise with a specific error message when the response size exceeds the limit, providing better clarity on the issue.
86-86: Consider backward compatibility implications of function signature change.The
ipparameter no longer has a default value. This change might break existing code that relies on the optional nature of this parameter.To assess the impact, let's check for usages of this function:
✅ Verification successful
Function signature change does not affect existing usages.
All current calls to
checkPortStatusprovide bothportandipparameters, so removing the default value foripdoes not break existing code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of checkPortStatus function rg --type js 'checkPortStatus\s*\(' packages/Length of output: 1065
Issue being fixed or feature implemented
There are might be issues during port check that are stored as ERROR to dashmate doctor report. It makes it difficult to analyze. Timouts are not handled properly
What was done?
How Has This Been Tested?
Running locally
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests