Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

listen needs a big fat signal hammer #105

Closed
ahoward opened this issue Apr 22, 2013 · 20 comments
Closed

listen needs a big fat signal hammer #105

ahoward opened this issue Apr 22, 2013 · 20 comments
Milestone

Comments

@ahoward
Copy link

ahoward commented Apr 22, 2013

some libs, like rb-fsevent do a bad job at handling signals currently. a meta-lib like listen should ensure signals are handled correctly by any/all adapters.

current i need to setup a watchdog child process

BEGIN {
# setup a child process to catch signals and brutally shut down the parent as
# a monkey-patch to listen/rb-fsevent's busted ctrl-c handling...
#
  unless((pid = fork))
    ppid = Process.ppid

    begin
      trap('SIGINT'){
        %w(
          SIGTERM SIGINT SIGQUIT SIGKILL
        ).each do |signal|

          begin
            Process.kill("-#{ signal }", ppid)
          rescue Object
            nil
          end

          sleep(rand)
        end
      }

      loop do
        Process.kill(0, ppid)
        sleep(1)
      end
    rescue Object => e
      exit!(0)
    end
  end

and then make Listen aware of it

    begin
      Listen.to!(*@directories, :latency => 0.1) do |modified, added, removed|
        [modified, added, removed].flatten.compact.uniq.each do |path|
          rego[path]
        end
      end
    rescue SignalException
      STDERR.reopen('/dev/null', 'w+')
      exit(42)
    end

just to be able to not want to eat glass using cli scripts that delegate fs event handling to listen....

@ahoward
Copy link
Author

ahoward commented Apr 22, 2013

ps. rb-fsevent is the adapter being used in the this case, but i think listen should protect users from adapter signal handling, otherwise the client code needs to understand the details of any/all adapters....

@nicovalencia
Copy link

👍 the number of times I end up using kill to bump the listener is redic.

@ttilley
Copy link
Member

ttilley commented Apr 23, 2013

interesting, because rb-fsevent's manual signal hacks are because guard used to use signals as a control mechanism. things were broken without it... now guard doesn't use signals as a control and things are broken without it?

hmm. deep thoughts.

@ttilley
Copy link
Member

ttilley commented Apr 23, 2013

The code and explanation:

int main(int argc, const char* argv[])
{
  /*
   * a subprocess will initially inherit the process group of its parent. the
   * process group may have a control terminal associated with it, which would
   * be the first tty device opened by the group leader. typically the group
   * leader is your shell and the control terminal is your login device. a
   * subset of signals triggered on the control terminal are sent to all members
   * of the process group, in large part to facilitate sane and consistent
   * cleanup (ex: control terminal was closed).
   *
   * so why the overly descriptive lecture style comment?
   *   1. SIGINT and SIGQUIT are among the signals with this behavior
   *   2. a number of applications gank the above for their own use
   *   3. ruby's insanely useful "guard" is one of these applications
   *   4. despite having some level of understanding of POSIX signals and a few
   *      of the scenarios that might cause problems, i learned this one only
   *      after reading ruby 1.9's process.c
   *   5. if left completely undocumented, even slightly obscure bugfixes
   *      may be removed as cruft by a future maintainer
   *
   * hindsight is 20/20 addition: if you're single-threaded and blocking on IO
   * with a subprocess, then handlers for deferrable signals might not get run
   * when you expect them to. In the case of Ruby 1.8, that means making use of
   * IO::select, which will preserve correct signal handling behavior.
   */
  if (setpgid(0,0) < 0) {
    fprintf(stderr, "Unable to set new process group.\n");
    return 1;
  }

This prevents initially process-group-wide signals from reaching rb-fsevent at all, as it is in its own process group. Rather than poor handling of signals, the signal never makes it to the process to begin with (at least in this example, where it's a ctrl-c sent via the control terminal). Since the library saves the PID of fsevent_watch, clean shutdown triggered as part of ctrl-c handling can pass on a kill to fsevent_watch.

  def stop
    unless @pipe.nil?
      Process.kill('KILL', @pipe.pid)
      @pipe.close
    end
  rescue IOError
  ensure
    @running = false
  end

Forgive the less then perfect coherency, it's 2 hours past Zolpidem o'clock.

@ttilley
Copy link
Member

ttilley commented Apr 23, 2013

aside: when did all Ls in the comment get capitalized? wonder how that happened.

@thibaudgg
Copy link
Member

I'm in the process of rewriting Listen with Celluloid and I hope this will handle that problem. In the meantime feel free to submit a pull request for resolving that in the 1.x release (master branch). Thanks!

@ahoward
Copy link
Author

ahoward commented Apr 23, 2013

@thibaudgg i think the hope is unfounded: celluloid doesn't deal with subprocesses/signals...

@ahoward
Copy link
Author

ahoward commented Apr 23, 2013

/cc @tarcieri

@tarcieri
Copy link

Celluloid doesn't do a great job of signal handling right now, but it could. This is a problem I've been meaning to solve.

What Ruby really needs, IMO, is a simple gem which routes traps to handlers that run in a separate thread. The traps should write to a pipe, and another thread should read from the pipe and run the appropriate handlers.

This is a gem I've been meaning to write, but if someone else wants to write it, knock yourself out ;)

@ahoward
Copy link
Author

ahoward commented Apr 23, 2013

@tarcieri why a pipe? i generally push 'em all into a Queue and have a thread consuming it...

@tarcieri
Copy link

@ahoward it's the same idea, however in Ruby 2.0 you cannot acquire a mutex within a signal handler (which, IMO, is a good idea)

@ahoward
Copy link
Author

ahoward commented Apr 23, 2013

@tarcieri taking this on twitter to avoid hi-jacking the issue...

@thibaudgg
Copy link
Member

Thanks for your feedback @tarcieri , signal handling built-in Celluloid would be great and a external gem too. That would be handy for Guard too.

@ahoward
Copy link
Author

ahoward commented Apr 24, 2013

@thibaudgg so, even with a re-write this is is legit i think... the listen library needs to, at minimum, catch ctrl-c correctly such that it's usable from the console - thoughts?

@thibaudgg
Copy link
Member

@ahoward yeah I agree, can you please submit a pull request? Thanks!

@ahoward
Copy link
Author

ahoward commented Apr 24, 2013

@thibaudgg i assume it'd also need to work with jruby (no fork) ??

@thibaudgg
Copy link
Member

Yep, it should work with MRI, jRuby & rbx and also work on each platform (OS X, Lunix, BSD, Windows).
If it simpler too handle you can only support 1.9+ and we'll release it for Listen 2.0.0 only.

@rehevkor5
Copy link
Contributor

👍

@thibaudgg
Copy link
Member

@ahoward could you please try the v2.0 branch to see if 3162761 as fix your problem? Thanks!

@thibaudgg
Copy link
Member

Should be fixed in v2.0 branch, feel free to reopen this issue if not.

adamlogic added a commit to edgecase/ghpreview that referenced this issue Dec 22, 2013
Manual signal trapping is necessary to exit ghpreview.
More discussion here: guard/listen#105

Fixes #18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants