Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server de-registers on GUI state change #3287

Closed
ann0see opened this issue Jun 7, 2024 · 22 comments · Fixed by #3302
Closed

Server de-registers on GUI state change #3287

ann0see opened this issue Jun 7, 2024 · 22 comments · Fixed by #3302
Assignees
Labels
bug Something isn't working

Comments

@ann0see
Copy link
Member

ann0see commented Jun 7, 2024

I tested running the server on macOS and it needed multiple tries to show up as directory. The directory did not show up for a local client if a path for recordings is set. It may not be related and needs more investigation (test with a path that is not writable).

It seems as if on 3.10.0 the directory shows up instantly.

Ursprünglich gepostet von @ann0see in #3259 (comment)

@softins
Copy link
Member

softins commented Jun 7, 2024

I'll try it here and see if I find the same, and do some diagnosis if I do. But it will probably be after the weekend.

@softins
Copy link
Member

softins commented Jun 7, 2024

I just tried the latest build of mac legacy, and it seems to register ok. I had one occurrence at the start when it registered, but then dropped back pretty quickly to not registered. But I've been unable to replicate that with several attempts: it has been registering fine and staying registered.

@ann0see Is there a repeatable way you have found to demonstrate the problem?

Or maybe I have misunderstood the description of the issue?

@ann0see
Copy link
Member Author

ann0see commented Jun 11, 2024

I had one occurrence at the start when it registered, but then dropped back pretty quickly to not registered.

I think that's what I saw. I don't have macOS access at the moment. Sorry for it being vague. It may have to do with restarting the server having a recording directory set to which the application doesn't have access to -> so the reason is the macOS sandbox.

If the directory is set again, the sandbox has a whitelist to the chosen directory. But this may be reset after an app restart.

@ann0see
Copy link
Member Author

ann0see commented Jun 11, 2024

Let me try to come up with steps to reproduce:

  1. Install a build with hardened runtime/sandbox enabled
  2. Set a recording directory
  3. Set custom directory address to localhost to register as directory.
  4. Choose the custom directory -> everything should work as expected
  5. Close the server
  6. Re open the server and check the directory with a client with custom directory as localhost.
  7. Check the registration status - Register with custom directory and one of the inbuilt directories. See what happens.

@ann0see
Copy link
Member Author

ann0see commented Jun 12, 2024

@softins could you please try again and wait a bit on the Server GUI? I can reproduce this with the new legacy build.

@softins
Copy link
Member

softins commented Jun 15, 2024

  1. Install a build with hardened runtime/sandbox enabled

What does this mean? Is it a special kind of build, or some special setting to apply to an installed application?

@ann0see
Copy link
Member Author

ann0see commented Jun 16, 2024

This means that it's a sandboxed build. Both legacy and normal from the CI should be sandboxed. At least both show the error.

@ann0see ann0see added this to the Release 3.11.0 milestone Jun 23, 2024
@ann0see ann0see added the bug Something isn't working label Jun 23, 2024
@github-project-automation github-project-automation bot moved this to Triage in Tracking Jun 23, 2024
@ann0see
Copy link
Member Author

ann0see commented Jun 23, 2024

Seems like a blocker!

@ann0see
Copy link
Member Author

ann0see commented Jun 23, 2024

@softins can you reproduce this bug on your Mac?

@ann0see
Copy link
Member Author

ann0see commented Jun 23, 2024

Now this also seems to apply to other directories like any genre 1, not just registering with localhost.

@ann0see ann0see moved this from Triage to Backlog in Tracking Jun 23, 2024
@pljones
Copy link
Collaborator

pljones commented Jun 24, 2024

Have you tried downgrading your version of macOS? That would help diagnose the problem, at least.

This occurs on a Qt 6.7.x build, too? If not, it's going to be down to a Qt bug and the fix will never be back-ported.

@ann0see
Copy link
Member Author

ann0see commented Jun 24, 2024

It occurs on legacy and non legacy but doesn't seem to happen on 3.10.0. So I suppose it's a bug on our side.

@pljones
Copy link
Collaborator

pljones commented Jun 25, 2024

What version is the non-Legacy build now?

What was it for 3.10.0?

Does the tagged 3.10.0 release built with the current build chain work or not?

@softins
Copy link
Member

softins commented Jun 28, 2024

I have just been testing on my Macbook. It looks like the latest version of GUI server will register with a directory (I tested using private.jamulus.io), but as soon at its window loses focus, such as when switching to a client to try connecting, the server loses its registration and the GUI reports "Not connected". It then does not appear in the list of servers shown in the client's Connect dialog.

I can see in Jamulus Explorer that the registration did initially succeed, and was maintained as long as the GUI retained focus. When it lost focus, the registration was actively cancelled with the directory, as Explorer showed the server was gone.

I went back to 3.10.0 server, and this behaviour doesn't happen; the registration is maintained when the server window loses focus.

Thankfully, I have about 16 different post-3.10.0 versions of Jamulus and JamulusServer app installed, so I want through them from earliest to latest until I got to one exhibiting the loss of registration.

By checking the commit IDs of the latest working build and the first failing build, I've deduced that the change in PR #3144 is what breaks it. This is the PR that adds the saving of settings on GUI state change. It was intended to fix an issue on the Android version of Jamulus client. But in server mode it does seem to kill the registration. I haven't diagnosed why yet. It's probably not specific to the Mac.

@pljones
Copy link
Collaborator

pljones commented Jun 29, 2024

It's really intended for mobile rather than desktop. If it does affect the Windows desktop (I think I tested it there, though), we could make it mobile-only.

@softins
Copy link
Member

softins commented Jun 29, 2024

I think on the client it's fine, but needs specific testing of the GUI server on the various platforms, and its effect on registration. I'm away for another week, so can't do much except on my Macbook.

@pljones
Copy link
Collaborator

pljones commented Jun 29, 2024

It sounds almost like MacOS is putting the app to sleep following that commit and, as it didn't previously acknowledge "states", it wasn't doing so. Is there some setting that can be used not to put the app to sleep when it's not got focus? Or like the run in background power states on Windows? (I notice 3.10.0 doesn't have the advanced option on Windows... let me check the latest.)

@pljones
Copy link
Collaborator

pljones commented Jun 29, 2024

Oh! It switches to "Not registered" immediately the window loses focus. Strange.

OK, there are builds of the main branch just before the merge of the state change PR and just after
Before: https://github.com/jamulussoftware/jamulus/actions/runs/9724415397
After: https://github.com/jamulussoftware/jamulus/actions/runs/9724432662

@ann0see
Copy link
Member Author

ann0see commented Jun 29, 2024

MacOS is putting the app to sleep

This can happen and is called App Nap I think. But it doesn't seem like that.

@pljones
Copy link
Collaborator

pljones commented Jun 29, 2024

src/settings.cpp 982:

    // we MUST do this after saving the value and Save() is only called OnAboutToQuit()
    pServer->SetDirectoryType ( AT_NONE );

... except Save() is no longer only called OnAboutToQuit(). I think maybe Server OnAboutToQuit() handler should call Save() and pServer->SetDirectoryType ( AT_NONE ) - not expect Save() to de-register the server.


Oh, we have two handlers for the QCoreApplication::aboutToQuit signal -- CSettings and CServer. The latter does not set pServer->SetDirectoryType ( AT_NONE ) because it could run before the former, which would then save the wrong value... Hm. What a mess.


Common to client and server - calls CSettings::Save, which is abstract, so either CClientSettings::Save or CServerSettings::Save is called.

src/settings.h:        QObject::connect ( QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &CSettings::OnAboutToQuit );

Server-only:

src/server.cpp:    QObject::connect ( QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &CServer::OnAboutToQuit );

src/recorder/jamcontroller.cpp-        QObject::connect ( QCoreApplication::instance(),
src/recorder/jamcontroller.cpp-                           &QCoreApplication::aboutToQuit,
src/recorder/jamcontroller.cpp-                           pJamRecorder,
src/recorder/jamcontroller.cpp:                           &CJamRecorder::OnAboutToQuit,
src/recorder/jamcontroller.cpp-                           Qt::ConnectionType::BlockingQueuedConnection );

src/serverlist.cpp:    QObject::connect ( QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &CServerListManager::OnAboutToQuit );

@pljones
Copy link
Collaborator

pljones commented Jun 29, 2024

I think the easier fix is to add a parameter to CSettings::Save that indicates it's being called to handle QCoreApplication::aboutToQuit rather than some other reason.

@pljones pljones changed the title GUI Server needs multiple tries to show up as directory on macOS GUI server de-registers on state change Jun 29, 2024
@pljones pljones self-assigned this Jun 29, 2024
@pljones pljones moved this from Backlog to In Progress in Tracking Jun 29, 2024
@pljones
Copy link
Collaborator

pljones commented Jun 29, 2024

PR raised - if someone could check on MacOS, please?

@pljones pljones changed the title GUI server de-registers on state change Server de-registers on GUI state change Jun 29, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Tracking Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants