Solid will create different backends for each separate thread but then some backends will use singletons to share some information.
This patch includes measures so this sharing only happens within the same thread or adds the necessary locks so threads respect each other.
Details
Details
- Reviewers
davidedmundson bruns - Group Reviewers
Frameworks - Commits
- R245:00abb4839abe: Make sure solid backends are reentrant
Tests pass, logic makes sense to me, plasmashell doesn't crash when using D22333 (not that it fixes any crashes there)
Diff Detail
Diff Detail
- Repository
- R245 Solid
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
How do you guarantee *each thread local* copy now is connected to the right signals, i.e. the socket notifier for the mtab and inotify for fstab?
Comment Actions
each backend will create things separately now, so it should work like it used to so far when there was just one thread involved.
Comment Actions
The invalidation has to be done per thread then as well, which you don't do. See fstabmanager.cpp
Comment Actions
As you can see in fstabmanager.cpp:40, FstabWatcher::instance() stays a singleton. It will notify about changes and each thread's backend will react to it accordingly.
Comment Actions
Why was this committed when changes were requested? Wasn't that the whole point of having reviews?
Comment Actions
I don't endorse things being closed whilst there are still review comments, but replying purely on the tech side:
How do you guarantee *each thread local* copy now is connected to the right signals, i.e. the socket notifier for the mtab and inotify for fstab?
Even before this patch frontend/DeviceManager puts each Backend in a thread local storage.
Each backend (and child tree) is a separate object. It only need to clear it's own TLS.
(though it does beg the question of why they're static in the backend rather than just member vars at that point)
Comment Actions
I would have preferred this to only land when D22333 had been addressed fully, I assumed this required no further notice.
So from a pure technical view, this indeed seems to be correct, but from an architectural view this seems less than ideal (to put it mildly).
One obvious issue is the multiple instantiation and initialization, i.e. when the UDisks2 backend is used from different threads each one has to introspect udisks itself, which seems wasteful for me.
I will extend this thought in D22333, can we move any further discussion there?