Page MenuHomePhabricator

[chronik-client] Support ping / pong in browser environment
ClosedPublic

Authored by bytesofman on Dec 29 2023, 21:55.

Details

Summary

ws.ping() is not available in the browser WebSocket object. However, ws.send is.

Implement a browser ping by subscribing to the first existing subscription if the ws.ping() method is unavailable.

Test Plan

In nodejs, install this version of chronik-client and connect to a websocket. Subscribe. Add console.log statements to confirm ws.ping() is being used.
Implement this version of chronik-client in cashtab and npm start. Ref D15050. Confirm the browser ping is working.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
chronik-client-ping-mods
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26081
Build 51735: Build Diffchronik-client-tests · chronik-client-integration-tests
Build 51734: arc lint + arc unit

Event Timeline

nodejs testing

image.png (241×555 px, 24 KB)

cashtab testing
... I don't have screenshots but I did run it with the ref diff and confirm no ws disconnects and the browser ping method was activated

bytesofman added a subscriber: tobias_ruck.

Note: would be nice to just ws.send('ping') -- however this would require the correct encoding, which would require adding this to the chronik.proto file. This is probably easy enough to do -- i'm checking with @tobias_ruck on the approach. I'm not really sure if there is any performance difference between 'ping' and subscribing to an already existing subscription. pinging is certainly more elegant.

Tail of the build log:

48 packages are looking for funding
  run `npm fund` for details

1 moderate severity vulnerability

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

> chronik-client@0.10.0 integration-tests
> mocha -j1 -r ts-node/register test/integration/*.ts --reporter mocha-junit-reporter --reporter-options mochaFile=test_results/chronik-client-integration-tests-junit.xml --reporter-options testsuitesTitle=Chronik Client Integration Tests --reporter-options rootSuiteTitle=Chronik Client

Starting test_runner
Test runner started
2023-12-29T21:59:42.547000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/chronik-client-integration-tests/test/tmp/test_runner_₿₵_🏃_20231229_215942/setup_scripts/chronik-client_blockchain_info_0
Setting chronik url to  http://127.0.0.1:30001
2023-12-29T21:59:44.340000Z TestFramework (INFO): Step 1: Mine 10 more blocks
2023-12-29T21:59:44.353000Z TestFramework (INFO): Received a stop message, exiting
2023-12-29T21:59:44.404000Z TestFramework (INFO): Stopping nodes
2023-12-29T21:59:45.412000Z TestFramework (INFO): Cleaning up /work/abc-ci-builds/chronik-client-integration-tests/test/tmp/test_runner_₿₵_🏃_20231229_215942/setup_scripts/chronik-client_blockchain_info_0 on exit
2023-12-29T21:59:45.412000Z TestFramework (INFO): Tests successful
Running Unit Tests for Test Framework Modules
setup_scripts/chronik-client_blockchain_info.py started
setup_scripts/chronik-client_blockchain_info.py passed, Duration: 3 s

TEST                                            | STATUS    | DURATION

setup_scripts/chronik-client_blockchain_info.py | ✓ Passed  | 3 s

ALL                                             | ✓ Passed  | 3 s (accumulated) 
Runtime: 3 s

-----------------------|---------|----------|---------|---------|-----------------------------------
File                   | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                 
-----------------------|---------|----------|---------|---------|-----------------------------------
All files              |    5.36 |     0.63 |    1.47 |    5.34 |                                   
 chronik-client        |     100 |      100 |     100 |     100 |                                   
  index.ts             |     100 |      100 |     100 |     100 |                                   
 chronik-client/proto  |     4.9 |     0.67 |    1.72 |    4.89 |                                   
  chronik.ts           |    5.44 |     0.84 |    1.81 |    5.42 | ...,3978-3985,3990-4027,4031-4036 
  chronikNode.ts       |    4.15 |     0.45 |    1.59 |    4.17 | ...,2682-2729,2740-2804,2808-2813 
 chronik-client/src    |    8.51 |        0 |       0 |    8.46 |                                   
  ChronikClient.ts     |    4.19 |        0 |       0 |    4.24 | 29-159,174-218,296-699            
  ChronikClientNode.ts |      40 |      100 |       0 |      40 | 23-42                             
  failoverProxy.ts     |    3.92 |        0 |       0 |    4.04 | 27-313                            
  hex.ts               |   31.57 |        0 |       0 |   33.33 | 30-34,38-42,46-59,63-65           
-----------------------|---------|----------|---------|---------|-----------------------------------

##teamcity[blockOpened name='Code Coverage Summary']
##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='156']
##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='2909']
##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='21']
##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='3286']
##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='8']
##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='541']
##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='154']
##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='2881']
##teamcity[blockClosed name='Code Coverage Summary']
Build chronik-client-integration-tests failed with exit code 1

update: the best way to do this is have chronik send pings on the server side. this is in Tobias queue but not sure when it will get in.

In the meantime, I think it's worth publishing this as

  1. Improves on what we have now
  2. Is required to implement latest chronik-client into Cashtab
Fabien requested changes to this revision.Jan 2 2024, 20:01
Fabien added a subscriber: Fabien.

Why is the subscribe better than a dummy send ? Does the server disconnect on unknown frames ? If yes this is a bug and should be fixed on the server.

modules/chronik-client/src/failoverProxy.ts
231 ↗(On Diff #43813)

you doubled the brackets

This revision now requires changes to proceed.Jan 2 2024, 20:01

Why is the subscribe better than a dummy send ? Does the server disconnect on unknown frames ? If yes this is a bug and should be fixed on the server.

A dummy send requires a dummy type in the chronik.proto file, then running the proto script here to support it. So, it would be best in this case to add the actual ping type with the dummy script.

For example, wsEndpoint.ws.send('ping') throws a wiretype error. I managed to find the hex that is usually sent for ping (couldn't immediately find it just now), but this also throws a wiretype error. I think there's not really a standard for this as some servers do different things, for example the blockchain.com websocket uses {op: 'ping'}. So, there probably is some struct I can ws.send that will be recognized as ping by the server, but the right way to get it is to run the proto script after adding the correct type to the chronik proto file. For now, subscribe is the only message I can send with existing and supported types.

But it's imo not worth adding the ping type to the chonik proto and running the script again, since the ultimate plan here is to have chronik itself send the pings, which is the proper way to do it. The only reason I think it's still worth landing this approach is because implementing the updated chronik (with auto backups) into Cashtab is a bit of a refactor, which I would like to do now, since it opens up work on other websocket improvements in Cashtab.

The change from client pings to server pings is not expected to require significant Cashtab refactor. Since we have already published chronik-client with client pings, and it is currently just not working for the browser, imo worth getting it fixed.

This revision is now accepted and ready to land.Jan 3 2024, 08:59