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.

Sign up for GitHub

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

Jump to bottom

Implement a background monitor #319

Merged
merged 23 commits into from
May 16, 2019
Merged

Implement a background monitor #319

merged 23 commits into from
May 16, 2019

Conversation

matthiasclasen
Copy link
Contributor

Implement a facility that monitors running flatpaks,
and notifies the user if they run in the background.
Add a permission table to store users responses
persistently.

The platform-dependent parts of this (getting the
state of running applications, and notifying the user)
are handled by a portal backend.

There is currently no application API.

This was referenced May 13, 2019
Merged
Closed
@matthiasclasen
Copy link
Contributor Author

See flatpak/xdg-desktop-portal-gtk#198 for the backend

@matthiasclasen
Copy link
Contributor Author

I've added the background portal to this PR, so applications can now request run-in-background permissions ahead of time.

@matthiasclasen matthiasclasen mentioned this pull request May 14, 2019
Closed
@hadess
Copy link
Contributor

hadess commented May 14, 2019

I had a quick look through this:

  • the org.freedesktop.portal.Background interface doc doesn't make it clear why it needs a window identifier, and it took me a while to realise that this was to show a dialogue to ask for permission, not for the functionality itself.
  • "Rename access impl" should probably be squashed into the original commit

Is there something that terminates the Flatpak when "running in the background" is disabled and the application is not otherwise running?

alexlarsson
alexlarsson reviewed May 14, 2019
View reviewed changes
src/background.c Outdated Show resolved Hide resolved
@alexlarsson
Copy link
Member

In some earlier permutation of this we had some leeway for being in the background to avoid the "race condition" where an app was just starting in parallel with the app background check. Like, allow the app to be in the background for one cycle, and only ask the second time we check. I think we need something like that to be robust.

alexlarsson
alexlarsson reviewed May 14, 2019
View reviewed changes
src/background.c Outdated Show resolved Hide resolved
@matthiasclasen
Copy link
Contributor Author 

* the `org.freedesktop.portal.Background` interface doc doesn't make it clear why it needs a window identifier, and it took me a while to realise that this was to show a dialogue to ask for permission, not for the functionality itself.

I copied the usual docs over now.

* "Rename access impl" should probably be squashed into the original commit

Will do.

Is there something that terminates the Flatpak when "running in the background" is disabled and the application is not otherwise running?

The background monitor checks every few minutes and kills apps that are running in the background and are not allowed. We could probably add a way to trigger that, so the control-center can call it when you change permissions there.

@matthiasclasen matthiasclasen force-pushed the background-monitor branch 3 times, most recently from 6aa4081 to 7420692 Compare May 14, 2019 16:41
@matthiasclasen
Copy link
Contributor Author

In some earlier permutation of this we had some leeway for being in the background to avoid the "race condition" where an app was just starting in parallel with the app background check. Like, allow the app to be in the background for one cycle, and only ask the second time we check. I think we need something like that to be robust.

Still the case. I added a comment to make that clear.

alexlarsson
alexlarsson reviewed May 15, 2019
View reviewed changes
src/background.c Outdated Show resolved Hide resolved
alexlarsson
alexlarsson reviewed May 15, 2019
View reviewed changes
src/background.c Outdated Show resolved Hide resolved
alexlarsson
alexlarsson reviewed May 15, 2019
View reviewed changes
src/background.c Outdated Show resolved Hide resolved
alexlarsson
alexlarsson reviewed May 15, 2019
View reviewed changes
src/background.c Outdated

g_debug ("Killing app %s", app_id);

instance = find_instance (app_id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there are multiple instances? This seems to be a way to escape this. We should kill them all.

@alexlarsson
Copy link
Member

What about commandline apps. I wonder if there is a way to tie an app tty to a terminal and mark that as not backgrounded.

Matthias Clasen added 4 commits May 15, 2019 15:22
fixup! background: Add autostart support
dc6bccd 
Make the NotifyBackground api more flexible
fixup! background: Monitor running apps
327a5d0 
Don't use GTask
fixup! background: Add autostart support
91c9c24 
Stop quoting
fixup! background: Add autostart support
4f60ff2 
Use xdp_app_info_rewrite_commandline
Stop linking against libflatpak
c80af15 
The configure check for flatpak was a leftover from
when we stopped linking our tests against libflatpak.
We should not bring this back for the main binary.
Instead, ship a standalone copy of FlatpakInstance,
and use that.
@matthiasclasen
Copy link
Contributor Author

I haven't looked at the code more, but one thing that came to mind:

* does the portal remove the autostart symlink when the permission store is updated another way (through the Settings for example)?
* what happens when if the portal isn't running at that time? (eg. do we need to modify the Settings so it goes through the portal to modify that, or at least ensure that it's running)

Currently not. But autostart is not a permission either. Just a file we put in place or remove. If you get autostarted and run in the background without permission for that, you will get killed by the monitor after a little while.

Would it be easier to implement parts of this at the Flatpak level, with the autostart desktop files always being installed but with the .desktop files rewritten so that the background bit only ever get started when the permissions allow it?

We decided to keep flatpak out of it. autostart files are no longer installed by the app, they are created by the portal, and we control what goes in them (ie no conditions, etc). This will make it easier to switch to systemd units later, if we choose to do so.

We might still need to figure out how the session is supposed to know about all this, to shut down background programs that got the permission removed. Maybe @benzea has an idea?

The portal has a monitor that kills flatpaks that are running in the background without the permission.

@matthiasclasen
Copy link
Contributor Author

What about commandline apps. I wonder if there is a way to tie an app tty to a terminal and mark that as not backgrounded.

Haven't thought about that. For now, just giving them the permission to run in the background seems easy enough

Matthias Clasen added 2 commits May 15, 2019 19:17
fixup! background: Monitor running apps
54844e9 
Redo application tracking
fixup! background: Monitor running apps
16d3662 
Add an allow-once option
@matthiasclasen
Copy link
Contributor Author

I'm happy with this now.

alexlarsson
alexlarsson reviewed May 16, 2019
View reviewed changes
src/background.c Outdated Show resolved Hide resolved
alexlarsson
alexlarsson reviewed May 16, 2019
View reviewed changes
src/background.c Outdated Show resolved Hide resolved
alexlarsson
alexlarsson reviewed May 16, 2019
View reviewed changes
src/background.c Outdated Show resolved Hide resolved
@alexlarsson
Copy link
Member

I think this looks generally good to go, but the thread issues have to be fixed.

Matthias Clasen added 3 commits May 16, 2019 11:11
fixup! background: Monitor running apps
452ef9d 
Actually make the timeout a minute
fixup! background: Add autostart support
4bf155d 
better option validation
fixup! background: Monitor running apps
45cbf03 
bring back locking
@alexlarsson
Copy link
Member

Otherwise looks good to me!

@matthiasclasen matthiasclasen merged commit c81e724 into flatpak:master May 16, 2019
@TingPing TingPing mentioned this pull request Jul 9, 2019
Closed
@darkdragon-001 darkdragon-001 mentioned this pull request Sep 19, 2020
Closed
@smcv smcv mentioned this pull request Sep 15, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexlarsson alexlarsson alexlarsson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthiasclasen @hadess @alexlarsson