fix: removing all listeners for pool and httpserver (#1668)

[rsercano]

can you please have a look @moshababo @sangaman @kilrau @raladev

[rsercano]

xud turns off fully only when gets EHOSTUNREACH error for connext client (connext client was killed before xud).

yes but that's only one of the cases, there're several I'm afraid

[rsercano]

After using wtfnode as suggested by you @sangaman I saw that those listeners were still there even after shutdown, and after this fix it wasn't there anymore.

[kilrau]

If it gets @sangaman's approval and @raladev can confirm it doesn't break anything it can be merged as is imo.

[sangaman]

After using wtfnode as suggested by you @sangaman I saw that those listeners were still there even after shutdown, and after this fix it wasn't there anymore.

@rsercano Which listeners? Does it say what event they were listening for? Does it help in any cases where xud isn't shutting down gracefully?

I don't expect this to break anything, but I also don't expect it to fix anything. I can't find any documentation that suggests this should be done with nodejs net servers. I think at a minimum though we should remove the removeAllListeners call into the close callback so that we're not removing listeners until after the server has closed, otherwise I could imagine some weird edge cases caused by like an incoming connection before the server has closed but after we've removed the listeners, I don't know what would happen then.

[rsercano]

Listeners according to the wtfnode:

You're right about moving them into close callback, tho. still I'm unsure wtfnode is helpful enough, it still shows peer sockets, intervals even tho we call clearInterval obv, and connext socket after shutdown.

Nonetheless after this fix I didn't see these listeners in the wtfnode, and it worths a shot, also I'll check peer side as far as I can.

[rsercano]

With a second thought, we never use http server while working on cmd, so I'm detaching removeAllListeners call from there, and keeping the one in the Pool by moving it to callback.

[sangaman]

Interestingly this change now breaks a bunch of our p2p network tests, I verified I'm getting the same locally.

  P2P Sanity Tests
    ✓ should connect successfully (56ms)
    ✓ should update the node state
    ✓ should fail connecting to the same node
    ✓ should disconnect successfully
    ✓ should fail when connecting to an unexpected node pub key
    ✓ should fail when connecting to an invalid node pub key
    ✓ should fail when connecting to self
    ✓ should fail connecting to a non-existing node
    ✓ should revoke connection retries when connecting to the same nodePubKey
    ✓ should fail when connecting to a node that has banned us
    1) "after all" hook for "should fail when connecting to a node that has banned us"

  P2P Networks Tests
    2) should fail to connect a node from mainnet to a node from simnet
    3) should fail to connect a node from mainnet to a node from testnet
    4) should fail to connect a node from mainnet to a node from regtest
    5) should fail to connect a node from testnet to a node from mainnet
(node:422797) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 SIGINT listeners added to [process]. Use emitter.setMaxListeners() to increase limit
(node:422797) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 SIGTERM listeners added to [process]. Use emitter.setMaxListeners() to increase limit
    6) should fail to connect a node from testnet to a node from simnet
    7) should fail to connect a node from testnet to a node from regtest
    8) should fail to connect a node from simnet to a node from mainnet
    9) should fail to connect a node from simnet to a node from testnet
    10) should fail to connect a node from simnet to a node from regtest
    11) should fail to connect a node from regtest to a node from mainnet
    12) should fail to connect a node from regtest to a node from testnet
    13) should fail to connect a node from regtest to a node from simnet
    14) should successfully connect a node from simnet to a node from simnet
    15) should successfully connect a node from testnet to a node from testnet
    16) should successfully connect a node from simnet to a node from simnet
    17) should successfully connect a node from regtest to a node from regtest

I'd like to dig into this a bit more to see what's going on. I'm guessing though that those listeners are added by internal NodeJS code and removing them breaks some stuff, since we're not adding any listeners ourselves.