search

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

Don't log SystemExit as an error #532

Closed RobinDaugherty closed 3 years ago

RobinDaugherty commented 3 years ago

Using Guard with listen, when Guard is notified by listen that the Guardfile changed, Guard calls exit.

This results in the following alarming output:

E, [2021-03-09T10:48:09.258474 #79158] ERROR -- : Exception rescued in _process_changes:
SystemExit: exit
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/guard-2.16.2/lib/guard.rb:166:in `exit'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/guard-2.16.2/lib/guard.rb:166:in `_guardfile_deprecated_check'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/guard-2.16.2/lib/guard.rb:121:in `block in _listener_callback'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/event/config.rb:28:in `call'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/event/processor.rb:117:in `block in _process_changes'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/thread.rb:26:in `rescue_and_log'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/event/processor.rb:116:in `_process_changes'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/event/processor.rb:25:in `block in loop_for'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/event/processor.rb:20:in `loop'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/event/processor.rb:20:in `loop_for'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/event/loop.rb:85:in `_process_changes'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/event/loop.rb:51:in `block in start'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/thread.rb:26:in `rescue_and_log'
/opt/rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/listen-3.4.1/lib/listen/thread.rb:18:in `block in new'

This seems intended to indicate a failure state, but a SystemExit is not a failure.

ColinDKelley commented 3 years ago

Thanks for the PR. I agree that we shouldn't be logging SystemExit!

In fact, in other code at my company we avoid rescuing this set of exceptions that indicate process failure:

SystemExit, SystemStackError, NoMemoryError, SecurityError, SignalException

I'd been meaning to put those in a PR and had lost track of it. I think the cleanest way is to pick them off like this:

      def rescue_and_log(method_name, *args, caller_stack: nil)
        yield(*args)
      rescue SystemExit, SystemStackError, NoMemoryError, SecurityError, SignalException
        raise
      rescue Exception => exception # rubocop:disable Lint/RescueException
        _log_exception(exception, method_name, caller_stack: caller_stack)
      end
RobinDaugherty commented 3 years ago

I agree @ColinDKelley, it looks better to use a rescue block to accomplish this.

Is re-raising the correct action? It's not raising right now, so I assumed it was already going to stop running after rescuing. But now that I look back at it, maybe my change here would keep it from exiting. Sounds like your method of re-raising is something you've already tested?

ColinDKelley commented 3 years ago

@RobinDaugherty The simplest thing of all is to just follow the Ruby guidelines and rescue only StandardError (the default). That ways the process exit exceptions will just get raised out of the threads. Ruby handles this ok already.

I created issue #533 for this, and a corresponding PR: https://github.com/guard/listen/pull/535

Look reasonable?

ColinDKelley commented 3 years ago

@RobinDaugherty PR #535 got approved and I've merged it to master. I believe it addresses the problem you reported and fixed here a bit more generally. (Including fixing several thread race bugs in the specs that were being hidden by the broad rescue. Those were a pain to track down!)

Look good to release that and close this PR?

RobinDaugherty commented 3 years ago

Perfect! Thanks @ColinDKelley!

ColinDKelley commented 3 years ago

@RobinDaugherty v3.5.0 is released with PR #535. Thanks again for reporting the issue.