Deadlock when deleting a non-empty RequestList with POSIX_THREADS and NO_FAST_MUTEXES

SNMP++/AGENT++
1

Hey :slight_smile:

I suspect, that I’ve stumbled upon a bug in Agent++, but I may be wrong, and I’m simply using the Agent++ API incorrectly.

First of all, the POSIX_THREADS macro is defined.
Thus, a pthread_mutex_t monitor exists as a member in Synchronized.

Furthermore, the NO_FAST_MUTEXES macro is defined.
Thus, the LockRequest and LockQueue classes are used - especially in RequestList::lock_request and RequestList::unlock_request.

Note, that RequestList::receive calls RequestList::lock_request.
Note, that Mib::process_request calls RequestList::unlock_request.

Therefore, RequestList::receive and Mib::process_request lock/unlock mutexes of the Requests via the LockQueue thread (LockQueue::run).
This seems to be intended.

If RequestList::receive returns a Request, that Request is locked via the LockQueue thread, and becomes an element of the RequestList.
This also seems to be intended.

If Mib::process_request is called, that Request is unlocked via the LockQueue thread, and removed from the RequestList.
This also seems to be intended.

Consider a code snippet, that looks something like the following (obviously in a very stripped-down version):

while (!stop) {
    snmpRequest = snmpRequestList->receive(2);
    if (snmpRequest && processSnmpRequests)
        snmpMib->process_request(snmpRequest);
    else
        snmpMib->cleanup();
}

delete snmpRequestList;

Note, that if processSnmpRequests is set to false, we receive but don’t process the Requests.
When stop is set to true, delete snmpRequestList calls ~RequestList.

Assume, that snmpRequestList contains at least one unprocessed Request, when ~RequestList is called.
Since Mib::process_request wasn’t called, RequestList::unlock_request wasn’t called, thus the LockQueue thread never unlocks the mutexes of the Requests.
Via ~RequestList we get to ~Request (for each Request in the RequestList), and thus to ~Synchronized.
The relevant part of that destructor:

Synchronized::~Synchronized()
{
#ifdef POSIX_THREADS
    int result;

    result = pthread_cond_destroy(&cond);
    if (result) {
        LOG_BEGIN(loggerModuleName, ERROR_LOG | 2);
        LOG("Synchronized cond_destroy failed with (result)(ptr)");
        LOG(result);
        LOG((unsigned long)this);
        LOG_END;
    }
    result = pthread_mutex_destroy(&monitor);
#ifdef NO_FAST_MUTEXES
        if( result == EBUSY ) {
            // wait for other threads ...
            if( EBUSY == pthread_mutex_trylock(&monitor) )
                pthread_mutex_lock(&monitor); // another thread owns the mutex, let's wait ...

Note, that ~Synchronized is NOT called in the LockQueue thread, but in the thread, which calls ~RequestList, thus we get a deadlock in the line with the “another thread owns the mutex, let’s wait” comment.

This is not just a theoretical problem - we ran into exactly this, and I discovered it via inspecting a core dump and reading the source code of Agent++.

If this is actually a bug in Agent++, and you plan to fix it, what can we do in the meantime, as a sensible workaround on our side?
And if this is not a bug in Agent++, but intended behavior instead, what are we doing wrong?

If you need more information, please let me know.
I’m not a regular user of Agent++, in fact I’ve never used it before.
I was provided a deadlock scenario, and this is what I found.

Thanks in advance :slight_smile:

2

Your analysis is correct. There is a deadlock in this case. To avoid this issue, let the all the requests in the RequestList be processed before shutting the RequestList down. Thus, when stopping, do not accept any new Request while processing all remaining pending.

To avoid memory violation crashes, the Requests are intentionally not unlocked before being deleted. This was needed to protect the AgentX master agent, against instrumentation errors.
I think it is time to do this in some other way.

Such a fix is rather complex (touches threads.cpp and request.cpp) and implements in ~Synchronized a timeout crafted unlock/destroy sequence to make sure that a pending lock is unlocked before the object is destroyed. I have implemented it for version 5.0.0 of AGENT++.
A back-port for 4.x will be provided as well, but it could happen that it will be released after 5.0.0.

3

Thank you very much for your fast response :slight_smile:

To avoid this issue, let the all the requests in the RequestList be processed before shutting the RequestList down. Thus, when stopping, do not accept any new Request while processing all remaining pending.

That should be easy enough, however I’m not sure how to actually do it :wink:
Obviously the “stop accepting new Requests” part is easy, but the “process all remaining pending Requests” part not so much.

I’d expect there to be a way to iterate over the Requests in the RequestList, so that I can process them, but I don’t see a (straight-forward) way.

Looking at the public functions of RequestList, I see get_request (which needs a transaction id) and find_request_on_id (which needs a request id), so that doesn’t seem like a straight-forward way to iterate over the RequestList.
I mean, sure, I could call get_request_id on each Request that I receive but don’t process, store those ids, and use them to fetch the Requests from the RequestList when I want to process them, but that seems… strange?

Looking at the public functions of Mib, I don’t see any, that could be useful here.

Am I missing something? What did you have in mind, when you wrote “process all remaining pending Requests”? Maybe it’s easiest, if you take my very minimal code snippet from the first post, and just show me?

Furthermore, regarding the word “process”, which obviously means Mib::process_request: What about RequestList::unlock_request followed by RequestList::remove_request instead? Wouldn’t that work as well, and actually discard the request? Or would that lead to weird side effects?

And lastly, based on my very minimal code snippet from the first post, would the following be fine?

while (!stop) {
    if (processSnmpRequests)
        snmpRequest = snmpRequestList->receive(2);
    else {
        snmpRequest = nullptr;
        // something like sleeping or whatever you fancy
    }

    if (snmpRequest)
        snmpMib->process_request(snmpRequest);
    else
        snmpMib->cleanup();
}

delete snmpRequestList;

I am explicitly asking, because I want to be sure about one thing: Is it guaranteed, that if RequestList::receive returns a Request, only that Request is stored in the RequestList? That means, is it impossible, that RequestList::receive returns a Request, but actually stores for instance two Requests, because two have been available for receiving?

Thanks in advance, again :slight_smile:

4

Yes, receive returns (and store) exactly one Request (or no Request on timeout).

5

Thanks for your answer regarding the last question - that already helps a lot :slight_smile:

But what about iterating over RequestList to get the Requests, and what about using RequestList::unlock_request followed by RequestList::remove_request instead of Mib::process_request?