ForegroundLock

[David Nolden]

Hi, I've read your IRC discussion about the foreground lock on:
http://webcache.googleusercontent.com/search?q=cache:gPpzdaHMDXcJ:fr.pastebin.ca/2019527+QMutex+bug+foregroundlock&cd=21&hl=de&ct=clnk&gl=de

and want to do some explanations.

- What's it's sense?
Often we require to access some resources that can only be accessed
safely from the foreground, for example the text from a text-editor,
the include-paths from the build-manager, or similar.

The first would be to try making those components thread-safe, however
this means that the whole kdevplatform (and kate) architecture would
have to become thread-safe, which is nearly impossible to create and
maintain properly (we've seen failed tries in kate with the "smart
mutex").

The second solution would be doing those actions _before_ forking into
the background by collecting the required data beforehand. However,
this would also mean a lot of added complexity, because right now, we
don't know which resources exactly we will require when starting a
parse-job for a document. It might process a random number of
includes, it might _not require_ certain expensive informations (like
include paths) as the document is only updated, or similar.

The third solution would be to acquire foreground privileges on-demand
when we require a certain piece of information. We can send an event
to the foreground process and wait for the answer in the background.
This is the most economic approach, because it requires minimal
changes to the source code (only local code in the place where this
mechanism is used). However, it still requires some additional code
for each such pattern.

The ForegroundLock is basically simply a generalization of the third
solution that requires nearly no local code to achieve the same thing.
When you have the foreground-lock held, then you can safely access all
the resources that you could access from within an event executed in
the foreground thread, without losing your current context. The
additional required code is minimal.

Example:
QString editorText;
{
  ForegroundLock lock;
  editorText = kateTextEditor->getText_WouldCrashWithoutForegroundLock();
}

Of course, deadlocks may happen too, like always if you "wait" for
another thread. However they can be prevented using a very simply
rule: Never lock anything else before locking the foreground lock (we
can enforce this using assertions).

- Why the ugly nested class?
Sometimes the foreground starts nested event loops. Those are ugly,
and the only way of dealing with them is even more uglyness. ;-) If an
additional event-loop is started in the foreground, then the
foreground-lock was already held beforehand. We somehow have to force
the foreground to release it, so we can acquire it by ourselves. We
send an event to the foreground within which the foreground releases
the lock, waits for the background to do take it, and then re-acquires
it.
However, unlike what you wrote on IRC, this is not unsafe, at least
not more "unsafe" than _any_ nested event loop. Whenever you start a
nested event-loop anything can happen. if we simply sent an event to
the foreground to do something, we would have the exactly the same
problem.

The only "safety" that the foreground-lock guarantees you is, that the
foreground thread isn't doing anything while you're holding it.

- Why the custom recursion counter?
Only this way we can unlock the foreground lock completely (in the
nested event-loop case).

- Why the volatile?
I added this while debugging, just to make sure that the problems
didn't arise from some caching issues (volatile is supposed to prevent
any kind of caching on the variable). Atomic isn't required. Anyway
the only operation done on those variables without the lock held is
"holderThread == QThread::currentThread()", which is impossible to get
wrong (if it switches between true/false, _our_ thread has to do the
transition, so it cannot be mis-informed about the outcome), so even
the volatile shouldn't be required.

- Isn't it slow? We're blocking the foreground!
Only simple and fast operations should be performed when the
foreground lock is held. Since those operations are not thread-safe
anyway, they must be performed in the foreground anyway, so there is
no way of preventing these costs. However, with the ForegroundLock, we
can still decide at a very late point of the parsing process that we
don't need certain information, and thus we can even reduce the
blocking of the UI. Also, we can retrieve lower amounts of information
in separate event cycles (first the text for just one document, later
the include-paths, etc.), which _reduces_ the blocking (in comparison
to collecting all the info before going into background).

- Issues
About the _only_ issue that I know of regarding the ForegroundLock is
that any QObjects created with the ForegroundLock held will not be
associated with the foreground thread. This is not relevant for 99% of
the API calls though. Apart from this, I still haven't heard any valid
issues, so I don't understand all the uneducated bitching.

- Milian: We need a proper rewrite!!
There is no "proper" rewrite. The way ForegroundLock is, is about the
only way to achieve the goal with such simplicity. If you don't like
it, then the alternative is not changing the foreground lock, but the
only alternative is changing the code that is using it. If you think
that the code will become better, then you can try doing it, and then
send the patch. However, I'm pretty sure that the code will only
become more complicated (and maybe with more UI blocking), without a
benefit (I couldn't think of anything simpler than the example snippet
above). Anyway I'd be interested to see the code, so feel free trying.

Greetings, David