JSON-RPC interface to control client and server

[dtinth]

Short description of changes

Allow Jamulus client and server to be programmatically controlled through a JSON-RPC-based protocol.

Context: Fixes an issue?

This PR addresses the following items:

• Headless Server: Reload Server Welcome Message #1958
  • jamulusserver/setWelcomeMessage to dynamically change welcome message.
• Remote Server Administration Requirements #1863
  • jamulusserver/getConnectedClients to get clients connected w/ IP info.
• Expanded status/state of server via file or message #1847
  • jamulusserver/getServerInfo to get server status

Does this change need documentation? What needs to be documented and how?

• --jsonrpcport <port>
• The JSON-RPC protocol.

Status of this Pull Request

I did this PR mainly to solve my own issues, so I see this more like an RFC with a working implementation. So PR status is more like a proof of concept, although I am already using it in my own systems. Also I am C++ noob, so there may be major issues with the code, esp with memory management.

Update: All PR comments have been address and all checks have passed, I believe the PR is in a mergeable state.

What is missing until this pull request can be merged?

• Consensus on whether or not, and how, to proceed forward? Assumed through lazy consensus.
• Updating this PR on top of master branch (right now it is on top of r3_8_0).
• Some exploratory testing to ensure that Jamulus does not crash.
• Test running server with constant API usage to ensure no memory leak
• API documentation for jamulus/ and jamulusserver/
• Check for other fixes/extensions on https://github.com/dtinth/jamulus/commits/dtinth
• Final test of all API calls
• Squash commits in a history friendly way

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

Background

I would like to programmatically control Jamulus server and client. My use cases include:

• Server
  • Change the welcome message without restarting server.
• Client
  • Changing the client’s name to reflect current number of Icecast listeners.
  • Creating chat bots, etc.

I have seen the proposal to add MQTT-based interface to Jamulus server. I prefer RPC-style messaging more and also would like to control the client programmatically. So I played around with Jamulus source code and came up with this.

High level design

• This PR adds a --jsonrpcport CLI flag.

  • When specified, it will run a TCP server, talking newline-delimited JSON-RPC protocol.
  • If not specified, then JSON-RPC server will not be started.
  • Functionality depends on whether a client or server is started.

• It introduces three main classes:

  • CRpcServer, a generic JSON-RPC server built on top of QTcpServer.
  • CClientRpc, an implementation of an RPC interface for the Jamulus client.
  • CServerRpc, an implementation of an RPC interface for the Jamulus server.

• Client methods (-->) and broadcast notifications (<--) implemented:

  • --> jamulusclient/getClientInfo {}
  • --> jamulusclient/getChannelInfo {}
  • --> jamulusclient/getClientList {}
  • --> jamulusclient/setName { name }
  • --> jamulusclient/setSkillLevel { skillLevel }
  • --> jamulusclient/sendChatText { message }
  • <-- jamulusclient/chatTextReceived { text }
  • <-- jamulusclient/connected { id }
  • <-- jamulusclient/clientListReceived { clients }
  • <-- jamulusclient/disconnected {}
  • <-- jamulusclient/channelLevelListReceived { channelLevelList }

• Server methods (-->) and broadcast notifications (<--) implemented:

  • --> jamulusserver/getServerInfo {}
  • --> jamulusserver/getConnectedClients {}
  • --> jamulusserver/setServerName { serverName }
  • --> jamulusserver/setWelcomeMessage { welcomeMessage }

How to use the JSON RPC server

On Unix systems, you can use netcat to talk to the server:

$ nc localhost 22123

Once connected, send a request on a single line, then the response will come back on a single line. In the example below, the first line is the request, and the second line is the response.

{"jsonrpc":"2.0","method":"jamulusserver/getServerInfo","params":{},"id":1}
{"id":1,"jsonrpc":"2.0","result":{"city":"","country":225,"name":"","registrationStatus":0,"welcomeMessage":""}}

Node.js, jayson can be used to talk to the server:

const jayson = require("jayson/promise");
const client = new jayson.client.tcp({ host: "127.0.0.1", port: 22100 });

client.request('jamulusserver/getServerInfo', {})
  .then(console.log)
  .catch(console.error)

[dtinth]

From the discussion, @ann0see suggested pinging:

• @cdmahoney, author of MQTT proposal Proposal to add inter process communication using mqtt protocol to Jamulus #1789
• @Rob-NY, author of discussions Remote Server Administration Requirements #1863 and Expanded status/state of server via file or message #1847.
• @softins on C++ and code reviews.

I would appreciate your feedback on this idea and how it’s implemented.

[pljones]

Looks nicely done.

In the longer term, if greater separation between clientui and client and between serverui and server can be achieved, with both having clearer interfaces, this concept can be extended easily enough to take advantage of it.

I'll have a good read through later.

[cdmahoney]

Looks good to me.

I think using MQTT may be more convenient on the server, with more natural support for notifications from jamulus to client code, but in the end the functionality would be very similar. On jamulus client RPC seems preferable.

[pljones]

OK, the main thing I'd like is for it to be possible for any method that can change state to be protected, so someone could expose those methods that retrieve state whilst not allowing control of a server.

[dtinth]

@pljones Thanks for the thorough review. I will address them in code later, but here are replies to your comments.

In particular, this one is about endpoint protection.

[ann0see]

I think using MQTT may be more convenient on the server, with more natural support for notifications from jamulus to client code, but in the end the functionality would be very similar. On jamulus client RPC seems preferable.

@cdmahoney So you'd suggest having both. Maybe you and @dtinth could agree on implementing one on the client and the other one server side? I'd leave the decision up to you but please keep in mind that duplicate work or even implementing two similar features is counter productive.

Edit: Could someone please also have a look at #1990 and compare & discuss the differences?

Edit: from the discussion Peter was neither happy with Mqtt and the file approach: #1847 (reply in thread)

Edit: To all contributors: Even if your suggestion is not merged/accepted, it will have had a huge impact on the one which did get included since your ideas probably got integrated. It's better to agree on one solution and improve this one than merging none or not improving the accepted one.

[pljones]

One of the best bits here is that it connects to client.cpp and server.cpp, exposing their existing behaviour in a clean manner without being invasive.

[dtinth]

@pljones Thanks, I have addressed your comments in the code. I will leave it to you to mark them as resolved.

[dtinth]

@ann0see I just looked at Rob-NY@b3dac8a. @Rob-NY’s implementation uses the same CL (connectionless) UDP-based protocol that is used in Jamulus, which is more lightweight, allows more code reuse, and should be lighter, in terms of load.

As for my implementation, I focused on ease of integration, so I opted to make the protocol strictly based on plaintext and JSON. This makes it more heavyweight (uses TCP and does more work to parse and serialize JSON) although I believe performance impact is negligible.

Note that the current implementation of JSON-RPC does not yet implement recording controls (start/stop/get status) as I currently do not use the recording feature in Jamulus. Should JSON-RPC proposal be accepted, I suggest implementing recording feature on top of this PR, in order to keep this PR small.

My implementation in this PR is mostly focused on automating the client, as I needed that for jamcaster (a tool that streams a Jamulus room to Icecast, YouTube or Facebook from a headless Linux server, see demo).

[ann0see]

Thanks for the explanation. Personally, I think although your approach might violate https://github.com/jamulussoftware/jamulus/blob/master/CONTRIBUTING.md#keep-it-simple-and-stupid-and-do-one-thing-and-do-it-well it is expandable which will make following additions easier. I'd vote for your approach.

We use lazy consensus by default. This means that any reversible decision is considered supported by the team as long as nobody objects. This also means that everyone must be transparent about their intentions and actions in order for every other member to have a chance to comment.

Source: https://jamulus.io/contribute/Administration

[dtinth]

Addressed all the comments above, thank you @pljones.

[dtinth]

My fork and this PR has diverged quite significantly now that my current supporting scripts are now incompatible (as a result, my servers are stuck at version 3.8.1 for the time being), so I’d also prefer option 1.

Once this is merged, I plan to rebase my fork on top of it (i.e. reapply all of my changes again but on top of latest master), so that I can upgrade all my servers to be based on the new JSON-RPC interface and provide additional testing. (Since the codebase has diverged, it is now no longer practical for me to test this PR on my fleet of servers; I’d rather wait for this to get merged first so I have a more stable base commit to rebase on top of.)

Therefore I’d prefer to be more aligned with trunk-based development where it’s ok to ship incomplete/unstable features behind feature flags. Given that the JSON-RPC code is “latent” (has no runtime effect) until explicitly enabled, and that this feature is clearly marked as experimental¹, IMO it is in a mergeable state.

I must admit I struggle to understand a reason for making skillLevel changeable via the RPC

My main use case for implement changeable skillLevel was to achieve blinking color for the streamer client² 😂

Footnotes

1. The concept of stability index may be useful. ↩

2. Some musicians used to disguise themselves as a streamer client, so changing color was implemented to make it harder to disguise. Nothing malicious is happening here by the way, it was just a fun response to imitators. ↩

[ann0see]

Approving on the basis that:

1. I have tested it a few days ago
2. As dtinth said it is not critical
3. We are planning (hopefully!!!) to extend and improve this
4. There was an excessive descussion and it is being tried on a real server.

[ann0see]

Therefore I’d prefer to be more aligned with trunk-based development

I think the website is being developed in a comparable method - but it created and creats a lot of confusion...

[softins]

My main use case for implement changeable skillLevel was to achieve blinking color for the streamer client (Some musicians used to disguise themselves as a streamer client, so changing color was implemented to make it harder to disguise. Nothing malicious is happening here by the way, it was just a fun response to imitators).

Well there you go! It never occurred to me that the reason for using it could be anything other than actual skill level! Very creative...

[softins]

I think this is close enough to merge now. Let's get it in and then address improvements incrementally.

[ann0see]

Can or can’t we squash merge this?

[pljones]

@hoffie or @dtinth

Could you please locally rebase -i to latest master, squashing down to one commit and then force push to the branch, please.

It can then be merged with a normal merge commit.

[hoffie]

Yes, will do, that was the plan anyway (I want it to make it three commits though).

[pljones]

Yes, will do, that was the plan anyway (I want it to make three commits though).

I was going to say if you want to have some nicer to read commits... but didn't want to suggest the extra work. :)

[hoffie]

I've rebased and polished the history. I've confirmed via git diff c133a9f3..bb1b6ee0 --stat that the rebasing/squashing did not miss anything.

PR can be merged on CI green.

[ann0see]

A nightly should be tagged (and maybe shortly announced in GH discussions) after this has been merged.

[hoffie]

@dtinth I just saw that you are not listed at https://github.com/jamulussoftware/jamulus/blob/master/src/util.cpp#L494

I neither wanted to add you with your full name without your consent nor did I want to delay the PR, but feel free to send a quick PR to add yourself.

Thanks again for all your hard work on this big feature/PR! :)

[dtinth]

@hoffie Finally~ 🎉 I will add it later tonight, thank you for mentioning that.

And thanks everyone involved for helping with pushing this PR to completion. I’ll test it on some of my servers soon. 🙏 🙏

[ann0see]

Removing the "needs documentation" label since documentation should be discussed in jamulussoftware/jamuluswebsite#717