In _websocketUrlConnects, timeoutFailure did not close the WS, added a closure.
Details
- Reviewers
Fabien bytesofman emack - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Commits
- rABCd27e5156c299: [chronik-client] Add handling for WebSocket closure
I wrote a test script by overwriting the WebSocket's close method to count the number of times it was called. Then directly tested _websocketUrlConnects, simulating normal and timeout scenarios. Before modification, in non-timeout mode (5000ms), close should be called once (due to testWs.onopen), in timeout mode (1ms), close was called 0 times, indicating that nodes couldn't be properly closed during timeout. After modification, in non-timeout mode (5000ms), close is called once (same as before modification), in timeout mode (1ms), close is called twice because testWs.close(); immediately triggers testWs.onerror, which demonstrates that after the modification, WebSocket closure is guaranteed even after timeout.
```ts // alitatest2.ts import { FailoverProxy } from './src/failoverProxy'; import WebSocket from 'isomorphic-ws'; async function simpleWsCloseTest() { console.log("Starting WebSocket connection and close test..."); // Record the number of WebSocket closes let closeCount = 0; // Save the original close method const originalClose = WebSocket.prototype.close; // Replace close method for monitoring WebSocket.prototype.close = function(code?: number, data?: string | Buffer) { closeCount++; console.log(`WebSocket close call #${closeCount}`); return originalClose.call(this, code, data); }; try { // Create proxy const proxy = new FailoverProxy('https://chronik.e.cash'); // Directly call private method (for test purposes only) // @ts-ignore - Ignore TypeScript warning about accessing private property const result = await proxy._websocketUrlConnects('wss://chronik.e.cash/ws'); console.log(`Connection result: ${result ? 'success' : 'failure'}`); console.log(`Total WebSocket.close() calls: ${closeCount}`); } catch (error) { console.error('Test error:', error); } finally { WebSocket.prototype.close = originalClose; console.log("Test completed, closing ws"); } } simpleWsCloseTest();
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- alita0418
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Event Timeline
modules/chronik-client/src/failoverProxy.ts | ||
---|---|---|
290 ↗ | (On Diff #53552) | Is the risk here that every single chronik url in the array errors out and it ends up continuously looping through the array over and over? In which case, isn't that an extremely rare occurrence? Adding a static 100ms delay to all reconnection attempts doesn't seem to be the right ROI as a mitigation. |
292 ↗ | (On Diff #53552) | This is also a separate change so should be in a separate diff to ring fence any unintended consequences of this logic. |
- As @emack mentioned, this is 2 diffs. Please make one diff to add the close, and another to add the delay
- npm test does not actually test the behavior described in this diff. If it is impractical to add a unit or integration test, the test plan should include a description of how to repeat the issue before this change, and how to demonstrate that the issue is resolved after this change. Then npm test is demonstrating that existing behavior is preserved with this patch.
- patch version bump npm version patch (can be for just the 2nd of these 2 diffs, or can be a separate npm version patch for each diff), update changelog section of README describing the change
modules/chronik-client/src/failoverProxy.ts | ||
---|---|---|
290 ↗ | (On Diff #53552) | Yes, it needs a better design. it's quite rare - recently it occurred once and caused a url to remain in an unexpected "DDoS" state. for example, in an array with only one url, if that node happens to enter a synchronized state or other failures, it starts an loop, |
modules/chronik-client/src/failoverProxy.ts | ||
---|---|---|
290 ↗ | (On Diff #53552) |
@bytesofman - would it make sense to mandate a minimum of 2-3 urls upon initialization and if less than that it throws an initialization error? |
modules/chronik-client/src/failoverProxy.ts | ||
---|---|---|
290 ↗ | (On Diff #53552) | Short answer: no. Long answer: see the other diff. |
This diff will only address the issue of _websocketUrlConnects not closing the WebSocket under timeout conditions.
The reconnection mechanism issue in connectWs mentioned earlier will be submitted in the next diff. The changes from this modification will be included in the readme and version update of the next diff.