Page MenuHomePhabricator

[chronik-client] Add handling for WebSocket closure
ClosedPublic

Authored by alitayin on Fri, Apr 18, 03:27.

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
Summary

In _websocketUrlConnects, timeoutFailure did not close the WS, added a closure.

Test Plan

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();

Event Timeline

Owners added a reviewer: Restricted Owners Package.Fri, Apr 18, 03:27
emack requested changes to this revision.Fri, Apr 18, 12:14
emack added inline comments.
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.
Instead perhaps check if it's the end of the _endpointArray array and if so, THEN add.

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.

This revision now requires changes to proceed.Fri, Apr 18, 12:14
  • 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

keep the first modification to close WS

alitayin retitled this revision from [chronik-client] Add handling for WebSocket closure and add delay for reconnection to [chronik-client] Add handling for WebSocket closure .
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)

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,

@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)

See D17955

modules/chronik-client/src/failoverProxy.ts
290 ↗(On Diff #53552)

Short answer: no. Long answer: see the other diff.

alitayin edited the test plan for this revision. (Show Details)

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.

This revision is now accepted and ready to land.Tue, Apr 22, 20:25