[Development] Deprecating QSharedMemory and QSystemSemaphore

[Fabian Kosmale]

Hi,

to have some reply  in this thread:

First of all, I agree with Thiago's assessment of the API's weaknesses. I believe there might be ways to make
the issues less likely in  practice but I don't see a way to fix this either. Certainly not without some new API
to at least have an atomic create and attach.

I am however a bit concerned about the timeline for the deprecation, and the migration path we want to
offer. There needs to be at least documentation on how to properly port away form QSharedMemory, and
maybe even a "new" QSingleApplication (but one that doesn't use QSM in its implementation).

I don't expect Thiago to do that work, but I'd like to have some commitment that _someone_ will take care
of it before deprecating the classes. That someone might be partially me, but certainly not before 6.7 FF.

My suggestion would be that we
- discuss current QSM patterns, and how to move away from them
- based on that, create documentation and if necessary implement new classes to help with the use cases
- and only then deprecate QSharedMemory.

Fabian

________________________________________
Von: Development <development-bounces at qt-project.org> im Auftrag von Thiago Macieira <thiago.macieira at intel.com>
Gesendet: Donnerstag, 16. November 2023 19:56
An: development at qt-project.org
Betreff: [Development] Deprecating QSharedMemory and QSystemSemaphore

Ref: https://codereview.qt-project.org/c/qt/qtbase/+/518039
http://bugreports.qt.io/browse/QTBUG-111855

For Qt 6.6, I undertook a refactor of those two classes to allow the backend
to be selected at runtime, instead of compile-time, because Apple in their
infinite wisdom[*] decided that App Store applications can't use the System V
API. But in the process I've discovered that those APIs are flawed by design.
That means they aren't really fixable without a source- and behaviour-
incompatible API rewrite. Whether someone decides to do that or not is not
important: the APIs as they stand are flawed, racy, and leak memory.

Therefore, those classes are dangerous to use and need to be avoided. The
change above marks them as deprecated, with no replacement, and schedules them
for removal in Qt 7.0 (probably to be moved to a "Qt6CoreCompat" module).

Worse, the refactor I attempted for 6.6 *made it worse* for Unix systems, with
unavoidable memory leaks.

[*] that was sarcastic. I don't think wisdom played a role at all. I think
they found it too difficult to secure the API, so they didn't.


Q: What's wrong with those classes?
A: The API is full of unavoidable race conditions. In Qt 4.4 or 4.5 when Ben
and I designed and reviewed those two classes, we attempted to make a common
API that would work on Unix and Windows systems, by choosing a simple and
common behaviour that would work across the two OS families. But "past us" had
much less experience and there are definite flaws that we didn't notice.

There are two major flaws in both classes:

1) improperly separate creation from attachment to an existing object. This
causes race conditions if two applications attempt to create the object at the
same time. It isn't clear what happens: whether the object gets replaced or
not, whether attachment happens for the failed creation, etc. QSharedMemory
attempts to fix this by using a QSystemSemaphore to control access, but since
QSystemSemaphore is racy on its own (and in a worse way), that just moves the
goalpost and introduces a second failure point.

2) improperly inform ownership of the object. This is the biggest of the two.
This happens because on Windows, the objects disappear when the last user
disconnects from them, but not so on Unix systems, so the backends attempt to
detect the situation and delete. However, the detection is racy and may choose
to delete at the moment that another application attempts to attach (see
problem #1). It's unclear what happens on Unix systems if two processes race
the deletion or race a deletion with an attachment.

To add insult to injury, #2 got worse in Qt 6.6 because the POSIX backend (the
one Apple wants us to use and I made default) does not support detecting the
last user at all. This means QSharedMemory/POSIX *never deletes* the shared
memory object. We needed an API for the user to explicitly choose to delete,
but I find that's covering the Sun with a sieve.

There's a third, minor flaw in QSharedMemory:
3) QSharedMeory::locked() exists. That suggests to people that this is a good
way to write shared memory code. Instead, they should strive to write thread-
safe code, and only if it is not possible use a locking primitive. Said
locking should be optional.


Q: I still need shared memory, what can I use instead?
A: Use QFile::map(). A memory-mapped file without the QFile::MapPrivateOption
is shared memory. See
https://doc.qt.io/qt-6/shared-memory.html#sharing-memory-via-memory-mapped-files

Files are well-understood and common across all OSes, which should also make
reviewing code for problems and race conditions much simpler.

For example, using QTemporaryFile and renaming, you can prepare the shared
memory area before committing it to visibility to other processes.


Q: I need an equivalent of QSystemSemaphore, what can I use instead?
A: Use QFile::map() and put an atomic or a PThread locking primitive there.
Alternatively, use QLockFile.


Q: But who deletes the file?
A: That's up to you to decide for your application. If we could have had a
one-size-fits-all solution, we'd have fixed QSharedMemory because this is
exactly the nature of the second flaw above (POSIX shared memory objects *are*
files on Linux).

My recommendation is that there's a central process that creates the file and
is always responsible for deleting it. This process must start before and exit
after any other processes that attempt to use the object.

If you really must know if anyone is still using it, add a QBasicAtomicInt
somewhere in it and implement a never-increment-from-zero reference counter in
it.


Q: What should I use for rendezvousing (i.e., this unclear-shared-ownership
model)?
A: Here are a couple of suggestions:
 - a D-Bus well-known bus name
 - a listening socket
 - a Q(Temporary)File with a well-known name
aside from files, the others automatically clean up on application crashes too


Q: My code is working fine for now, I don't want to touch it. What should I do?
A: Add
 #define QT_NO_DEPRECATED_IPC
and the deprecation warnings will be silenced.


Q: Should we use the new API in Qt 6.6?
A: No, they're dead on arrival.


Q: Will you work on a replacement API?
A: No. QFile suffices for my needs and I've lost interest / motivation in this
functionality.


Q: Will you accept if someone else works in fixing these classes or providing a
replacement?
A: Sure. I'll be happy to review your code and offer my insight. I just won't
do the coding myself.

--
Thiago Macieira - thiago.macieira (AT) intel.com
  Cloud Software Architect - Intel DCAI Cloud Engineering