New client with same daemon creates new IRC session unnecessarily

[AdamISZ]

If joinmarketd is run persistently with new clients connecting, new instances of JMDaemonServerProtocol are created, meaning that the member variable self.irc_config is reset to None, meaning a new IRC session + nick is created even though the IRC config is the same.

The parts of the class instance which manage the connection should therefore be class variables, but this requires a bit of work. At least the messagechannelcollection object must be thus, and also it seems the orderbookwatch related data. So this is a bit more of a rework than I'd thought it would be. Also there might be something simpler than moving to class variables, perhaps moving these elements to the JMDaemonServerProtocolFactory, not sure.

This isn't labeled as a bug because it does not break functionality, but any long running daemon used repeatedly by clients will create a bunch of useless IRC connections, so it nearly counts. Should be fixed soon.

[AdamISZ]

Note that this does not occur in all-in-one mode (no_daemon=1 in config), which is the default for both scripts and Joinmarket-Qt; noticed it for electrum, where that option is not currently used.

[undeath]

Isn't the current behaviour correct? Assume a single jmdaemon is used by multiple clients, they all will require different nicks and thus different connections.

[AdamISZ]

@undeath's response to this very old issue was correct in as far as it goes. The problem was slightly wrongly characterised by me here.

On the one hand, starting a new protocol instance, with a new nick, clearly is correct. The problem I was observing here was just a result of the fact that there was not a 'shut down message channels' event on the completion of a task while the process is still running. This obviously didn't matter for one-and-done scripts where jmclient and jmdaemon were running in the same process, but did for a distinct joinmarketd.py against which you ran multiple different tasks via command messages sent remotely.

I believe the problem was partially fixed, but only in new usage contexts, by the inclusion of request_mc_shutdown. See these two use cases:

joinmarket-clientserver/jmclient/jmclient/yieldgenerator.py

Line 337 in bbd5019

self.clientfactory.proto_client.request_mc_shutdown()

and

joinmarket-clientserver/jmclient/jmclient/wallet_rpc.py

Line 513 in bbd5019

self.clientfactory.proto_client.request_mc_shutdown()

but I believe the problem would still exist for a user running a remote joinmarketd.py and starting yieldgenerator multiple times with scripts; the old connections don't shut down (TODO: let's please check that that's actually true).

Assuming this analysis is still correct, it shows up something about refactoring the codebase pretty clearly:

• Encapsulate long running processes as services so that we can handle shutdown in a uniform way. This concretely means that all instantiations of the yield generator should be done using jmclient.yield_generator.YieldGeneratorService instead of isolated so we always do this the same way.
• Moving more of the functions into the RPC framework so again, we do everything the same way and don't duplicate work. So our CLI scripts could literally call the same functions as remote clients do in the now clearly defined API.