KQueue: Fix write notification requests being ignored when read notifications are requested, too (#13903)
ClosedPublic

Authored by waldheinz on Jun 30 2017, 8:22 AM.

Details

Summary

Signed-off-by: Matthias Treydte <mt@waldheinz.de>

KQueue: Drop Bits/FiniteBits instances for Filter as they are really constants whose bits should not be fiddled with

Signed-off-by: Matthias Treydte <mt@waldheinz.de>

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
waldheinz created this revision.Jun 30 2017, 8:22 AM
bgamari requested changes to this revision.Jun 30 2017, 9:53 AM

This looks good to me. Thanks @waldheinz! One request inline.

Also, can we construct a testcase to catch this issue? Perhaps a pair of programs communicating over a FIFO? The IO manager could really use more tests.

libraries/base/GHC/Event/KQueue.hsc
89–90

Let's drop the bangs here and below as they aren't doing anything but forcing the head of the list.

This revision now requires changes to proceed.Jun 30 2017, 9:53 AM

@bgamari I can try to construct a test case. I did not yet have a look at existing tests for the IO manager, but I imagine something that targets the GHC.Event.Manager directly instead of this specific backend. This test would obviously only fail on BSD OSes, but could exercise the other backends as well. Also I'd rather go for a single process talking via FIFOs to itself instead of multiple processes (if I'm able to come up with something that triggers this issue). Should I add the test here or as a separate issue?

waldheinz updated this revision to Diff 12985.Jun 30 2017, 10:25 AM
  • KQueue: Fix write notification requests being ignored when read notifications are requested, too (Trac #13903)
  • KQueue: Drop Bits/FiniteBits instances for Filter as they are really constants whose bits should not be fiddled with
  • KQueue: Cleanup / remove useless bangs in modifyFd~ functions

@bgamari I can try to construct a test case. I did not yet have a look at existing tests for the IO manager, but I imagine something that targets the GHC.Event.Manager directly instead of this specific backend. This test would obviously only fail on BSD OSes, but could exercise the other backends as well. Also I'd rather go for a single process talking via FIFOs to itself instead of multiple processes (if I'm able to come up with something that triggers this issue).

I agree; we should test against the IO manager generally, not particular backends.

Should I add the test here or as a separate issue?

Either works for me. Keeping the test in a separate patch has the advantage that we can apply it first and ensure that it does indeed fail without your fix.

Any progress on a test? Let me know if you get stuck!

waldheinz marked an inline comment as done.Jul 8 2017, 2:59 AM

Sorry, I did not get around working on the test this week. Next week looks better, I'll report back.

But something else just occured to me -- I'm not sure if this is the right place to discuss this, but I'd like to point it out:

The modifyFd function checks

  • if nevt is mempty, and if so unregisters everything from oevt,
  • otherwise registers everything from nevt

Assuming that oevt means "old events" and nevt means "new events", this will not work if the event manager tries to switch a fd from "read" to "write" (or vice versa) notifications in one step, as nevt /= mempty and we'll end up with both, read and write notifications enabled. This doesn't look good to me, but the EPoll backend does the same thing:

https://github.com/waldheinz/ghc/blob/master/libraries/base/GHC/Event/EPoll.hsc#L89

Is the contract for modifyFd that this never happens or is this intended behaviour?

waldheinz planned changes to this revision.Jul 8 2017, 3:04 AM

Nevermind, I just realised that the EPoll backend does *not* do the same thing. I souldn't try to think before coffee, never works. So this patch is incomplete and should not be merged at this point. Sorry.

Nevermind, I just realised that the EPoll backend does *not* do the same thing. I souldn't try to think before coffee, never works. So this patch is incomplete and should not be merged at this point. Sorry.

Quite alright. Thanks for your effort and let me know if you get stuck.

How is this going, @waldheinz?

Any updates on this @waldheinz?

austin resigned from this revision.Nov 9 2017, 5:35 PM

I'm the author of KQueue and the maintainer of the network package.
I noticed this issue from https://github.com/haskell/network/issues/262
I confirmed that this patch fixes the issue.
Yes, this is my bad.
Thank you for fixing!

This patch should be merged into GHC 8.4 even without test code.

On macOS, oneshot feature is disabled because *parallel* building (make -j) of GHC sometime stacks with Mio on macOS.
I bet that the stack is due to this bug.
So, let's remove two #ifdef for "darwin_HOST_OS" and see what happens for parallel bulding.

Sorry for not commenting for so long. I just reviewed what I did here again and it seems this:

Assuming that oevt means "old events" and nevt means "new events", this will not work if the event manager tries to switch a fd from "read" to "write" (or vice versa) notifications in one step, as nevt /= mempty and we'll end up with both, read and write notifications enabled.

is plain wrong. man kqueue says about EV_ADD:

Re-adding an existing event will modify the parameters of the original event, and not result in a duplicate entry.

This is exactly what the modifyFd function as proposed in this patch does: we either remove an event or add/update an event, and because of how the kqueue API works, the latter are handled both by the `othewise´ case.

TL;DR: to my knowledge this patch fixes the issue. please merge. if i'll ever come around to write a test for this, i'll create a separate differential/merge request.

waldheinz requested review of this revision.Jan 2 2018, 4:49 AM
bgamari accepted this revision.Jan 7 2018, 12:32 PM
bgamari updated the Trac tickets for this revision.

Alright, given that @kazu_yamamoto agrees with the patch it looks good to me. I'll make sure it is merged to 8.4.

This revision is now accepted and ready to land.Jan 7 2018, 12:32 PM
This revision was automatically updated to reflect the committed changes.