Bug 423931

Summary:	ability to reset the per-thread KSycoca instance
Product:	[Frameworks and Libraries] frameworks-kservice	Reporter:	Harald Sitter <sitter>
Component:	general	Assignee:	David Faure <faure>
Status:	RESOLVED FIXED
Severity:	normal	CC:	kdelibs-bugs
Priority:	NOR
Version:	unspecified
Target Milestone:	---
Platform:	Other
OS:	Linux
See Also:	https://bugs.kde.org/show_bug.cgi?id=423818
Latest Commit:	https://invent.kde.org/frameworks/kservice/commit/d55de6ff8b1d7899e8661e8fe8d65d046318e282	Version Fixed In:	5.75
Sentry Crash Report:

Description Harald Sitter 2020-07-06 15:49:19 UTC

Relates to bug #423928 and kind of to bug #423818

The service runner which includes apps/kcms and the like in krunner/kickoff results, runs in thread-woven threads meaning it can initialize up to N ksysoca instances; where N = thread pool size.

This is problematic since ksycoca includes a kdirwatch and kdirwatch holds an inotify instances and inotify instances are capped per-user, so we can contribute substantially to exhaustion of that limit with unused/unuseful ksycoca instances that hold unused/unuseful kdirwatch instances.

Dealing with this is a bit awkward but the best option might be to grow API that allows the runner to forcefully unset & delete the thread-local KSycoca instance once the runner is done. That way the KDirWatch also gets deleted and the KDirWatch internals can take care of closing the inotify instance.

Comment 1 David Faure 2020-09-07 18:19:13 UTC

I see. Good analysis. And I often ran out of inotify instances (until I increased the limit on my system, but most users don't do that), so I'm all for fixing that.

We could undeprecate KSycoca::disableAutoRebuild() (or call it something else) and in there delete d->m_fileWatcher.

Obviously the caller has to make sure that nothing in that thread relies on ksycoca noticing changes made by other threads/processes...

Comment 2 David Faure 2020-09-07 18:20:35 UTC

(title edited since "global" made me think of the GlobalDatabase (mis-)feature)

Comment 3 Harald Sitter 2020-09-07 18:37:34 UTC

(In reply to David Faure from comment #1)
> We could undeprecate KSycoca::disableAutoRebuild() (or call it something
> else) and in there delete d->m_fileWatcher.

That would disable reloading entirely, right? I believe that would break things a bit.

e.g.
- user starts krunner and types 'vlc'
- vlc isn't installed so the appstream runner sends the user to discover to install vlc
- after installation user types 'vlc' in krunner again
- krunner should now list the actual vlc service

So we need to reload the database, arguably even with the kdirwatch, but if the runner thread is unused for N seconds we want to stop watching and only start again when a servicerunner query begins again.

Comment 4 David Faure 2020-09-09 20:12:10 UTC

This should actually work out of the box.
KSycoca::ensureCacheValid has code that says

"// Check if the file on disk was modified since we last checked it."

So we don't need the file watcher. It's mostly there to detect initial creation, and for the signal emitted by ksycoca.

Comment 5 Bug Janitor Service 2020-09-10 12:13:54 UTC

A possibly relevant merge request was started @ https://invent.kde.org/frameworks/kservice/-/merge_requests/14

Comment 6 Harald Sitter 2020-09-10 12:37:03 UTC

Git commit d55de6ff8b1d7899e8661e8fe8d65d046318e282 by Harald Sitter.
Committed on 10/09/2020 at 12:11.
Pushed by dfaure into branch 'master'.

bring back disableAutoRebuild from the brink

it now deletes the kdirwatch to disable file watching. this allows
krunner plugins to free inotify instances that would needlessly be used
by runner threads in the threadpool.
ensureCacheValid() will make sure database changes are still picked up
when new runner queries happen.
FIXED-IN: 5.75

M  +17   -13   src/sycoca/ksycoca.cpp
M  +7    -6    src/sycoca/ksycoca.h
M  +6    -1    src/sycoca/ksycoca_p.h

https://invent.kde.org/frameworks/kservice/commit/d55de6ff8b1d7899e8661e8fe8d65d046318e282