meganz / mingw-std-threads

Standard threads implementation currently still missing on MinGW GCC on Windows
BSD 2-Clause "Simplified" License
439 stars 137 forks source link

Fix sleep_for with large/negative values. #45

Closed ghost closed 5 years ago

ghost commented 5 years ago

Win32 Sleep() takes an unsigned int for milliseconds, so this function would break if you passed it a negative value (calling sleep_until with a time in the past) or a duration longer than 49.7 days.

nmcclatchey commented 5 years ago

This needs a revision. Your intent is foiled by how Sleep() handles the special value INFINITE. As formulated, your implementation will sleep indefinitely whenever the number of milliseconds is sufficiently large.

Instead of using std::numeric_limits<DWORD>::max(), I recommend using the value std::numeric_limits<DWORD>::max() - 1 and accompanying it with the following static assertion:

static_assert(std::numeric_limits<DWORD>::max() <= static_cast<DWORD>(INFINITE), "As currently implemented, `sleep_for` and `sleep_until` may sleep indefinitely. This behavior can be triggered when the number of milliseconds equals the Windows API constant INFINITE.");

Other than the special-case handling, commit 187d94d will behave as expected. In particular, the type conversions are safe. DWORD is defined to be a 32-bit unsigned integer, and std::chrono::milliseconds uses at least a 45-bit signed integer, so conversion from DWORD to milliseconds is well-defined and will behave as expected. My personal preference is to add static assertions to verify this:

static_assert(std::numeric_limits<DWORD>::max() <= std::numeric_limits<std::chrono::milliseconds::rep>::max(), "Casting the maximum DWORD value to milliseconds::rep is undefined behavior because DWORD is larger than expected.");

But it is unnecessary in this case.

alxvasilev commented 5 years ago

I have updated the fix to cap the size of single sleep to INFINITE - 1. This assumes INFINITE is >= 1, but I think this is a safe assumption, as zero and 1 are valid values for the sleep interval, and DWORD is by definition unsigned (or maybe we should have a static assert for that). Please let me know if this looks ok for you guys.

nmcclatchey commented 5 years ago

This looks good to me.

alxvasilev commented 5 years ago

Okay, merged. Thanks guys!