base: fdReady(): Assert that `msecs` is >= 0
AbandonedPublic

Authored by nh2 on Sep 14 2017, 8:52 AM.

Details

Reviewers
bgamari
austin
hvr
nh2 created this revision.Sep 14 2017, 8:52 AM
bgamari requested changes to this revision.Sep 14 2017, 9:06 AM
bgamari added inline comments.
libraries/base/cbits/inputReady.c
45

GHC has its own mechanisms for this sort of thing which integrate into the RTS's error handling framework. See RtsMessages.c, in particular replace abort with barf.

This revision now requires changes to proceed.Sep 14 2017, 9:06 AM
nh2 updated this revision to Diff 13886.Sep 14 2017, 9:38 AM
nh2 edited edge metadata.

Address comment, switching to barf()

nh2 marked an inline comment as done.Sep 14 2017, 9:39 AM
bgamari requested changes to this revision.Sep 14 2017, 9:48 AM
bgamari added inline comments.
libraries/base/cbits/inputReady.c
44

Note that barf doesn't take an fd as its first argument.

This revision now requires changes to proceed.Sep 14 2017, 9:48 AM
nh2 updated this revision to Diff 13888.Sep 14 2017, 10:28 AM
nh2 edited edge metadata.

Fix barf() invocation

nh2 marked an inline comment as done.Sep 14 2017, 10:28 AM
bgamari accepted this revision.Sep 14 2017, 11:18 AM

Looks good to me. Thanks!

This revision is now accepted and ready to land.Sep 14 2017, 11:18 AM
NicolasT added inline comments.
libraries/base/cbits/inputReady.c
44

The docstring of this procedure says

within 'msecs' milliseconds (or indefinitely if 'msecs' is negative)

so either the docstring is wrong now, or this code is?

nh2 planned changes to this revision.Sep 16 2017, 5:24 PM
nh2 added inline comments.
libraries/base/cbits/inputReady.c
44

Good point, I should check if any callers do that.

poll() supports -1 == infinite out of the box; I just looked up what happens on Windows, and as usual the results are somewat disturbing:

The docs for WaitForSingleObject https://msdn.microsoft.com/en-us/library/windows/desktop/ms687032(v=vs.85).aspx say:

If dwMilliseconds is INFINITE, the function will return only when the object is signaled.

Now, what is INFINITE given that it's a DWORD (32-bit unsigned integer)?

This books says it https://books.google.de/books?id=r5tCAwAAQBAJ&pg=PT517&lpg=PT517&dq=INFINITE+dword&source=bl&ots=PSZD9dtkM0&sig=XmJQFp8fSZyg4NW4pQi4Z3rZ8bk&hl=en&sa=X&redir_esc=y#v=onepage&q=INFINITE%20dword&f=false:

It's 0xFFFFFFFF! Or -1. So it would work, and probably did before.

But that seems a pretty dangerous value for INFINITE, as that is just 49 days! In other words, I think it was a bug before: If you waited for 49 days and a bit, it would actualy wait forever.

Also, I think that it means even the current GHC code is wrong, as there are multiple checks for like if (msecs > 0) { /* wait more */ } else { return -1 }, and those should be waiting more also for the msecs == -1 case.

nh2 abandoned this revision.Sep 22 2017, 12:51 PM

Abandoning this in favour of https://phabricator.haskell.org/D4011, which fixes the found issues.