guard / listen

The Listen gem listens to file modifications and notifies you about the changes.
https://rubygems.org/gems/listen
MIT License
1.92k stars 245 forks source link

application exceptions raised from the Listen.to callback will break listening in the process #505

Closed ColinDKelley closed 3 years ago

ColinDKelley commented 3 years ago

Current State

When the listen processor calls back into the application's callback block, if any exception gets raised from that block, it will tear down the processor thread and listen will stop working in that process.

Exception rescued in listen-wait_thread:
ArgumentError: "exception from callback!"
<call stack>

Desired State

Any exception raised from the application should be rescued and logged by the processor thread, but it should keep running; the exception should not tear down the thread.

Steps to Reproduce

require 'listen'

listener = Listen.to('/srv/app') do |modified, added, removed|
  raise ArgumentError, "exception from callback!"
end
listener.start
sleep

Design Questions

  1. Should we rescue Exception or StandardError? Typically it's recommended to just rescue StandardError. But that is probably too limiting here, isn't it? Consider for example a Ruby typo that raises a ScriptError for example.
ColinDKelley commented 3 years ago

We found this bug while testing v3.3.0.pre.2 in production. I want to fix it before releasing generally.

@ioquatix Any opinion on the rescue question? I'm leaning towards rescue Exception in this case.

ColinDKelley commented 3 years ago

If anyone's curious about the rescue-and-log that's described at the beginning, it's here. And that's using rescue Exception.

ioquatix commented 3 years ago

You should almost never do rescue Exception.

If the user code raises an exception, I don't see why that should not cause the event loop to exit.

Think about a user pressing Ctrl-C which causes Interrupt.

How about if the exception is silently consumed? User never knows there is problem.

ColinDKelley commented 3 years ago

You should almost never do rescue Exception.

Yes, I'm aware of that rule of thumb. It's fairly common to need to break that rule, though, and this feels like it might be such a case.

If the user code raises an exception, I don't see why that should not cause the event loop to exit.

It just happened to us in production while using v3.3.0.pre.2. We had a run-of-the-mill nil-dereference bug that was mysterious and cost a full day of head-scratching debugging. The symptoms were that the listen gem simply stopped working. Its inotify event monitoring thread was still running but its processor thread had ceased.

Think about a user pressing Ctrl-C which causes Interrupt.

I just checked and process exceptions like that get delivered to the main thread. Other threads (like this listen one) won't see those process exceptions.

Thread.new do
  sleep
rescue Exception => ex
  puts "rescued: #{ex.class}: #{ex.message}"
end

loop { } # Interrupt is raised here

How about if the exception is silently consumed? User never knows there is problem.

The exception is logged in both the old and new code. What's addressed in this issue is the problem of tearing down the listen processor thread, causing listen to become useless.

ColinDKelley commented 3 years ago

Fix merged and released in v3.3.0.