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

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

Closed
ColinDKelley opened this issue Nov 5, 2020 · 5 comments
Labels
Milestone

Comments

@ColinDKelley
Copy link
Collaborator

ColinDKelley commented Nov 5, 2020

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 ColinDKelley added this to the v3.3.0 milestone Nov 5, 2020
@ColinDKelley
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Member

ioquatix commented Nov 5, 2020

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
Copy link
Collaborator Author

ColinDKelley commented Nov 5, 2020

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
Copy link
Collaborator Author

Fix merged and released in v3.3.0.

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

No branches or pull requests

2 participants