Persistent server lists using --directoryfile #1867

pljones · 2021-06-13T13:33:56Z

Short description of changes

This change makes the server lists supplied by directory servers more reliable.

Context: Fixes an issue?

A number of users have complained server lists are not available reliably. When a directory server is restarted, it could take up to 15 minutes for all the servers registered with it to re-register. With this change in place, the directory should be fully populated on start up.

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

The help output has been added.

The Server Administrators' Guide will need updating to cover this option, when it's needed and how to use it.

It relies on the server administrator specifying a file in which to store the list across restarts.

Status of this pull request

Replacement for #1799 and pljones#9. Makes #1803 and #1804 redundant. All of those now closed.

This is running on all my directory servers (i.e. the genre-specific ones, plus my additional ones).

I'll also be using the Windows Client code (just to confirm no adverse side effects) from 19th June onwards.

What is missing until this pull request can be merged?

Review of the latest version of the code.

Checklist

I 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.

src/server.cpp

src/serverlist.cpp

src/main.cpp

ann0see · 2021-06-18T08:47:43Z

Todo:

Some more tests
In deph review (I don't feel comfortable doing that; however, I trust you that it should all be ok)

pljones · 2021-06-18T16:19:31Z

Todo:

* [ ]  Some more tests

Agreed - I've run this the way I expect it to work. I've not tried to break it at all.

* [ ]  In deph review (I don't feel comfortable doing that; however, I trust you that it should all be ok)

Also agreed. I know @softins has looked at the directory server code; it also looks like @passing and @hoffie have touched it:

peter@fs-peter:~/git/Jamulus-wip$ git blame src/serverlist.cpp | grep -v '(Volker Fischer   \|(Peter L' | sed -e 's/^[^(]*(//' -e 's/  *20.*) .*$//' | sort -u

passing · 2021-06-18T17:15:37Z

With 2bd1654 I have touched all the code 😄

pljones · 2021-06-18T17:25:13Z

With 2bd1654 I have touched all the code 😄

Ah yes, true...

ann0see · 2021-06-19T16:33:42Z

~~Jamulus -h gives back --directoryfile not --directorylist --> change cli option or pull request title depending on which name we want to use.~~ (fixed)

No error message is given if we run a non directory server. (Fixed for non GUI servers)

If we don't give a file name but a directory, the server does not exit and a warning message is not quite obvious. Shouldn't the "Could not open ..." etc. messages be a bit more prominent (e.g. by appending "WARNING:") (fixed)

Only after gracefully stopping a directory the file gets written which means that we don't handle a crash. Is this intended?

All in all I think it's on a good way to get merged.

pljones · 2021-06-20T07:41:35Z

Thanks for the negative tests!

No error message is given if we run a non directory server.

Well, a warning message - it'll carry on starting up at that point.

If we don't give a file name but a directory, the server does not exit and a warning message is not quite obvious. Shouldn't the "Could not open ..." etc. messages be a bit more prominent (e.g. by appending "WARNING:")

Mm, yes... I need to say "either it doesn't exist or it's a file". I could do that in src/main.cpp, too. That still doesn't give reliable results -- you could pass a name that doesn't exist that can't be created, as you point out: it's still got to be something that can be written to.

Overall, I'm tempted to going back to requiring an existing, readable, writeable file to be passed: no sudden surprises that way.

pljones · 2021-06-20T11:56:12Z

Hm. Made worse by the server ini file. I can use a server ini file to set my server list to "custom" "localhost" and run as a directory server.

On start up, as I don't supply --serverlist, validation can't check -- because the ini file hasn't been loaded -- whether or not a combination of options is actually valid.

Like I've said elsewhere, we need to refactor start up further -- parse the command line options, load the ini file (either default or specified) and then override ini file from command line. Currently, it's the other way around (ini file overrides command line), which feels totally wrong. Once all of that's been done, then validate the settings work together -- you (should) know what got to the ini file was valid, so it's only going to be command line options causing the problem. As it stands, I can't know whether I'm running as a directory server when parsing the command line options. Or even if I'm registering.

pljones · 2021-06-20T13:33:26Z

Well, after several attempts, I've come up with something that does the best it can.

If we're a headless server, there is no inifile (regardless of passing the argument), so the command line specifies everything there is to know. Otherwise, certain checks cannot be made as the inifile might change things.

I might have missed some bits, though.

Ugh, I also left some changes in I'd meant to revert.

pljones · 2021-06-20T18:31:23Z

And, of course, whilst I was tidying up, I did some more changes...

Now running on all my directory servers and my jam server.

ann0see · 2021-06-20T18:53:45Z

src/main.cpp

+                if ( !strServerListFilter.isEmpty() )
+                {
+                    qWarning() << "Server list filter will only take effect when running as a directory server.";
+                    strServerListFileName = "";
+                }
+            }
+            else
+            {
+                if ( !strServerListFileName.isEmpty() )
+                {
+                    QFileInfo serverListFileInfo ( strServerListFileName );
+                    if ( !serverListFileInfo.exists() )
+                    {
+                        QFile strServerListFile ( strServerListFileName );
+                        if ( !strServerListFile.open ( QFile::OpenModeFlag::ReadWrite ) )
+                        {
+                            qWarning() << qUtf8Printable (
+                                QString ( "Cannot create %1 for reading and writing.  Please check permissions." ).arg ( strServerListFileName ) );
+                            strServerListFileName = "";
+                        }
+                    }
+                    else if ( !serverListFileInfo.isFile() )
+                    {
+                        qWarning() << qUtf8Printable (
+                            QString ( "Server list file %1 must be a plain file.  Please check the name." ).arg ( strServerListFileName ) );
+                        strServerListFileName = "";
+                    }
+                    else if ( !serverListFileInfo.isReadable() || !serverListFileInfo.isWritable() )
+                    {
+                        qWarning() << qUtf8Printable (
+                            QString ( "Server list file %1 must be readable and writeable.  Please check the permissions." )
+                                .arg ( strServerListFileName ) );
+                        strServerListFileName = "";
+                    }
+                }
+


Hmm. It's a lot of new code in main.cpp then.

Yep - you suggested that --directoryfile should only be allowed when running as a directory server. That meant lots of new code. As I was doing it for the one option, it felt necessary to be consistent, so I did it for as many I as I could. And that meant refactoring to avoid repetition.

It's a lot cleaner now than it was, with multiple "if this then not that" sections all mixed in with each other.

Yes. I know. Probably it's a good move then anyway. So the next steps would be another few tests and another person who reviews this.

Currently CClientSettings and CServerSettings need, respectively, to be passed an instance of CClient and CServer. Which means you have to create the Client or Server before loading the inifile. The class in question configures itself according to its defaults or command line option overrides. Then, if there's an inifile (i.e. not headless server), a whole load of other options get applied, changing how the class is configured.

This is why you cannot know whether a server is a directory server if it's not headless when

validating the command line options

when creating the server instance

It's not until during inifile loading that you know. That's after the directoryfile parameter has been set in the server. Too late now!

All of this is confusing -- it leaves me totally confused every time I have to think about it, anyway.

So what I want to do is make it so:

the command line arguments are read

the decision on Client vs Server is made

the appropriate CSettings object gets created, but just reading the entries in from the inifile

the command line options are sanity checked against the CSettings object and, if sane, applied

the CClient or CServer object gets created, passing in the CSettings object

the application runs, making changes to the CSettings object in place of the current settings values

on shutdown, the CSettings object is saved to the inifile - excluding any entry from the command line options, which is left "as is" in the file

(Volker didn't like it...)

Thanks for the explanation.

This seems to explain why my tests with the GUI didn't do what I thought they would do: I set up a GUI directory via CLI which resulted in a message saying it is a directory, but I couldn't register with it or have the register my server box checked.

There must be a reason why Volker defended the current implementation?

I think it was very much "that's the way it's always worked, why change it?". Which is valid if you're trying to reduce the amount of change being made to something you have [edit]limited[/edit] time to manage, which he was.

ann0see · 2021-06-20T19:04:33Z

Compiling on Linux gives a warning:

In file included from src/serverlist.cpp:25:
src/serverlist.h: In constructor ‘CServerListManager::CServerListManager(quint16, const QString&, const QString&, const QString&, const QString&, const QString&, int, CProtocol*)’:
src/serverlist.h:213:16: warning: ‘CServerListManager::pConnLessProtocol’ will be initialized after [-Wreorder]
  213 |     CProtocol* pConnLessProtocol;
      |                ^~~~~~~~~~~~~~~~~
src/serverlist.h:203:19: warning:   ‘ESvrRegStatus CServerListManager::eSvrRegStatus’ [-Wreorder]
  203 |     ESvrRegStatus eSvrRegStatus;
      |                   ^~~~~~~~~~~~~
src/serverlist.cpp:94:1: warning:   when initialized here [-Wreorder]
   94 | CServerListManager::CServerListManager ( const quint16  iNPortNum,
      | ^~~~~~~~~~~~~~~~~~

Probably you'll need to move methods around.

ann0see · 2021-06-21T18:39:33Z

Just re-ran the auto build again since Android seemed to be broken. I think this PR would be ready then, however, I'd like to wait for others to test it too.

pljones · 2021-06-22T06:29:49Z

Compiling on Linux gives a warning:

Good spot - CodeQL should have objected more vigorously to the new warning, too.

ann0see · 2021-06-27T11:41:01Z

Missing:

@ann0see test again.
Someone else with more knowledge on the directory server handling needs to review and approve this PR

ann0see

Works as expected; however the cli handling is - as you mentioned - not perfect. But that's not part of this PR.

pljones · 2021-06-29T21:12:45Z

OK, I've just split #1896 out from this - this still has that commit in, as I'm depending on it locally.

softins

This appears to include all the changes that are separately in #1896. I've approved the latter; maybe that needs to be merged first and this PR then rebased again?

ann0see · 2021-07-13T19:42:14Z

No. I think if this PR is merged, the other one will close automatically?

…mmand line option

ann0see · 2021-07-16T21:32:22Z

@softins this PR is about something else (persistent server lists for directories)?

softins · 2021-07-17T12:10:34Z

@softins this PR is about something else (persistent server lists for directories)?

Yes, but it had been branched off the commit for #1896 rather than directly off master. Now that #1896 has been merged, this PR only contains code relevant to the intended improvement.

On first read through it looks good. I'll try it out later and approve after that.

softins

Just tested this out on my Pi and it appears to work fine. Approving. Thank you Peter!

pljones force-pushed the feature/persistent-server-lists branch from 2228e33 to 69c4d1c Compare June 13, 2021 13:35

ann0see reviewed Jun 16, 2021

View reviewed changes

src/main.cpp Outdated Show resolved Hide resolved

ann0see reviewed Jun 16, 2021

View reviewed changes

src/main.cpp Show resolved Hide resolved

src/main.cpp Show resolved Hide resolved

src/main.cpp Outdated Show resolved Hide resolved

src/main.cpp Outdated Show resolved Hide resolved

ann0see added this to the Release 3.8.1 milestone Jun 16, 2021

pljones force-pushed the feature/persistent-server-lists branch from 69c4d1c to 81c4a5d Compare June 17, 2021 17:28

pljones mentioned this pull request Jun 17, 2021

Add macOS signing changes #1856

Merged

pljones changed the title ~~Persistent server lists using --directorylist~~ Persistent server lists using --directoryfile Jun 20, 2021

pljones force-pushed the feature/persistent-server-lists branch from 81c4a5d to cf3b6f0 Compare June 20, 2021 13:31

pljones force-pushed the feature/persistent-server-lists branch 5 times, most recently from 5c0a262 to 1ce9665 Compare June 20, 2021 18:24

ann0see reviewed Jun 20, 2021

View reviewed changes

pljones force-pushed the feature/persistent-server-lists branch from 1ce9665 to a0026a4 Compare June 22, 2021 06:28

pljones force-pushed the feature/persistent-server-lists branch from a0026a4 to 282fc6e Compare June 22, 2021 06:30

ann0see approved these changes Jun 27, 2021

View reviewed changes

pljones force-pushed the feature/persistent-server-lists branch from 282fc6e to 7db4dcf Compare June 29, 2021 20:58

pljones mentioned this pull request Jun 29, 2021

Re-order and refactor help for client/server validation #1896

Merged

5 tasks

pljones force-pushed the feature/persistent-server-lists branch from 7db4dcf to a681ce5 Compare July 6, 2021 07:26

ann0see mentioned this pull request Jul 11, 2021

Document persistent server lists PR jamulussoftware/jamuluswebsite#562

Closed

ann0see requested a review from softins July 11, 2021 18:37

softins reviewed Jul 13, 2021

View reviewed changes

pljones force-pushed the feature/persistent-server-lists branch 3 times, most recently from 59c3e69 to 13cd055 Compare July 16, 2021 16:52

Server list persistence for directory servers with --directoryfile co…

7d9164a

…mmand line option

pljones force-pushed the feature/persistent-server-lists branch from 13cd055 to 7d9164a Compare July 16, 2021 17:53

softins approved these changes Jul 17, 2021

View reviewed changes

ann0see merged commit 249b85a into jamulussoftware:master Jul 17, 2021

pljones deleted the feature/persistent-server-lists branch July 17, 2021 20:45

ann0see added a commit that referenced this pull request Jul 17, 2021

Document #1867

2e1e05e

Persistent server lists using --directoryfile #1867

Persistent server lists using --directoryfile #1867

Uh oh!

Conversation

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

Uh oh!

Short description of changes

Context: Fixes an issue?

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

Status of this pull request

What is missing until this pull request can be merged?

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

pljones commented Jun 18, 2021

Uh oh!

passing commented Jun 18, 2021

Uh oh!

pljones commented Jun 18, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ann0see Jun 20, 2021

Choose a reason for hiding this comment

Uh oh!

pljones Jun 20, 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!

ann0see Jun 20, 2021

Choose a reason for hiding this comment

Uh oh!

pljones Jun 20, 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!

ann0see Jun 20, 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 20, 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!

ann0see commented Jun 20, 2021

Uh oh!

ann0see commented Jun 21, 2021

Uh oh!

pljones commented Jun 22, 2021

Uh oh!

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

Uh oh!

pljones commented Jun 13, 2021 •

edited

Loading

ann0see commented Jun 18, 2021 •

edited

Loading

ann0see commented Jun 19, 2021 •

edited

Loading

pljones commented Jun 20, 2021 •

edited

Loading

pljones commented Jun 20, 2021 •

edited

Loading

pljones commented Jun 20, 2021 •

edited

Loading

pljones commented Jun 20, 2021 •

edited

Loading

pljones Jun 20, 2021 •

edited

Loading

pljones Jun 20, 2021 •

edited

Loading

ann0see Jun 20, 2021 •

edited

Loading

pljones Jun 20, 2021 •

edited

Loading

ann0see commented Jun 27, 2021 •

edited

Loading

ann0see left a comment •

edited

Loading