[issue15038] Optimize python Locks on Windows

[Richard Oudkerk]

Richard Oudkerk <shibturn at gmail.com> added the comment:

Py_LOCAL_INLINE(int)
_PyCOND_WAIT_MS(PyCOND_T *cv, PyMUTEX_T *cs, DWORD ms)
{
    DWORD wait;
    cv->waiting++;
    PyMUTEX_UNLOCK(cs);
    /* "lost wakeup bug" would occur if the caller were interrupted here,
     * but we are safe because we are using a semaphore wich has an internal
     * count.
     */
    wait = WaitForSingleObject(cv->sem, ms);
    PyMUTEX_LOCK(cs);
    if (wait != WAIT_OBJECT_0)
        --cv->waiting;
        /* Here we have a benign race condition with PyCOND_SIGNAL.
         * When failure occurs or timeout, it is possible that
         * PyCOND_SIGNAL also decrements this value
         * and signals releases the mutex.  This is benign because it
         * just means an extra spurious wakeup for a waiting thread.
         */
    ...

Are you really sure this race is benign?

If cv->waiting gets double decremented then it can become negative.  PyCOND_SIGNAL() is defined as

Py_LOCAL_INLINE(int)
PyCOND_SIGNAL(PyCOND_T *cv)
{
    if (cv->waiting) {
        cv->waiting--;
        return ReleaseSemaphore(cv->sem, 1, NULL) ? 0 : -1;
    }
    return 0;
}

While cv->waiting is negative, each call of PyCOND_SIGNAL() decrements cv->waiting, and increments the semaphore, while each call of PyCOND_WAIT() will increment cv->waiting and decrement the semaphore.

So if calls of PyCOND_SIGNAL() outnumber calls of PyCOND_WAIT() then we can have cv->waiting becoming very negative and the semaphore overflowing.

Maybe just changing the test in PyCOND_SIGNAL() to

    if (cv->waiting > 0) {

would be enough, but I am not convinced.

----------
nosy: +sbt

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue15038>
_______________________________________