Websocket connection never cleaned up on connect() causing unrecoverable loop

[nickewansmith]

It seems that if Connection _subscribeToLedger throws on connect() that Connection _ws is never cleaned up due to the cleanup code in connect() ->_ws.once('close') never actually being reached due to returning immediately on throw.

This can be reproduced in this way:

const RippleAPI = require('ripple-lib').RippleAPI;

const api = new RippleAPI({
  server: 'wss://s.altnet.rippletest.net:51233' 
});

const run = async () => {
  await api.connect();
  console.log('connected');
  await api.disconnect();
  console.log('disconnected');
  api.connection._subscribeToLedger = () => {
    throw new Error('Ripple Not Initialized');
  };
  setInterval(async () => {
    console.log('run interval connect');
    await api.connect().then(() => {
      console.log('interval connect success');
    }).catch((e) => {
      console.log(e, 'interval connect failure, stuck in loop, never connects');
      api.connection._subscribeToLedger = () => {};
    })
  }, 2500);
}

run()
  .catch((e) => {
    console.error('Caught', e);
  });

[nickewansmith]

Here is a proposed solution #1186

[FredKSchott]

hey @nickewansmith, thanks for filing this! This logic is definitely tricky, so I want to make sure I understand your issue & the reproduction example and give some context:

• The on('close') logic was written to clean up a live Connection, that may have requests in flight.
• The _subscribeToLedger() step was written to happen before connect() resolved, meaning there would never be live/in-flight connections needing to be cleaned up at this point.
• Your example above is behaving as expected:
  • connect() should only resolve once _subscribeToLedger() resolves.
  • If _subscribeToLedger() rejects, connect() should also reject.
  • If _subscribeToLedger() hangs, thats a bug since it should never hang past a timeout :)
• When _subscribeToLedger() rejects (like in your example above) it's error should be caught in that try/catch, where we then call disconnect()which handles the _ws cleanup via this._ws.close(): https://github.com/ripple/ripple-lib/blob/develop/src/common/connection.ts#L538

Does that make sense? If I'm missing something / you are still seeing unexpected behavior, let me know.

[nickewansmith]

Hey, np @FredKSchott. What you've explained is correct, however, because the try/catch in question is run before the _ws.on('close') event gets a listener, calling _ws.close() in the disconnect() run in the catch never runs a cleanup listener for the close event, never cleaning up the connection and causing errors on each subsequent attempt to connect Websocket connection never cleaned up.

[FredKSchott]

Ah, you're right! Thanks for filling in that missing bit.

Okay, let's discuss solutions in #1186 since I think that PR is on the right track.