Igor Kushnir authored Jan 27, 2022 and Milian Wolff committed May 28, 2022

commentRepository() returned a reference to StringRepository, which
could be accidentally used directly rather than via the helper function
commentRepositoryOp() that locks the mutex. Prevent such usage mistakes
by encapsulating the mutex and the repository in class CommentRepository
similar to class StaticCacheData in modificationrevision.cpp.

This new code is functionally equivalent but much simpler to
understand. This was suggested by Igor Kushnir, many thanks!

Instead, add lock/unlock API that operates on the internal
mutex and use that from the ItemRepositoryRegistry where we are
calling the AbstractItemRepository API which used to lock internally.

I.e. mark the implementations in ItemRepository as final because
we don't want anyone else to mess with these functions. Sadly
the item repo itself cannot be final yet as the set repos (ab)use
this still. Maybe we can further clean that up eventually.

Use direct initialization and make const what should be const.

Instead, take the mutex type which allows us to use either a
QRecursiveMutex or a normal QMutex. Then simplify the code
and remove the ItemRepositoryPrivate again, which is now obsolete.

Introduce mirrored API in the Private type which assumes the mutex
is locked to prevent deadlocks when one function calls another there
as happened in e.g. filteredDeclarations.

Pretty straight forward, just lock the mutex once for every
action instead of multiple times when adding/removing items.
Once again I don't think the old code was correct when multiple
threads concurrently ran e.g. addItem/updateItem/removeItem on
the same key.

Remove unused mutex getter and only forward statistics instead of
the full internal item repository.

The implementation is based on that established already by
IndexedIdentifier.

Make the utility code from identifier.cpp reusable for that purpose.
Use a struct ItemRepositoryUtils which can be made a friend such
that we can continue to access m_refcount, even for the instantiation
information types.

Same pattern we established elsewhere. We cannot yet remove the
recursive mutex as the instantiation information requires that
and could otherwise lead to deadlocks. We'll revisit this later.

Instead of giving access to the full manager, only let the mutex
be known and reusable for the instantiationinformation repo.

Ensure the mutex is always locked when we operate on the item
repositories. This way, we only lock the mutex once for every
high level operation instead of potentially multiple times, e.g. to
lookup an index and then access the corresponding item.

I initially wanted to leverage this then to get rid of the recursive
mutex here too, like was done in other areas. But it seems that here
this is not possible, as the IndexedQualifiedIdentifier can load
IndexedIdentifiers, and by sharing the mutex we would need to have
that recursive. I.e. using a non-recursive mutex could trigger this
deadlock here:

```
(gdb) bt
#0  0x00007ffff3ea018d in syscall () at /usr/lib/libc.so.6
#1  0x00007ffff450c316 in QBasicMutex::lockInternal() () at /usr/lib/libQt5Core.so.5
#2  0x00007ffff6bfaf3d in QMutexLocker::QMutexLocker(QBasicMutex*)
    (this=0x7fffffffbb18, m=0x7ffff7a30c68 <KDevelop::RepoForItem<KDevelop::IndexedIdentifier>::repo()::mutex>) at /usr/include/qt/QtCore/qmutex.h:238
#3  0x00007ffff6cd2e20 in KDevelop::repositoryOp<KDevelop::IndexedIdentifier, KDevelop::inc<KDevelop::IndexedIdentifier>(KDevelop::IndexedIdentifier*)::<lambda(auto:23&)> >(struct {...} &&) (op=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:172
#4  0x00007ffff6cd2eb7 in KDevelop::inc<KDevelop::IndexedIdentifier>(KDevelop::IndexedIdentifier*) (item=0x5555556cf4d4)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:377
#5  0x00007ffff6cd26e8 in KDevelop::IndexedIdentifier::IndexedIdentifier(unsigned int) (this=0x5555556cf4d4, index=65604)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:1392
#6  0x00007ffff6cd2764 in KDevelop::IndexedIdentifier::IndexedIdentifier(KDevelop::IndexedIdentifier const&) (this=0x5555556cf4d4, rhs=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:1406
#7  0x00007ffff6cde959 in KDevelop::AppendedList<false, KDevelop::IndexedIdentifier>::copy(KDevelop::IndexedIdentifier*, KDevelop::IndexedIdentifier const*, unsigned int) (this=0x5555556cf4d0, target=0x5555556cf4d4, data=0x5555557195a0, size=1)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/appendedlist_static.h:148
#8  0x00007ffff6cdab26 in KDevelop::QualifiedIdentifierPrivate<false>::identifiersCopyAllFrom<KDevelop::QualifiedIdentifierPrivate<true> >(KDevelop::QualifiedIdentifierPrivate<true> const&) (this=0x5555556cf4c4, rhs=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:233
#9  0x00007ffff6cd7165 in KDevelop::QualifiedIdentifierPrivate<false>::copyListsFrom<KDevelop::QualifiedIdentifierPrivate<true> >(KDevelop::QualifiedIdentifierPrivate<true> const&) (this=0x5555556cf4c4, rhs=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:235
#10 0x00007ffff6cd4db9 in KDevelop::QualifiedIdentifierPrivate<false>::QualifiedIdentifierPrivate<true>(KDevelop::QualifiedIdentifierPrivate<true> const&)
    (this=0x5555556cf4c4, rhs=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:216
#11 0x00007ffff6cd4358 in KDevelop::QualifiedIdentifierItemRequest::createItem(KDevelop::QualifiedIdentifierPrivate<false>*) const
    (this=0x7fffffffbe80, item=0x5555556cf4c4) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:314
#12 0x00007ffff6cdb8a3 in KDevelop::Bucket<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, 0u>::index(KDevelop::QualifiedIdentifierItemRequest const&, unsigned int)::{lambda()#1}::operator()() const (__closure=0x7fffffffbd30)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:289
#13 0x00007ffff6cdbef8 in KDevelop::Bucket<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, 0u>::index(KDevelop::QualifiedIdentifierItemRequest const&, unsigned int) (this=0x5555555d8b30, request=..., itemSize=20)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:424
#14 0x00007ffff6cd77b5 in KDevelop::ItemRepositoryPrivate<KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, false, 0u, 1048576u>, KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, 0u, 1048576u>::index(KDevelop::QualifiedIdentifierItemRequest const&) (this=0x7fffd79d1028, request=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:1185
#15 0x00007ffff6cd4fdb in KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, false, 0u, 1048576u>::index(KDevelop::QualifiedIdentifierItemRequest const&) (this=0x7fffd79d1010, request=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/serialization/itemrepository.h:2326
#16 0x00007ffff6cd1caa in operator()(KDevelop::QualifiedIdentifierRepository&) const (__closure=0x7fffffffbf00, repo=warning: RTTI symbol not found for class 'KDevelop::ItemRepository<KDevelop::QualifiedIdentifierPrivate<false>, KDevelop::QualifiedIdentifierItemRequest, true, false, 0u, 1048576u>'
...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:1230
#17 0x00007ffff6cd1d63 in KDevelop::repositoryOp<KDevelop::IndexedQualifiedIdentifier, KDevelop::QualifiedIdentifier::makeConstant() const::<lambda(KDevelop::QualifiedIdentifierRepository&)> >(struct {...} &&) (op=...) at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:173
#18 0x00007ffff6cd1dc1 in KDevelop::QualifiedIdentifier::makeConstant() const (this=0x7fffffffbf90)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:1229
#19 0x00007ffff6cd0787 in KDevelop::QualifiedIdentifier::operator=(KDevelop::QualifiedIdentifier const&) (this=0x7fffffffbfa0, rhs=...)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/identifier.cpp:850
#20 0x000055555556070a in TestIdentifier::testQualifiedIdentifier() (this=0x7fffffffc7d0)
    at /home/milian/projects/kde/src/kdevelop/kdevplatform/language/duchain/tests/test_identifier.cpp:134
#21 0x000055555555cb73 in TestIdentifier::qt_static_metacall(QObject*, QMetaObject::Call, int, void**)
    (_o=0x7fffffffc7d0, _c=QMetaObject::InvokeMetaMethod, _id=4, _a=0x7fffffffc130)
    at /home/milian/projects/kde/build/kdevelop/kdevplatform/language/duchain/tests/test_identifier_autogen/EWIEGA46WW/moc_test_identifier.cpp:105
#22 0x00007ffff46c66bb in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const () at /usr/lib/libQt5Core.so.5
#23 0x00007ffff7f10ce2 in QTest::qRun() () at /usr/lib/libQt5Test.so.5
#24 0x00007ffff7f11eb2 in QTest::qExec(QObject*, int, char**) () at /usr/lib/libQt5Test.so.5
```

For now, I'll leave this part as-is and investigate changes in a
follow-up patch. For now, the changes proposed here are already good
for readability alone I believe.

We must not have multiple copies of the same repository alive,
prevent that from happening by accident here.

Introduce helper functions that are shorter to call instead of
repeating the relatively long invocation all the time.

This approach was already used in IndexedQualifiedIdentifier's
operator= and I see no reason to not do it here too.

There's no need to repeat ourselves excessively here, let's just
delegate the code to central functions that operate on raw index
values directly.

This way clang-format doesn't get confused.

Again, use locking on the outside here and disable internal locking.

Same same of the by now established pattern: Use external locking
to ensure proper serialization across threads which looks much
more solid to me than individual locking that could lead to bad
interleaved behavior in the worst case.

Again, provide the mutex externally and lock it manually before
accessing the item repository API.

The code here is pretty ugly and brittle, but I hope I'm not making
things worse while touching it here. Testing shows that it all still
seems to work even on my 8 core / 16 thread laptop.

Same as before, I believe the old code wasn't necessarily safe.
We now provide the mutex externally which should be more reliable.

Don't lock the mutex internally but instead lock it externally
before accessing the repository. This will mean the same lock is
held for longer periods of times instead of getting relocked
multiple times, but I believe that the old code was not actually
safe in that regard - what guaranteed that itemFromIndex returned
something isn't getting removed while we are accessing it?

While the IndexedString used a repository with a non-recursive mutex,
this was not a setup that worked in general: removing an item e.g.
would deadlock in such a scenario. IndexedString got lucky because
it never tries to delete anything from the repository...

Refactor the code as follows:

The bulk of what was ItemRepository is now ItemRepositoryPrivate
and assumes that it gets accessed with external serialization.

The new ItemRepository is just a shim around that Private but it
provides the serialization by using the ThisLocker as before.

This way, the Private class can call its own API in any way it wants
and we don't run into a situation where we would accidentally deadlock
by relocking a non-recursive mutex.

For now, only the tests are using the non-recursive mutex now. Other
repositories will be ported one-by-one now.

It does not lock the mutex and thus is not safe to be called from
the outside. The data can be queried via the statistics() interface
instead.

These are all not thread safe and thus must lock the mutex or
risk undefined behavior.

Igor Kushnir writes:
> ItemRepository::close() was called from:
>
> 1. ~ItemRepository() and ItemRepository::open() - no need for a lock;
> 2. ItemRepositoryRegistry::unRegisterRepository() - no need for a lock because unRegisterRepository() is called only from within ~ItemRepository(). But there seems to be an inefficiency here: close() is called from unRegisterRepository() and then immediately from ~ItemRepository() again. Should perhaps this call be removed from unRegisterRepository()?
> 3. from ItemRepositoryRegistryPrivate::close(), which in turn is called from ~ItemRepositoryRegistry(). Here the missing mutex lock might have been a bug.

Then there was `ItemRepositoryRegistry::printAllStatistics` which
didn't lock the repository mutex when printing statistics.

The code is clearly not thread safe and thus must always lock the
mutex. It is only used through the AbstractItemRepository API from
ItemRepositoryRegistry which definitely doesn't lock the item repo
lock, only the register mutex.

All code paths that use this set repository already lock the mutex,
so there's no need to lock a second mutex on top.

Instead of having m_ownMutex and a way to optionally override
that from the outside, always force the mutex to be provided
from the outside.

This is by itself not a functional change, but it will allow us
to get rid of the (deprecated) recursive mutices one by one, just
like we have established already in the indexedstring.cpp code.
I.e. all of the mutices we are providing are still recursive.

In case of conflict in i18n, keep the version of the branch "ours"
To resolve a particular conflict, "git checkout --ours path/to/file.desktop"

In case of conflict in i18n, keep the version of the branch "ours"
To resolve a particular conflict, "git checkout --ours path/to/file.desktop"