Skip to content
This repository has been archived by the owner on Sep 15, 2018. It is now read-only.

Fix a potential Signal starvation based on creation order #41

Merged
merged 1 commit into from Jul 22, 2018
Merged

Fix a potential Signal starvation based on creation order #41

merged 1 commit into from Jul 22, 2018

Conversation

ipetkov
Copy link
Collaborator

@ipetkov ipetkov commented Jul 22, 2018

  • As observed in Signal starvation caused by order of registration/dropping #38, Signal instances can starve based on the order they are created in, and this ordering appears to be platform/OS specific
  • The crux of the issue is that we woud only attempt to broadcast any pending signals if we successfully read out at least one byte from the global pipe.
  • For reasons unclear to me, the affected Signal instance would get woken up after the signal handler writes to the global pipe, but it would immediately hit a WouldBlock error and give up, bypassing the broadcast attempt (even though the pending flag was correctly set).
    • Maybe this has to do with OS specifics with how the bytes are delivered (or not), or with some complex interaction with tokio and the pipe registration. It seems fishy since strace logs didn't show the signal handler pipe write fail either, but I'm all out of ideas
  • The fix appears simple: unconditionally attempt to broadcast any pending signals any time a Driver instance is woken up.
  • Since we perform an atomic check for each pending signal, we know that each (coalesced) signal broadcast will happen at most once. If we were supuriously woken up and no signals were pending, then nothing will be yielded to any pollers of Signal
  • The down side is that since each Signal instance polls a Driver instance, each poll to Signal will essentially perform N atomic operations (N = number of signals we support) in an attempt to broadcast any pending signals.
    • I have some ideas on how we can better optimize this by going back to using a single global Driver, but I'll expand on those separately after I find some more time!

Fixes #38

r? @alexcrichton

* As observed in #38, Signal instances can starve based on the order
they are created in, and this ordering appears to be platform/OS
specific
* The crux of the issue is that we woud only *attempt* to broadcast any
pending signals if we successfully read out at least one byte from the
global pipe.
* For reasons unclear to me, the affected Signal instance would get
woken up after the signal handler writes to the global pipe, but it
would immediately hit a WouldBlock error and give up, bypassing the
broadcast attempt (even though the pending flag was correctly set).
 - Maybe this has to do with OS specifics with how the bytes are
   delivered (or not), or with some complex interaction with tokio and
   the pipe registration. It seems fishy since strace logs didn't show
   the signal handler pipe write fail either, but I'm all out of ideas
* The fix appears simple: unconditionally attempt to broadcast any
pending signals *any* time a Driver instance is woken up.
* Since we perform an atomic check for each pending signal, we know that
each (coalesced) signal broadcast will happen at most once. If we were
supuriously woken up and no signals were pending, then nothing will be
yielded to any pollers of Signal
* The down side is that since each Signal instance polls a Driver
instance, each poll to Signal will essentially perform N atomic
operations (N = number of signals we support) in an attempt to broadcast
any pending signals.
 - However, we can revisit optimizing this better in the future

Fixes #38
@alexcrichton
Copy link
Owner

Makese sense to me, thanks for tracking this down!

Most of signal handling is pretty inefficient anyway, so I'm not too worried about the perf here!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signal starvation caused by order of registration/dropping
2 participants