Investigate Bitbox Socket Issue

[christroutner]

From a user report in our Telegram channel:

I'm using bitbox.Socket to listen for address activity and noticed that each call to socket.listen creates an additional TCP connection for its query and callback but socket.close only closes the first one that was created.

The scope of this issue is to investigate this claim. The deliverable is code that reproduce this issue of creating multiple TCP connections.

I've seen behavior on the back end that suggest that some apps are creating redundant connection which slows down both the server and end-user app.

[rnbrady]

I've added steps to reproduce in branch issue-173 on my fork here.

Running node test/e2e/count-connections-issue-173/countConnectionsSocketIO.js results in:

Outbound connections from this node process before calling listen:

Outbound connections from this node process after calling listen twice:
node      67150   rb   29u  IPv4 0x39cac9b8a208741      0t0  TCP 10.129.108.118:54962->13.53.62.102:443 (ESTABLISHED)
node      67150   rb   31u  IPv4 0x39cac9b8a2b2db9      0t0  TCP 10.129.108.118:54963->13.53.62.102:443 (ESTABLISHED)

Outbound connections from this node process after calling close (zombie connections):
node      67150   rb   29u  IPv4 0x39cac9b8a208741      0t0  TCP 10.129.108.118:54962->13.53.62.102:443 (ESTABLISHED)

This shows that a developer wanting to listen() for both "transactions" and "blocks" will inadvertently open two connections, once of which will remain after calling close().

Running node test/e2e/count-connections-issue-173/countConnectionsBitSocket.js gives the same result for the BitSocket case with multiple queries submitted to same instance.

There are different ways to approach fixing this, with different amounts of overhaul to lib/Socket.ts and the API.

A minimal change would just track connections, re-using them where possible and throwing errors where not.

But it might also make sense to refactor a bit like moving connection establishment to the constructor, which is what the documentation implies happens. This would allow the constructor() and close() callbacks to be wired up in a meaningful way as they are currently short-circuited (i.e. called synchronously and immediately). It would also bring forward the decision between socket.io vs bitsocket. I would also move require('eventsource') to top-level and convert to import.

If you could reply with "minimal change" or "refactoring ok" to indicate your preference I will submit a suitable PR for your consideration.

[rnbrady]

Just to add that the test as committed only calls socket.close() once but calling it twice still leaves the zombie connections.

Also on further investigation moving the connection establishment to constructor is not possible for the BitSocket case (as it depends on knowing the query) and not a good idea for the socket.io case (TIL "constructors should be pure") so I'd like cancel that suggestion.

I'm tending towards the "minimal change" option.

Cc @SpendBCH.

[rnbrady]

Just discovered there is a separate issue in EventSource dependency also causing zombie connections. Please note the PR submitted here does not address that issue.