Server list persistence for directory servers by pljones · Pull Request #1799 · jamulussoftware/jamulus

pljones · 2021-05-31T18:55:05Z

Currently, when restarting a directory server, it clears its list of registered servers. This means clients consulting the directory get an empty server list returned (well, containing the directory server itself only).

In order to make maintenance of directory servers less annoying for client users, this feature adds persistence of the list of registered servers across restarts.

Persistence is optional. It's enabled by specifying an existing, readable filename as an additional entry in the server info (i.e. name;city;country;persistencefile). Providing an empty file the first time is mandatory.

I have chosen not to add to the --help display, as this is only for directory server lists. I needs adding to the appropriate Wiki page, however.

When specified, the file is read on start up and written on shutdown (if writeable). Each line written to the file contains an entry from the server list (excluding the directory server itself). These are read on start up, prepopulating the list.

Servers must still re-register regularly to maintain their entry in the list.

NOTE: a7bb996 is #1803, on which this change depends.

softins · 2021-05-31T21:50:41Z

Persistence is optional. It's enabled by specifying an existing, readable filename as an additional entry in the server info (i.e. name;city;country;persistencefile). Providing an empty file the first time is mandatory.

This seems a slightly arcane way of specifying the persistence file. Why not just add a command-line option such as --persistfile to specify the filename?

softins · 2021-05-31T21:52:30Z

And for that matter, what is wrong with having the server create an empty file if it is specified but does not already exist?

hoffie · 2021-05-31T21:57:19Z

Persistence is optional. It's enabled by specifying an existing, readable filename as an additional entry in the server info (i.e. name;city;country;persistencefile). Providing an empty file the first time is mandatory.

This seems a slightly arcane way of specifying the persistence file. Why not just add a command-line option such as --persistfile to specify the filename?

I agree, but if it is limited to directory servers, we should probably have that context in the flag name. --directoryserver-persistfile (not sure if we have any flags with dashes in the middle yet though, but it's getting hard to read otherwise).

passing · 2021-05-31T22:28:36Z

src/serverlist.cpp

+    sl.append ( QString::number ( this->iMaxNumClients ) );
+    sl.append ( QString::number ( this->bPermanentOnline ) );
+
+    return sl.join ( ";" );


a server can have a semicolon in its name or city (when it is started from the GUI) and would get sorted out when loading the file.
Newline characters could also be added (when using --nogui) which could cause issues as well (probably would make sense to filter these out in general)

Good points. We should make ; illegal in the GUI, in that case (and I can strip here), as it's illegal in --serverinfo. That was overlooked when adding the GUI. Same for newlines (which should never be allowed).

A thought, after reviewing #1803 and #1804: those PRs only prevent the server itself creating names containing \n and ;; they don't do anything to prevent older registering servers from sending them to the directory. The code below strips out \n but not ;.

Also, I think there is some code duplicated between those PRs and this one? (e.g. reIllegal)

Volker avoided the problem of strange characters in names stored in the ini file by base64 encoding the names and writing in XML format. Maybe there is some code there that could be used here, instead of having to disallow characters?

Not a bad idea. Convert the values to base64, then join with ;. It would save having to have the other two PRs, too.

Just to note -- the INI file does not use base64 encoding for server name and city:

826 // name 827 PutIniSetting ( IniXMLDocument, "server", "name", pServer->GetServerName() ); 828 829 // city 830 PutIniSetting ( IniXMLDocument, "server", "city", pServer->GetServerCity() );

Again, that's probably an oversight.

pljones · 2021-06-01T06:19:44Z

Persistence is optional. It's enabled by specifying an existing, readable filename as an additional entry in the server info (i.e. name;city;country;persistencefile). Providing an empty file the first time is mandatory.

This seems a slightly arcane way of specifying the persistence file. Why not just add a command-line option such as --persistfile to specify the filename?

Because it's a file containing server info. It also avoids adding unnecessary code bloat. Basically, it's extra info for the directory server about servers. So there's no point adding a new option just for that, really.

pljones · 2021-06-01T06:22:02Z

And for that matter, what is wrong with having the server create an empty file if it is specified but does not already exist?

Because I expect anyone using the option to know what they're doing. This way it can be validated. If the file isn't there, it's more likely something isn't working and it's safer not to start guessing - so it issues a warning.

I should really require it to be writeable on start up, too...

passing · 2021-06-01T11:55:52Z

I see a few disadvantages in extending --serverinfo to name;city;country;persistencefile

In case some other serverinfo field that a server can provide should be added later (maybe its Genre, or other metadata), you will have to add that to the end again, but then the 4th parameter needs to be left empty as it is only relevant for the special case of running an own directory server.
Another use case could be to run Jamulus as a Directory Server only. In that case, the sub-parameters 1-3 would need to be left empty.

also jamulus --help is separated into sections:

General options:
...
Server only:
...
Client only:
...

So IMO it makes sense have separate Directory server only options.

I'd even rather suggest to split --serverinfo into 3 independent parameters - keeping the original for backwards compatibility - to not have to provide 3 values in the specific format & order name;city;country

pljones · 2021-06-01T20:35:50Z

In case some other serverinfo field that a server can provide should be added later ...

Then that case can be dealt with if it ever arises.

Another use case could be to run Jamulus as a Directory Server only. In that case, the sub-parameters 1-3 would need to be left empty.

No, that's not true. The directory server needs its own server info values as it's always in its own list.

IMO it makes sense have separate Directory server only options.

Not part of this change.

Also, I disagree with the need to split things out. I expect anyone setting up a directory server to be more than capable of using the existing command line parameter.

passing · 2021-06-01T21:29:03Z

Another use case could be to run Jamulus as a Directory Server only. In that case, the sub-parameters 1-3 would need to be left empty.

No, that's not true. The directory server needs its own server info values as it's always in its own list.

@pljones, that's how it is right now. But logically it doesn't make sense that a directory server needs to be a server it its own list.

My main idea is to not make the way to a more modular architecture harder.
I am just trying to help here with suggesting alternatives / trying to discuss them. But I don't have the impression that this is wanted.

pljones · 2021-06-03T06:23:47Z

@pljones, that's how it is right now. But logically it doesn't make sense that a directory server needs to be a server it its own list.

Yes, it does. The server has to register with itself ("localhost" and default port), otherwise it's not a directory server. As it can also be a jam server, it should supply details. All three Any Genre directories are jam servers, too. (I only limit jamming to a single client on my genre servers as I've limited uplink bandwidth.)

pljones · 2021-06-03T19:40:12Z

This is now running on Rock, Jazz, Classical/Folk and Choral/Barbershop genre servers.

I have to admit, having messed up configuration the first time I set it up, having a separate command line option might make it easier.

Server only:
...
  -e, --directoryserver address of the directory server with which to register
                        (or 'localhost' to host a server list on this server)
      --directoryfile   filename in which to store directory server list
                        across restarts
...

I'm also thinking splitting the Directory server only options out might be good, too - but not in this change. KISS (hence #1803 and #1804).

In fact, I'll leave it as is for now - I'll add the new command line option separately and re-organise the list at that time.

ann0see · 2021-06-04T16:52:43Z

Yes, it does. The server has to register with itself ("localhost" and default port), otherwise it's not a directory server.

Could you please clarify why this is needed? Yes, currently a directory server is a normal Jamulus server which also allows "slave servers" to register with it. However, the Jam server functionality is not needed. From my tests with @passing's python library, It is perfectly ok and possible to have a directory which is not shown on the list. This could also work with your directories which are only limited to one user (which is honestly just an implementation workaround).

softins · 2021-06-04T17:40:14Z

I thought a while ago that it would be useful if a directory server could accept -u 0 to prevent it adding itself to the list of registered servers, and from accepting any audio connections. I just never got around to implementing it.

hoffie · 2021-06-04T20:29:43Z

I thought a while ago that it would be useful if a directory server could accept -u 0 to prevent it adding itself to the list of registered servers, and from accepting any audio connections. I just never got around to implementing it.

I agree and this was requested by users as well. We would still have to be careful because the first Directory server reply in the server list is handled specially in the client. @passing found a way around that though. Should probably create a new Issue for that topic if it is going to be implemented.

pljones · 2021-06-05T11:24:01Z

From my tests with @passing's python library, It is perfectly ok and possible to have a directory which is not shown on the list.

Those are experiments with something not currently part of Jamulus. This is a change to improve the existing Jamulus Directory Server behaviour. I see no relationship.

Should probably create a new Issue for that topic if it is going to be implemented.

It's not related to this work, so yes.

src/serverlist.cpp

ann0see · 2021-06-12T10:09:57Z

src/serverlist.cpp

                    // servers behind a NAT and dealing with external, public
                    // clients.
                    vecServerInfo[iIdx].HostAddr = ServerList[iIdx].LHostAddr;
+                    // ?? Shouldn't this send the ping, as below ??


Hrm, so this is about making the --serverpublicip Directory-side logic more consistent with the regular code path regarding NAT traversal, right?

I agree that a ping should be send to be as close to the normal path.

On the other hand, this feature is only used by people who already have the capability to set up proper port forwardings (otherwise they couldn't run a public directory server behind NAT). Therefore, I'd argue that it might not be necessary. As this hasn't been reported as a bug or missing feature either, I'd rather not do something right now. If anyone disagrees, please say so and I can open a PR.

src/serverlist.cpp

pljones · 2021-06-12T23:12:03Z

My directory servers now running this latest version (briefly having taken each down, reformatted the persistence file, then started up with the new version).

pljones · 2021-06-13T09:02:04Z

I have to admit, having messed up configuration the first time I set it up, having a separate command line option might make it easier.
...
In fact, I'll leave it as is for now - I'll add the new command line option separately and re-organise the list at that time.

See pljones#9

I've left the server list options in with the other server options for now:

--directoryserver applies to both, so it doesn't have an obvious home
there's only --directorylist and --listfilter other than that
--serverpublicip is supplied to a server that's registering, but so is --serverinfo - would they go under server or directory server?

pljones · 2021-06-13T11:25:14Z

pljones#9 now running all my directory servers.

I'm probably going to merge that into this shortly.

passing · 2021-06-13T13:45:38Z

I've left the server list options in with the other server options for now:

* `--directoryserver` applies to both, so it doesn't have an obvious home
* there's only `--directorylist` and `--listfilter` other than that
* `--serverpublicip` is supplied to a server that's registering, but so is `--serverinfo` - would they go under server or directory server?

It would make sense to me to keep --directoryserver, --serverpublicip and --serverinfo under "Server only". So it would be just 2 "Directory Server only" options at the moment, which would make the server options list a bit shorter at least. But I also don't see a problem in keeping it as it is.

pljones force-pushed the feature/persistent-server-lists branch 2 times, most recently from f865765 to 782aae0 Compare May 31, 2021 19:49

pljones self-assigned this May 31, 2021

pljones force-pushed the feature/persistent-server-lists branch from 782aae0 to 258bb45 Compare May 31, 2021 21:36

passing reviewed May 31, 2021

View reviewed changes

pljones force-pushed the feature/persistent-server-lists branch from 258bb45 to 532d2ab Compare June 1, 2021 07:07

pljones force-pushed the feature/persistent-server-lists branch 2 times, most recently from d51dbc4 to 239ee9b Compare June 3, 2021 18:00

pljones mentioned this pull request Jun 3, 2021

New lines are illegal in --serverinfo values #1803

Closed

pljones force-pushed the feature/persistent-server-lists branch 2 times, most recently from 2269f30 to 6075685 Compare June 7, 2021 07:38

ann0see reviewed Jun 12, 2021

View reviewed changes

pljones force-pushed the feature/persistent-server-lists branch 4 times, most recently from b19ee0e to 2feed29 Compare June 12, 2021 23:10

Server list persistence for directory servers

68460ba

pljones force-pushed the feature/persistent-server-lists branch from 2feed29 to 68460ba Compare June 13, 2021 07:41

pljones closed this Jun 13, 2021

pljones deleted the feature/persistent-server-lists branch June 13, 2021 13:23

pljones mentioned this pull request Jun 13, 2021

Persistent server lists using --directoryfile #1867

Merged

4 tasks

Comments

Conversation

pljones commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softins commented May 31, 2021

Uh oh!

softins commented May 31, 2021

Uh oh!

hoffie commented May 31, 2021

Uh oh!

passing May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pljones Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

softins Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

pljones Jun 12, 2021

Choose a reason for hiding this comment

Uh oh!

pljones Jun 12, 2021

Choose a reason for hiding this comment

Uh oh!

pljones commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

passing commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones commented Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

passing commented Jun 1, 2021

Uh oh!

pljones commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pljones commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ann0see commented Jun 4, 2021

Uh oh!

softins commented Jun 4, 2021

Uh oh!

hoffie commented Jun 4, 2021

Uh oh!

pljones commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ann0see Jun 12, 2021

Choose a reason for hiding this comment

Uh oh!

hoffie Jun 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pljones commented Jun 12, 2021

Uh oh!

pljones commented Jun 13, 2021

Uh oh!

pljones commented Jun 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

passing commented Jun 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

pljones commented May 31, 2021 •

edited

Loading

passing May 31, 2021 •

edited

Loading

pljones commented Jun 1, 2021 •

edited

Loading

pljones commented Jun 1, 2021 •

edited

Loading

passing commented Jun 1, 2021 •

edited

Loading

pljones commented Jun 1, 2021 •

edited

Loading

pljones commented Jun 3, 2021 •

edited

Loading

pljones commented Jun 3, 2021 •

edited

Loading

pljones commented Jun 5, 2021 •

edited

Loading

pljones commented Jun 13, 2021 •

edited

Loading