#2079 startInterpreter() requests the kernel lock while holding the resource lock, the pattern removed from terminateInterpreter() for bug #1734

[Moritz Hoffmann]

Interpreter.hpp states the rule:

IMPORTANT NOTE: To avoid deadlocks, never request the kernel lock while
holding the resourceLock ... It is permissible to request the resource lock
while holding the kernel lock, but this ordering must be strictly observed.

Interpreter::startInterpreter() breaks it:

void Interpreter::startInterpreter(InterpreterStartupMode mode, const char *imageTarget)
{
    ResourceSection lock;

    if (!isActive())
    {
        ...
        if (localServer == OREF_NULL)
        {
            // Get an instance.  This also gives the root activity of the instance
            // the kernel lock.
            InstanceBlock instance;

The comment on that line says what it does: InstanceBlock acquires the kernel
lock, and it does so while the enclosing ResourceSection is held.

This is the same pattern that was removed from Interpreter::terminateInterpreter()
in r12435 for bug #1734, "Hang with multiple threads", which added this comment:

the resource lock needs to be released here because unloading packages will
require the kernel lock, which can never be requested while holding the
resource lock

The startup half was never changed.

Evidence

ThreadSanitizer reports it as a lock order inversion. In one run of the
ProcessInvocation test group there are 212 reports, all of them the same single
cycle:

Cycle in lock order graph: M0 (resourceLock) => M1 (kernel lock) => M0

Mutex M1 acquired here while holding mutex M0:
    ActivityManager::lockKernel
    ActivityManager::getRootActivity
    Interpreter::createInterpreterInstance
    Interpreter::createInterpreterInstance
    InstanceBlock::InstanceBlock
    Interpreter::startInterpreter

Mutex M0 acquired here while holding mutex M1:
    Interpreter::getResourceLock
    ResourceSection::ResourceSection
    ActivityManager::createCurrentActivity
    ActivityManager::getRootActivity
    Interpreter::createInterpreterInstance

The second stack is the permitted direction and is normal. The first is the
violation.

This only becomes visible once the resource lock actually locks. While
Interpreter::resourceLock was never created, there was no lock order to
invert and ThreadSanitizer reported zero inversions.

Is it reachable?

Not today, and I want to be clear about that rather than overstate it.

The violating path is one-time interpreter bootstrap. active is set true in
startInterpreter() and is never set back to false anywhere, and
interpreterInstances never returns to OREF_NULL, so the guarded body runs
exactly once per process. At that moment no other thread can hold the kernel
lock: a competing thread blocks on the resource lock at the top of
createInterpreterInstance() holding nothing.

Measured with a counter in the bootstrap body: 16 threads doing 10 rounds of
create/terminate each, 160 instance creations, 1 entry. A test binary that
tries to provoke it does not hang at 2, 4, 8 or 16 threads.

So this is a latent defect, not a live hang. It becomes a live hang if startup
is ever made re-entrant — which resetting active during termination would do,
and that is arguably a fix worth making on its own, since after
terminateInterpreter() the interpreter is torn down but isActive() still
reports true.

On fixing it

The obvious fix, mirroring r12435, is to drop the lock around the part that
needs the kernel:

lock.release();
InstanceBlock instance;
...
lock.reacquire();

ResourceSection already has release() and reacquire() for this.

That is not sufficient on its own. The caller also holds the lock:

// Interpreter::createInterpreterInstance
{
    ResourceSection lock;
    if (interpreterInstances == OREF_NULL)
    {
        startInterpreter(RUN_MODE, NULL);
    }
}

The resource lock is recursive, so an inner release() only drops the count
from 2 to 1 and the outer section still holds it across the kernel acquisition.
Any fix has to account for the nesting.

Two options, for whoever knows this code best to choose between:

1. Give one-time bootstrap its own lock, separate from the general purpose
   resource lock, held by both createInterpreterInstance's guard and
   startInterpreter. The order then becomes startup lock, kernel lock,
   resource lock, which is acyclic as long as nothing takes the startup lock
   while holding either of the other two, and today the only two acquisition
   sites are those. It has to stay recursive for the nested InstanceBlock
   re-entry. std::call_once looks attractive here but would break the
   if (!isActive()) restart path, which fires once per flag.

2. Split startInterpreter into the part needing no activity (memory
   subsystem, session queue, instances list) and the local server creation that
   does, and take the kernel lock first throughout. Truer to the documented
   invariant, but a real change to bootstrap ordering.

Related

Depends on the resource lock actually being created; see the separate report on
Interpreter::resourceLock never being locked. A test binary for concurrent
instance creation, which is what surfaced this, is attached there.