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

Already on GitHub? Sign in to your account

USR1/USR2 handlers register late, leading to unpredictable results. #263

Closed
steakknife opened this Issue Mar 16, 2012 · 12 comments

Comments

Projects
None yet
3 participants
Contributor

steakknife commented Mar 16, 2012

The problem is that the refactored code doesn't work as anticipated. The previous code registers them sufficiently early and works correctly, btw.

The fix will be attached soon.

Contributor

netzpirat commented Mar 16, 2012

The signal traps are attached before the listener starts and since the signals change the listener state, this is perfectly valid.

The refactoring was done because the listener will be replaced with Listen soon and I didn't want to have them lost.

You current Guard master does not work as advertised. When I send a SIGUSR1 or SIGUSR2 to your version of Guard, it exits the process.

In addition the specs are also failing:

» git clone git://github.com/steakknife/guard.git                                                      ruby-1.9.3-p125 
Cloning into 'guard'...
remote: Counting objects: 5415, done.
remote: Compressing objects: 100% (2020/2020), done.
remote: Total 5415 (delta 3427), reused 5313 (delta 3345)
Receiving objects: 100% (5415/5415), 770.63 KiB | 349 KiB/s, done.
Resolving deltas: 100% (3427/3427), done.

otherland ~
» cd guard                                                                                             ruby-1.9.3-p125 

otherland ~/guard
» bundle                                                                               master 5c60354  ruby-1.9.3-p125 
Fetching gem metadata from http://rubygems.org/........
Using rake (0.9.2.2) 
Using bundler (1.1.1) 
Using coderay (1.0.5) 
Using diff-lcs (1.1.3) 
Using ffi (1.0.11) 
Using thor (0.14.6) 
Using guard (1.0.0) from source at /Users/michi/guard 
Installing hpricot (0.8.6) with native extensions 
Installing mustache (0.99.4) 
Installing rdiscount (1.6.8) with native extensions 
Installing ronn (0.7.3) 
Installing guard-ronn (0.1.2) 
Using guard-rspec (0.6.0) 
Using method_source (0.7.1) 
Using slop (2.4.4) 
Using pry (0.9.8.4) 
Using rb-readline (0.4.2) 
Installing redcarpet (2.1.0) with native extensions 
Using rspec-core (2.8.0) 
Using rspec-expectations (2.8.0) 
Using rspec-mocks (2.8.0) 
Using rspec (2.8.0) 
Installing ruby_gntp (0.3.4) 
Using yard (0.7.5) 
Your bundle is complete! Use `bundle show [gemname]` to see where a bundled gem is installed.

otherland ~/guard
» bundle exec rspec spec/guard/listeners/signals_spec.rb                               master 5c60354  ruby-1.9.3-p125 
Please do not update/create files while tests are running.
Run options: include {:focus=>true}

All examples were filtered out; ignoring {:focus=>true}
FFFFFFFFFFFF

Failures:

  1) Guard::Listener when #initialize a new Listener on an USR1 signal#pause once
     Failure/Error: guard.should_receive(:pause).once
     SignalException:
       SIGUSR1
     # ./spec/guard/listeners/signals_spec.rb:23:in `block (4 levels) in <top (required)>'

  2) Guard::Listener when #initialize a new Listener on an USR2 signal does nothing
     Failure/Error: let(:guard) { Guard::Listener.new }
     SignalException:
       SIGUSR2
     # ./lib/guard/listener.rb:60:in `initialize'
     # ./spec/guard/listeners/signals_spec.rb:11:in `new'
     # ./spec/guard/listeners/signals_spec.rb:11:in `block (3 levels) in <top (required)>'
     # ./spec/guard/listeners/signals_spec.rb:35:in `block (4 levels) in <top (required)>'

  3) Guard::Listener when #initialize a new Listener on duplicate USR1 signals#pause once
     Failure/Error: guard.should_receive(:pause).once
     SignalException:
       SIGUSR1
     # ./spec/guard/listeners/signals_spec.rb:50:in `block (4 levels) in <top (required)>'

  4) Guard::Listener when #initialize a new Listener on duplicate USR2 signals does nothing
     Failure/Error: guard.should_not_receive(:pause).any_number_of_times
     SignalException:
       SIGUSR2
     # ./spec/guard/listeners/signals_spec.rb:65:in `block (4 levels) in <top (required)>'

  5) Guard::Listener when #initialize a new Listener on an USR1 and then USR2 signal#pause and then #run
     Failure/Error: Process.kill :USR2, Process.pid
     SignalException:
       SIGUSR2
     # ./spec/guard/listeners/signals_spec.rb:76:in `exception'
     # ./spec/guard/listeners/signals_spec.rb:76:in `kill'
     # ./spec/guard/listeners/signals_spec.rb:76:in `block (4 levels) in <top (required)>'

  6) Guard::Listener when #initialize a new Listener on an USR2 and then USR1 signal#run once
     Failure/Error: Unable to find matching line from backtrace
     SignalException:
       SIGUSR2
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/hooks.rb:35:in `exception'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/hooks.rb:35:in `run_in'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/hooks.rb:70:in `block in run_all'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/hooks.rb:70:in `each'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/hooks.rb:70:in `run_all'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/hooks.rb:368:in `run_hook'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:292:in `block in run_before_each_hooks'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:292:in `each'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:292:in `run_before_each_hooks'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example.rb:217:in `run_before_each'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example.rb:79:in `block in run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example.rb:173:in `with_around_hooks'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example.rb:77:in `run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:355:in `block in run_examples'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:351:in `map'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:351:in `run_examples'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:337:in `run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:338:in `block in run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:338:in `map'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:338:in `run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:338:in `block in run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:338:in `map'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/example_group.rb:338:in `run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/command_line.rb:28:in `map'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/command_line.rb:28:in `block in run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/reporter.rb:34:in `report'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/command_line.rb:25:in `run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/runner.rb:80:in `run_in_process'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/runner.rb:69:in `run'
     # /Users/michi/.rvm/gems/ruby-1.9.3-p125-perf/gems/rspec-core-2.8.0/lib/rspec/core/runner.rb:10:in `block in autorun'

  7) Guard::Listener when #initialize a new Listener when #pause on an USR1 signal does nothing
     Failure/Error: before { Process.kill :USR1, Process.pid }
     SignalException:
       SIGUSR1
     # ./spec/guard/listeners/signals_spec.rb:110:in `kill'
     # ./spec/guard/listeners/signals_spec.rb:110:in `block (5 levels) in <top (required)>'

  8) Guard::Listener when #initialize a new Listener when #pause on an USR2 signal#pause once
     Failure/Error: before { Process.kill :USR2, Process.pid }
     SignalException:
       SIGUSR2
     # ./spec/guard/listeners/signals_spec.rb:122:in `kill'
     # ./spec/guard/listeners/signals_spec.rb:122:in `block (5 levels) in <top (required)>'

  9) Guard::Listener when #initialize a new Listener when #pause on duplicate USR1 signals does nothing
     Failure/Error: Process.kill :USR1, Process.pid
     SignalException:
       SIGUSR1
     # ./spec/guard/listeners/signals_spec.rb:136:in `kill'
     # ./spec/guard/listeners/signals_spec.rb:136:in `block (5 levels) in <top (required)>'

  10) Guard::Listener when #initialize a new Listener when #pause on duplicate USR2 signals#run once
     Failure/Error: Process.kill :USR2, Process.pid
     SignalException:
       SIGUSR2
     # ./spec/guard/listeners/signals_spec.rb:151:in `kill'
     # ./spec/guard/listeners/signals_spec.rb:151:in `block (5 levels) in <top (required)>'

  11) Guard::Listener when #initialize a new Listener when #pause on an USR1 and then USR2 signal#run once
     Failure/Error: Process.kill :USR1, Process.pid
     SignalException:
       SIGUSR1
     # ./spec/guard/listeners/signals_spec.rb:166:in `kill'
     # ./spec/guard/listeners/signals_spec.rb:166:in `block (5 levels) in <top (required)>'

  12) Guard::Listener when #initialize a new Listener when #pause on an USR2 and then USR1 signal#run and then #pause
     Failure/Error: Process.kill :USR2, Process.pid
     SignalException:
       SIGUSR2
     # ./spec/guard/listeners/signals_spec.rb:181:in `kill'
     # ./spec/guard/listeners/signals_spec.rb:181:in `block (5 levels) in <top (required)>'

Finished in 0.00904 seconds
12 examples, 12 failures

Failed examples:

rspec ./spec/guard/listeners/signals_spec.rb:22 # Guard::Listener when #initialize a new Listener on an USR1 signal#pause once
rspec ./spec/guard/listeners/signals_spec.rb:34 # Guard::Listener when #initialize a new Listener on an USR2 signal does nothing
rspec ./spec/guard/listeners/signals_spec.rb:49 # Guard::Listener when #initialize a new Listener on duplicate USR1 signals#pause once
rspec ./spec/guard/listeners/signals_spec.rb:64 # Guard::Listener when #initialize a new Listener on duplicate USR2 signals does nothing
rspec ./spec/guard/listeners/signals_spec.rb:79 # Guard::Listener when #initialize a new Listener on an USR1 and then USR2 signal#pause and then #run
rspec ./spec/guard/listeners/signals_spec.rb:94 # Guard::Listener when #initialize a new Listener on an USR2 and then USR1 signal#run once
rspec ./spec/guard/listeners/signals_spec.rb:112 # Guard::Listener when #initialize a new Listener when #pause on an USR1 signal does nothing
rspec ./spec/guard/listeners/signals_spec.rb:124 # Guard::Listener when #initialize a new Listener when #pause on an USR2 signal#pause once
rspec ./spec/guard/listeners/signals_spec.rb:140 # Guard::Listener when #initialize a new Listener when #pause on duplicate USR1 signals does nothing
rspec ./spec/guard/listeners/signals_spec.rb:155 # Guard::Listener when #initialize a new Listener when #pause on duplicate USR2 signals#run once
rspec ./spec/guard/listeners/signals_spec.rb:170 # Guard::Listener when #initialize a new Listener when #pause on an USR1 and then USR2 signal#run once
rspec ./spec/guard/listeners/signals_spec.rb:185 # Guard::Listener when #initialize a new Listener when #pause on an USR2 and then USR1 signal#run and then #pause

I'm very sorry that you're disappointed, but as a maintainer it's my job to test each pull request that I'm merging and make sure the specs are passing and the changes works.

Owner

rymai commented Mar 16, 2012

@steakknife could you explain more in details the "unpredictable results"?

EDIT: As far as I can tell –using the guard@guard/master– sending kill -USR1 pid pauses Guard and sending kill -USR2 pid un-pauses Guard, as expected.

Contributor

steakknife commented Mar 18, 2012

The handlers are not installed for several seconds with the refactored patch. If a USR1 is sent before the handler is installed, the result is obvious.

Contributor

steakknife commented Mar 18, 2012

@netzpirat

The previous code works with s/self.windows?/Listener.windows?/. It was throwing a NoMethod error that was not visible from inside the Thread block.

Also, the signal handlers are not added for over 40 seconds consistently whereas the pull request add them at about 500 ms.

~/guard-example (master ✔) guard 
/Volumes/Users/barry/.rvm/gems/ruby-1.9.3-p125@guard-example/gems/guard-1.0.0/lib/guard/listener.rb:310: Added signal handlers to Guard, PID 65581
Guard uses GNTP to send notifications.
Guard is now watching at '/Volumes/Users/barry/guard-example'
Bundle already up-to-date
Starting guard-rake build
Guard::Rails will now restart your app on port 3000 using development environment.
Starting Rails...
=> Booting Thin
=> Rails 3.2.2 application starting in production on http://0.0.0.0:3000
=> Call with -d to detach
=> Ctrl-C to shutdown server
.
.
.
.
.
.
/Volumes/Users/barry/.rvm/gems/ruby-1.9.3-p125@guard-example/gems/guard-1.0.0/lib/guard.rb:205: Added signal handlers to Guard, PID 65581

Furthermore, installing traps on windows fails with an ArgumentError exception.

Contributor

netzpirat commented Mar 18, 2012

So the issue is that the handlers get attached after Guard triggers start. Why not just move it?

I have tested the Signal handlers on three Rubies on Windows 7 and they can be attached without exception. What was your test case?

Owner

rymai commented Mar 18, 2012

Yeah, why not moving the handlers registration here with a check to be sure @listener is instanciated (but maybe it's not even necessary)?

@netzpirat netzpirat closed this in efe8f76 Mar 18, 2012

Contributor

steakknife commented Mar 18, 2012

Way to encourage collaboration. Good luck!

Owner

rymai commented Mar 18, 2012

Don't be rude man! We did what you wanted and you're still not happy? Discussion is part of collaboration, don't take it that way! You could at least say "Thanks"! :)

Contributor

netzpirat commented Mar 18, 2012

Let me summarize what happend until now:

  1. You send us pull request #254 with a broken implementation and failing specs that test the wrong stuff.
  2. I fixed the implementation and rewrote the specs in 6802646 for you and I gave you a polite explanation why I did so.
  3. You gave me a disparaging answer without any constructive feedback.
  4. You opened #263 with (again) a disparaging description and no constructive feedback.
  5. I gave you a more detailed explanation why I changed some code from your pull request.
  6. You opened pull request #264 without a meaningful title and description. You ignoring my previous comment why I moved the logic to the Guard class and you moved the handlers back to the Listener class. Again, the specs are failing.
  7. I gave you a polite feedback why I can't merge the pull request #264.
  8. I decided to fix the problem for you in efe8f76.
  9. You gave again a rude answer without any constructive feedback.

In short, you send us broken code, I fix it for you and you react with abusive behavior. You are resistant to any feedback, ignoring any good advice and you communicate in a way that is not acceptable. And you call me to not encourage collaboration? What a joke.

What did you expect? That I'm merging broken code and failing specs? No way. It was never my intention to offend you, I always tried to support you to get the feature you want into Guard. And instead of learning from experienced people and saying thanks, you behave totally strange and offend me.

You have a serious problem man. I suggest that you maintain your own fork of Guard to fiddle around and leave us alone. From now on I will ignore you and immediately close every issue and pull request without comment.

Contributor

steakknife commented Mar 18, 2012

This is not professional at all. I suggest you ask your contributors to behave in a more constructive manner in the future to prevent such misunderstandings. Life is too short to waste on such foolishness. I let you have the last word.

Contributor

steakknife commented Mar 18, 2012

For the record, Windows 2003 Server, RubyInstaller 1.9.3p0 http://screencast.com/t/P4wlJ66Jk44M

Be nice, it's contagious. 🍰 🍺

Owner

rymai commented Mar 18, 2012

Man, what is your drug seriously? 🐻

I can't believe you dare telling us to "be nice".

If you really want to collaborate to Guard, you'll have to (at least):

  • be polite;
  • be thankful (you never said "thanks", we all did at least once);
  • accept that sometimes your code is not perfect;
  • not be always ironic otherwise no one will ever trust you.

Why don't you simply say "I'm really sorry guys, there were some misunderstanding but now everything is alright, thanks very much for including my contribution into Guard." and stop playing the victim that has nothing to say but "Be nice" when you're not nice yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment