New NamePipe Deadlocks

I think there’s a deadlock been introduced in to NamedPipe with the recent-ish additions of the read/write locks?

If I have one thread waiting on a read (or write) with a timeout of -1 here:

int NamedPipe::read (void* destBuffer, int maxBytesToRead, int timeOutMilliseconds)
{
    ScopedReadLock sl (lock);
    return pimpl != nullptr ? pimpl->read (static_cast<char*> (destBuffer), maxBytesToRead, timeOutMilliseconds) : -1;
}

and another thread trying to close the pipe here:

void NamedPipe::close()
{
    ScopedWriteLock sl (lock);

    if (pimpl != nullptr)
    {
        pimpl->stopReadOperation = true;

        const char buffer[] { 0 };
        const auto done = ::write (pimpl->pipeIn.get(), buffer, numElementsInArray (buffer));
        ignoreUnused (done);
    }

    pimpl.reset();
}

The stopReadOperation can never be set due to the ScopedWriteLock so the read call never exits.

I think what is needed is an initial ScopedReadLock to stop the pipe and then try and take the write lock. Does that sound sensible?

Something like:

void NamedPipe::close()
{
    {
        ScopedReadLock sl (lock);

        if (pimpl != nullptr)
            pimpl->stopReadOperation = true;
    }

    ScopedWriteLock sl (lock);

    if (pimpl != nullptr)
    {
        const char buffer[] { 0 };
        const auto done = ::write (pimpl->pipeIn.get(), buffer, numElementsInArray (buffer));
        ignoreUnused (done);
    }

    pimpl.reset();
}

I realise this is only on posix but the same pattern already seems to be implemented on win32:

void NamedPipe::close()
{
    {
        ScopedReadLock sl (lock);

        if (pimpl != nullptr)
        {
            pimpl->shouldStop = true;
            SetEvent (pimpl->cancelEvent);
        }
    }

    {
        ScopedWriteLock sl (lock);
        pimpl.reset();
    }
}
1 Like

Thanks for reporting, I think your suggested fix is the correct one. I’ve added that here:

Cheers :+1:

Were there any changes to windows NamedPipes recently? I’m not see any in git, but I’m seeing full system hangs shutting down the pipe I didn’t see before.

No, I don’t think so. Other than the change above, it looks like the last set of NamedPipe changes were around November 2020.