Commit 9d299253 authored by Niklas Hambüchen's avatar Niklas Hambüchen Committed by Ben Gamari
Browse files

base: fdReady(): Return only after sycall returns after `msecs` have passed

Reviewers: bgamari, austin, hvr, dfeuer

Reviewed By: dfeuer

Subscribers: syd, dfeuer, rwbarton, thomie

Differential Revision: https://phabricator.haskell.org/D4012
parent 430d1f6a
...@@ -33,6 +33,10 @@ ...@@ -33,6 +33,10 @@
/* /*
* Returns a timeout suitable to be passed into poll(). * Returns a timeout suitable to be passed into poll().
* *
* If `remaining` contains a fractional milliseconds part that cannot be passed
* to poll(), this function will return the next larger value that can, so
* that the timeout passed to poll() would always be `>= remaining`.
*
* If `infinite`, `remaining` is ignored. * If `infinite`, `remaining` is ignored.
*/ */
static inline static inline
...@@ -45,7 +49,11 @@ compute_poll_timeout(bool infinite, Time remaining) ...@@ -45,7 +49,11 @@ compute_poll_timeout(bool infinite, Time remaining)
if (remaining > MSToTime(INT_MAX)) return INT_MAX; if (remaining > MSToTime(INT_MAX)) return INT_MAX;
return TimeToMS(remaining); int remaining_ms = TimeToMS(remaining);
if (remaining != MSToTime(remaining_ms)) return remaining_ms + 1;
return remaining_ms;
} }
#if defined(_WIN32) #if defined(_WIN32)
...@@ -88,6 +96,11 @@ compute_windows_select_timeout(bool infinite, Time remaining, ...@@ -88,6 +96,11 @@ compute_windows_select_timeout(bool infinite, Time remaining,
* Returns a timeout suitable to be passed into WaitForSingleObject() on * Returns a timeout suitable to be passed into WaitForSingleObject() on
* Windows. * Windows.
* *
* If `remaining` contains a fractional milliseconds part that cannot be passed
* to WaitForSingleObject(), this function will return the next larger value
* that can, so that the timeout passed to WaitForSingleObject() would
* always be `>= remaining`.
*
* If `infinite`, `remaining` is ignored. * If `infinite`, `remaining` is ignored.
*/ */
static inline static inline
...@@ -112,7 +125,11 @@ compute_WaitForSingleObject_timeout(bool infinite, Time remaining) ...@@ -112,7 +125,11 @@ compute_WaitForSingleObject_timeout(bool infinite, Time remaining)
if (remaining >= MSToTime(INFINITE)) return INFINITE - 1; if (remaining >= MSToTime(INFINITE)) return INFINITE - 1;
return (DWORD) TimeToMS(remaining); DWORD remaining_ms = TimeToMS(remaining);
if (remaining != MSToTime(remaining_ms)) return remaining_ms + 1;
return remaining_ms;
} }
#endif #endif
...@@ -150,6 +167,55 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -150,6 +167,55 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
Time remaining = MSToTime(msecs); Time remaining = MSToTime(msecs);
// Note [Guaranteed syscall time spent]
//
// The implementation ensures that if fdReady() is called with N `msecs`,
// it will not return before an FD-polling syscall *returns*
// with `endTime` having passed.
//
// Consider the following scenario:
//
// 1 int ready = poll(..., msecs);
// 2 if (EINTR happened) {
// 3 Time now = getProcessElapsedTime();
// 4 if (now >= endTime) return 0;
// 5 remaining = endTime - now;
// 6 }
//
// If `msecs` is 5 seconds, but in line 1 poll() returns with EINTR after
// only 10 ms due to a signal, and if at line 2 the machine starts
// swapping for 10 seconds, then line 4 will return that there's no
// data ready, even though by now there may be data ready now, and we have
// not actually checked after up to `msecs` = 5 seconds whether there's
// data ready as promised.
//
// Why is this important?
// Assume you call the pizza man to bring you a pizza.
// You arrange that you won't pay if he doesn't ring your doorbell
// in under 10 minutes delivery time.
// At 9:58 fdReady() gets woken by EINTR and then your computer swaps
// for 3 seconds.
// At 9:59 the pizza man rings.
// At 10:01 fdReady() will incorrectly tell you that the pizza man hasn't
// rung within 10 minutes, when in fact he has.
//
// If the pizza man is some watchdog service or dead man's switch program,
// this is problematic.
//
// To avoid it, we ensure that in the timeline diagram:
//
// endTime
// |
// time ----+----------+-------+---->
// | |
// syscall starts syscall returns
//
// the "syscall returns" event is always >= the "endTime" time.
//
// In the code this means that we never check whether to `return 0`
// after a `Time now = getProcessElapsedTime();`, and instead always
// let the branch marked [we waited the full msecs] handle that case.
#if !defined(_WIN32) #if !defined(_WIN32)
struct pollfd fds[1]; struct pollfd fds[1];
...@@ -174,7 +240,7 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -174,7 +240,7 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
return 1; // FD has new data return 1; // FD has new data
if (res == 0 && !infinite && remaining <= MSToTime(INT_MAX)) if (res == 0 && !infinite && remaining <= MSToTime(INT_MAX))
return 0; // FD has no new data and we've waited the full msecs return 0; // FD has no new data and [we waited the full msecs]
// Non-exit cases // Non-exit cases
CHECK( ( res < 0 && errno == EINTR ) || // EINTR happened CHECK( ( res < 0 && errno == EINTR ) || // EINTR happened
...@@ -184,7 +250,6 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -184,7 +250,6 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
if (!infinite) { if (!infinite) {
Time now = getProcessElapsedTime(); Time now = getProcessElapsedTime();
if (now >= endTime) return 0;
remaining = endTime - now; remaining = endTime - now;
} }
} }
...@@ -231,7 +296,7 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -231,7 +296,7 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
return 1; // FD has new data return 1; // FD has new data
if (res == 0 && !infinite && remaining <= MSToTime(INT_MAX)) if (res == 0 && !infinite && remaining <= MSToTime(INT_MAX))
return 0; // FD has no new data and we've waited the full msecs return 0; // FD has no new data and [we waited the full msecs]
// Non-exit cases // Non-exit cases
CHECK( ( res < 0 && errno == EINTR ) || // EINTR happened CHECK( ( res < 0 && errno == EINTR ) || // EINTR happened
...@@ -241,7 +306,6 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -241,7 +306,6 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
if (!infinite) { if (!infinite) {
Time now = getProcessElapsedTime(); Time now = getProcessElapsedTime();
if (now >= endTime) return 0;
remaining = endTime - now; remaining = endTime - now;
} }
} }
...@@ -279,7 +343,7 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -279,7 +343,7 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
// compute_WaitForSingleObject_timeout(), // compute_WaitForSingleObject_timeout(),
// so that's 1 ms too little. Wait again then. // so that's 1 ms too little. Wait again then.
if (!infinite && remaining < MSToTime(INFINITE)) if (!infinite && remaining < MSToTime(INFINITE))
return 0; return 0; // real complete or [we waited the full msecs]
goto waitAgain; goto waitAgain;
case WAIT_OBJECT_0: break; case WAIT_OBJECT_0: break;
default: /* WAIT_FAILED */ maperrno(); return -1; default: /* WAIT_FAILED */ maperrno(); return -1;
...@@ -346,6 +410,11 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -346,6 +410,11 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
// //
// PeekNamedPipe() does not block, so if it returns that // PeekNamedPipe() does not block, so if it returns that
// there is no new data, we have to sleep and try again. // there is no new data, we have to sleep and try again.
// Because PeekNamedPipe() doesn't block, we have to track
// manually whether we've called it one more time after `endTime`
// to fulfill Note [Guaranteed syscall time spent].
bool endTimeReached = false;
while (avail == 0) { while (avail == 0) {
BOOL success = PeekNamedPipe( hFile, NULL, 0, NULL, &avail, NULL ); BOOL success = PeekNamedPipe( hFile, NULL, 0, NULL, &avail, NULL );
if (success) { if (success) {
...@@ -358,8 +427,9 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -358,8 +427,9 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
} else if (msecs == 0) { } else if (msecs == 0) {
return 0; return 0;
} else { } else {
if (endTimeReached) return 0; // [we waited the full msecs]
Time now = getProcessElapsedTime(); Time now = getProcessElapsedTime();
if (now >= endTime) return 0; if (now >= endTime) endTimeReached = true;
Sleep(1); // 1 millisecond (smallest possible time on Windows) Sleep(1); // 1 millisecond (smallest possible time on Windows)
continue; continue;
} }
...@@ -392,7 +462,7 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -392,7 +462,7 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
// compute_WaitForSingleObject_timeout(), // compute_WaitForSingleObject_timeout(),
// so that's 1 ms too little. Wait again then. // so that's 1 ms too little. Wait again then.
if (!infinite && remaining < MSToTime(INFINITE)) if (!infinite && remaining < MSToTime(INFINITE))
return 0; return 0; // real complete or [we waited the full msecs]
break; break;
case WAIT_OBJECT_0: return 1; case WAIT_OBJECT_0: return 1;
default: /* WAIT_FAILED */ maperrno(); return -1; default: /* WAIT_FAILED */ maperrno(); return -1;
...@@ -401,7 +471,6 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock) ...@@ -401,7 +471,6 @@ fdReady(int fd, bool write, int64_t msecs, bool isSock)
// EINTR or a >(INFINITE - 1) timeout completed // EINTR or a >(INFINITE - 1) timeout completed
if (!infinite) { if (!infinite) {
Time now = getProcessElapsedTime(); Time now = getProcessElapsedTime();
if (now >= endTime) return 0;
remaining = endTime - now; remaining = endTime - now;
} }
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment