This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author jeffr@livedata.com
Recipients jeffr@livedata.com, paul.moore, steve.dower, tim.golden, zach.ware
Date 2019-01-05.03:25:25
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1546658725.61.0.827924851828.issue35662@roundup.psfhosted.org>
In-reply-to
Content
Python 3.x defaults to using emulated condition variables on Windows.  I tested a build with native Windows condition variables (#define _PY_EMULATED_WIN_CV 0), and found a serious issue.

The problem is in condvar.h, in this routine:


/* This implementation makes no distinction about timeouts.  Signal
 * 2 to indicate that we don't know.
 */
Py_LOCAL_INLINE(int)
PyCOND_TIMEDWAIT(PyCOND_T *cv, PyMUTEX_T *cs, long long us)
{
    return SleepConditionVariableSRW(cv, cs, (DWORD)(us/1000), 0) ? 2 : -1;
}


The issue is that `SleepConditionVariableSRW` returns FALSE in the timeout case.  PyCOND_TIMEDWAIT returns -1 in that case. 

But... COND_TIMED_WAIT, which calls PyCOND_TIMEDWAIT, in ceval_gil.h, fatals(!) on a negative return value


#define COND_TIMED_WAIT(cond, mut, microseconds, timeout_result) \
    { \
        int r = PyCOND_TIMEDWAIT(&(cond), &(mut), (microseconds)); \
        if (r < 0) \
            Py_FatalError("PyCOND_WAIT(" #cond ") failed"); \



I'd like to suggest that we use the documented behavior of the OS API call already being used (SleepConditionVariableSRW https://docs.microsoft.com/en-us/windows/desktop/api/synchapi/nf-synchapi-sleepconditionvariablesrw) and return 0 on regular success and 1 on timeout, like in the _POSIX_THREADS case.  

"""
Return Value
If the function succeeds, the return value is nonzero.

If the function fails, the return value is zero. To get extended error information, call GetLastError.

If the timeout expires the function returns FALSE and GetLastError returns ERROR_TIMEOUT.
"""

I've tested this rewrite -- the main difference is in the FALSE case, check GetLastError() for ERROR_TIMEOUT and then *do not* treat this as a fatal error.
 


/*
 * PyCOND_TIMEDWAIT, in addition to returning negative on error,
 * thus returns 0 on regular success, 1 on timeout
*/
Py_LOCAL_INLINE(int)
PyCOND_TIMEDWAIT(PyCOND_T *cv, PyMUTEX_T *cs, long long us)
{
    BOOL result = SleepConditionVariableSRW(cv, cs, (DWORD)(us / 1000), 0);

    if (result)
        return 0;

    if (GetLastError() == ERROR_TIMEOUT)
        return 1;

    return -1;
}


I've attached the test I ran to reproduce the crash.
History
Date User Action Args
2019-01-05 03:25:28jeffr@livedata.comsetrecipients: + jeffr@livedata.com, paul.moore, tim.golden, zach.ware, steve.dower
2019-01-05 03:25:25jeffr@livedata.comsetmessageid: <1546658725.61.0.827924851828.issue35662@roundup.psfhosted.org>
2019-01-05 03:25:25jeffr@livedata.comlinkissue35662 messages
2019-01-05 03:25:25jeffr@livedata.comcreate