Deadlock in shared mem mutex when apps are killed

[marthtz]

Required information

Operating system:
Ubuntu 18.04 LTS

Compiler version:
GCC 7.4.0

Observed result or behaviour:
Deadlock in shared mem mutex when apps are killed

Expected result or behaviour:
No deadlock.

Conditions where it occurred / Performed steps:
Kill an app while some system is running

Related issues

• mutex owner died -> POPO__CHUNK_LOCKING_ERROR #2193
• POPO__CHUNK_LOCKING_ERROR on iox-roudi #2260

[elBoberido]

I've had an idea how this could be solved in an elegant way without a mutex but an atomic and semaphore instead.
Let's assume we have the following class

class DualAccessTransactionTray
{
    private:
    class AccessGuard;

    public:
    AccessGuard acquireRouDiAccess()
    {
        switch (m_accessToken.exchange(AccessToken::ROUDI, std::memory_order_acquire))
        {
        case AccessToken::NONE:
            break;
        case AccessToken::ROUDI:
            errorHandler(kBROKEN_INVARIANT);
            break;
        case AccessToken::APP:
            m_waitingLine.wait();
            break;
        }
        return AccessGuard{*this, AccessToken::ROUDI};
    }
    AccessGuard acquireAppAccess()
    {
        switch (m_accessToken.exchange(AccessToken::APP, std::memory_order_acquire))
        {
        case AccessToken::NONE:
            break;
        case AccessToken::ROUDI:
            m_waitingLine.wait();
            break;
        case AccessToken::APP:
            errorHandler(kBROKEN_INVARIANT);
            break;
        }
        return AccessGuard{*this, AccessToken::APP};
    }
    void cleanupAfterAppWalkedThePlank()
    {
        releaseAccess(AccessToken::APP);
        // just to be sure the memory is synchronized
        AccessToken expected = AccessToken::APP;
        m_accessToken.compare_exchange_strong(expected, AccessToken::NONE, std::memory_order_acq_rel);
    }

    private:
    enum class AccessToken
    {
        NONE,
        ROUDI,
        APP
    };

    void releaseAccess(AccessToken expected)
    {
        auto casSuccessful =
            m_accessToken.compare_exchange_strong(expected, AccessToken::NONE, std::memory_order_release);
        if (!casSuccessful)
        {
            if (expected == AccessToken::NONE)
            {
                errorHandler(kBROKEN_INVARIANT);
            }
            m_waitingLine.post();
        }
    }

    class AccessGuard
    {
        public:
        AccessGuard(DualAccessTransactionTray& transactionTray, AccessToken accessToken)
            : m_transactionTray(transactionTray)
            , m_accessToken(accessToken)
        {
        }
        ~AccessGuard()
        {
            m_transactionTray.releaseAccess(m_accessToken);
        }

        private:
        DualAccessTransactionTray& m_transactionTray;
        AccessToken m_accessToken{AccessToken::NONE};
    };

    std::atomic<AccessToken> m_accessToken{AccessToken::NONE};
    posix::Semaphore m_waitingLine;
};

So basically a futex but just for two threads. If the application terminates abnormally, the semaphore can be cleaned up contrary to the mutex which cannot be unlocked from an other thread than the one which acquired the lock.

This would be an example of a concurrent access from RouDi and the application

                              DualAccessTransactionTray
RouDi (discovery loop)               AccessFlag            App (publisher thread)
                                        NONE
acquireRouDiAccess                      NONE
-> previously NONE                      ROUDI
-> continue                             ROUDI
                                        ROUDI              acquireAppAccess
                                        APP                -> previously ROUDI but expected NONE
                                        APP                -> m_waitingLine.wait
-> add/remove queues                    APP
releaseRouDiAccess                      APP
-> CAS fails                            APP
-> m_waitingLine.post                   APP
                                        APP                -> continue (maybe check if to be destroyed flag is set)
                                        APP                -> push to all queues
                                        APP                releaseAppAccess
                                        NONE               -> CAS succeeds
                                        NONE               -> nothing to be done
                                        NONE
acquireRouDiAccess                      NONE
-> previously NONE                      ROUDI
-> continue                             ROUDI
                                        ROUDI              acquireAppAccess
                                        APP                -> previously ROUDI but expected NONE
                                        APP                -> m_waitingLine.wait
-> add/remove queues                    APP
releaseRouDiAccess                      APP
-> CAS fails                            APP
-> m_waitingLine.post                   APP
                                        APP                -> continue (maybe check if to be destroyed flag is set)
acquireRouDiAccess                      APP
-> previously APP but expected NONE     ROUDI
-> m_waitingLine.wait                   ROUDI
                                        ROUDI
                                        ROUDI              -> push to all queues
                                        ROUDI              releaseAppAccess
                                        ROUDI              -> CAS fails
                                        ROUDI              -> m_waitingLine.post
-> continue                             ROUDI
-> add/remove queues                    ROUDI
releaseRouDiAccess                      ROUDI
-> CAS succeeds                         NONE
-> nothing to be done                   NONE

@budrus @MatthiasKillat @elfenpiff what do you think?

[budrus]

@elBoberido so like a mutex that can be unlocked by someone else if this one is sure that the locking one walked the plank. But we could still have corrupted data structures which we maybe are not allowed to access. So we could detect and continue with the consequence that chunks are lost. Or we have to go for the extended UsedChunkList to have a more robust data structure.
Which is this todo I added some day

[elBoberido]

@budrus yes, the history vector might be corrupted. The simple approach would indeed be to leak chunks and print a warning if RouDi detects that the lock was hold by the application. A more sophisticated solution would be a HistoryRingBuffer with the same mechanism like the UsedChunkList. In my view, extending the UsedChunkList makes no sense since it has a totally different API. I'd rather go for a small HistoryRingBuffer which does one thing but well.

[mossmaurice]

@marthtz @sculpordwarf @bishibashiB @shankar-in
What is the status here? Will you re-open the PR to solve this bug?

cc @CarolinaGGB

[shankar-in]

@mossmaurice @marthtz is on vacation. he will be back in 2 weeks.