[Freeswitch-dev] Why is _thread_cond_timedwait written the way it is?

[Peter Olsson]

It seems this is more of a question for the APR developers. The basic idea for APR is to make a lib that works the same way on multiple OS'es, so I guess it might look strange in some cases. However, do you really have a problem that you think might cause issues in FS?

/Peter

11 apr 2012 kl. 07:53 skrev "Richard Lewis Haggard" <haggard at msn.com<mailto:haggard at msn.com>>:

I was looking at _thread_cond_timedwait  in thread_code. There is a section of the code that I do not understand the purpose of. Could someone please explain to me what the purpose of the other part of the code is?

Before the code enters a loop it serializes access to the passed in cond structure pointer, increments a counter and makes a local copy of a value before releasing the cond again.


    EnterCriticalSection(&cond->csection);

    cond->num_waiting++;

    generation = cond->generation;

    LeaveCriticalSection(&cond->csection);

What is the purpose of ‘num_waiting’ and ‘generation’?

The next part looks odd to me. Why does the code unlock the mutex when what is desired is to lock it?


    apr_thread_mutex_unlock(mutex);


The code then enters a do { } while (true); loop.


    do {

The first thing it does is to wait for access to a semaphore. This is a limited time wait and may time out before the event is encountered. The result of this wait is checked later.


        res = WaitForSingleObject(cond->semaphore, timeout_ms);

The ‘cond’ structure is going to be modified again and the code needs to have exclusive access.


        EnterCriticalSection(&cond->csection);


I do not know what the purpose of this is. Can someone explain it to me? What is the purpose of this particular block?


        if (cond->num_wake) {

            if (cond->generation != generation) {

                cond->num_wake--;

                cond->num_waiting--;

                rv = APR_SUCCESS;

                break;

            } else {

                wake = 1;

            }

        }

        else if (res != WAIT_OBJECT_0) {

            cond->num_waiting--;

            rv = APR_TIMEUP;

            break;

        }

If neither ‘break’ executes then the code releases the critical section. The code would be less obtuse (and smaller) if the two ‘break’ functions were removed. That would save having duplicate LeaveCriticalSection() functions. In any case, the code releases the critical section.


        LeaveCriticalSection(&cond->csection);

What is the purpose of the next bock? It only executes if the semaphone acquisition was successful but the ‘cond->generation’ has changed while attempting to determine if it is OK to execute the mutex lock.


        if (wake) {

            wake = 0;

            ReleaseSemaphore(cond->semaphore, 1, NULL);

        }
Note: If the two ‘break’ statements were removed from above and the following lines added. At this point in the code the critical section would have been released so it would not be necessary to explicitly do so outside of the forever loop.

        else {

            break;

        }

Under what conditions can ‘cond->generation’ change?

The bottom of the loop is encountered.


    } while (1);

To get here one of the two ‘break’ statements must have been hit. The critical section is still locked and must be released. Note: if my suggested changes were implemented then it would not be necessary to have this line:


    LeaveCriticalSection(&cond->csection);


And now, finally, the mutex gets locked.


    apr_thread_mutex_lock(mutex);


==============================
Richard Lewis Haggard


!DSPAM:4f8519dc32766378388994!
_________________________________________________________________________
Professional FreeSWITCH Consulting Services:
consulting at freeswitch.org<mailto:consulting at freeswitch.org>
http://www.freeswitchsolutions.com




Official FreeSWITCH Sites
http://www.freeswitch.org
http://wiki.freeswitch.org
http://www.cluecon.com

FreeSWITCH-dev mailing list
FreeSWITCH-dev at lists.freeswitch.org<mailto:FreeSWITCH-dev at lists.freeswitch.org>
http://lists.freeswitch.org/mailman/listinfo/freeswitch-dev
UNSUBSCRIBE:http://lists.freeswitch.org/mailman/options/freeswitch-dev
http://www.freeswitch.org


!DSPAM:4f8519dc32766378388994!