Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

SIGINT prints warning #197

Closed
nex3 opened this Issue · 16 comments

4 participants

@nex3

When I send a SIGINT to a process with an active listener, it prints the following warning:

[Listen warning]: Change block raised an exception: attempted to call a dead actor
Backtrace:
        /home/nweiz/.rvm/gems/ruby-1.9.3-p194/gems/celluloid-0.15.2/lib/celluloid/proxies/sync_proxy.rb:23:in `method_missing'
        /usr/local/google-old/home/code/listen/lib/listen/listener.rb:148:in `_wait_for_changes'
        /usr/local/google-old/home/code/listen/lib/listen/listener.rb:43:in `block in start'

This is on Ubuntu 12.04. git bisect indicates that this issue started with 74b1d11.

@thibaudgg
Owner

@nex3 sorry about that. it shouldn't happen, that for sure!
@swistak what do you think?

@swistak

I can reproduce this. No idea why it's happening though.

@swistak

I've used this to test the issue:

require 'listen'

callback = lambda do |modified, added, removed|
  modified.each{|path| puts "Modified #{path}" }
  added.each{|path|    puts "Added #{path}" }
  removed.each{|path|  puts "Removed #{path}" }
end 

puts "Listener starting"
@listener = Listen.to("./", only: /\.rb$/, &callback)
@listener.start
puts "Started"

puts "sleeping. SIGINT in 1s. Kill in 2s"
Thread.new do
  sleep 1
  Process.kill("INT", Process.pid)
  sleep 1
  exit!
end
sleep 
  • I've added notification to the Listen.stop

my conclusion is: Listen.stop actually does not stop all listeners :(
@thibaudgg based on your comments in #194 I assumed that Listen.stop will stop all listeners, but aparently it only kills celluloid and listener hapilly continues their work trying to call actors on (now stoped) celluloid.

I have no expirience with celluloid, and I really didn't dig in into the listen code, so any advise? should we collect the instances of listeners and manually stop them before attempting to shoot down celluloid?

@swistak

I'm still investigating but my theory is like this:

We're executing Listen.stop which sets the @stopping

Then in https://github.com/guard/listen/blob/master/lib/listen/listener.rb#L139 The Listener sees that end exits loop. So far so good.

But then in https://github.com/guard/listen/blob/master/lib/listen/listener.rb#L148
supervisor.finalize is called which freaks out since the celluloid was stoped in the meantime in https://github.com/guard/listen/blob/master/lib/listen.rb#L37

Now 100 points question is why it does not happen when we call Listen.stop from tests and/or by hand in app, but does happen in SIGINT handler.

@swistak

Ok. I've prepared the fix. I still don't like the race condition but this should fix the issue at hand.
I'm still working on adding spec that'd test this issue

@swistak swistak referenced this issue from a commit in swistak/listen
@swistak swistak Preventing finalization on dead actor
When using Calluloid.shutdown there's race condition between listeners
and celluloid which in some cases leads to Listener calling #finalize on
dead actor. Added a guard that checks if actor is alive when we're
exiting from #_wait_for_changes and calls finalizer only if it's alive.

PS. I couldn't for a life figure out how to make this test pass. I hate
mocking libraries with all my heart. Feel free to fix this.

Fixes #197
7b244d6
@swistak

I've added a fix & spec but it does not pass since mocking library bitches that some object receives unexpected call, after 10 minutes of trying to figure it out I gave up. Anyone who knows how rspec-mock works please fix it before merging.

@thibaudgg thibaudgg closed this in #198
@thibaudgg thibaudgg reopened this
@thibaudgg
Owner

@swistak Thanks for your investigation and PR, I just commit (5dc6ddf) a fix that remove Celluloid.shutdown from Listen.stop. I have the feeling that shut-downing Celluloid from there is maybe a little too much if other Celluloid based librairies are using Listen.

@nex3 could you give a try to master branch before I release a fixed release, thanks!

Regarding Signal trapping, what do you think about removing it completely and let Listen users handle it themselves? We would avoid such issue in the feature...

@swistak

I think that removin g signal trap might be a best idea.
If the shutdown of listeners is necessery before quiting aplication (I don't know if it's), we might add something along the lines of Kernel.at_exit { Listen.stop(:and_wait) }
Which would stop listeners and wait for all of them to finish?

@thibaudgg
Owner

Yeah that could work. @nex3 / @guard/core-team what do you think about that?

@swistak

I guess we need answers to two questions:

Why the library stoped listeners on ctrl+c in the first place?
Is the on-exit cleanup necessery, or will closing the application cleanup everything anyway?

@thibaudgg
Owner

That's a good question, Guard already handle signal (ctrl+c) himself (https://github.com/guard/guard/blob/master/lib/guard/setuper.rb#L207-L234), not sure about the other gems, but it would be easier to lets every user handling that on their side.

I think that finalizing every Celluloid actors with Listen.stop should be enough to closing the app cleanly. I'll do some test.

@thibaudgg
Owner

@swistak I start to remove Signal handling in remove_signal_handling branch, could you give it a try? Thanks!

@swistak swistak referenced this issue from a commit in swistak/listen
@swistak swistak Removing signal handlers
Stopping Listen on SIGINT seems to be no longer equired.
Instead we provide a way to stop all listeners in a non-blocking
`Listen.stop` and blocking way `Listen.stop(:and_wait)`

It's recommended to call `Listen.stop(:and_wait)` before exiting your
application to finalize all listeners.

Related to #194
Fixes #197
7ba45a6
@swistak

I wonder if stopping Celluloid is a good idea. (I think you've commented on that before that it might cause app to fail if it also used celluloid).

Maybe it'd be better this way? (not tested - quick draft to show alternative direction).

swistak@7ba45a6

@thibaudgg
Owner

I think Listen.stop should be more used for testing purpose, so there we really want to shutdown Celluloid.
The right way of stopping a listener should be listener.stop and it the user responsibility to keep track of each listener object.

Sounds good?

@swistak

Alright. If that's the case then your version looks much better.

I'd only recommend to add info to readme that user should do that (keep track of listeners and stop them on finish).

@thibaudgg
Owner

Ok good point, I done it and submit #200

@rymai rymai closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.