Global error handler #73
Conversation
| def error_watchdog(exit_on_error:) | ||
| # Set exit handler | ||
| %w[INT TERM].each do |signal| | ||
| Signal.trap(signal) do |
There was a problem hiding this comment.
Signal.trap is global, so I'd rather seen it in the main CLI module.
|
|
||
| def write_system_information(stream) | ||
| write_section_header(stream, 'System Information') | ||
| stream.puts `uname -a` |
There was a problem hiding this comment.
You can use Etc.uname for that.
| def print_error(error) | ||
| write_error(error, $stderr) | ||
|
|
||
| File.open('crash.log', 'w') do |io| |
There was a problem hiding this comment.
I think it is better to just print it to the STDERR.
For example, in containers, the filesystem can disappear and you are only left with stdout/stderr.
There was a problem hiding this comment.
Some information like error message and stacktrace is printed to STDERR.
For providing support lots of information bits are dumped to a file. Have seen it in other projects and I think it is a good idea.
If working with ephemeral containers, this feature will be disabled, obviously.
| else | ||
| print_error(error) | ||
| end | ||
| exit(1) if exit_on_error |
There was a problem hiding this comment.
Seems like the better place for this is the main CLI module. This flag is being passed across several methods in the end to just call exit.
| puts 'Caught USR1; dumping a stack trace' | ||
| puts caller.map { |i| " #{i}" }.join("\n") | ||
| end | ||
| rescue ArgumentError |
There was a problem hiding this comment.
Rubocop should scream about this. Why this error can happen? At least a comment explaining why it would be useful.
There was a problem hiding this comment.
Indeed, Rubocop complaining.
Read about that when some signal cannot be trapped, ArgumentError is raised. The intention was to avoid error propagation and silently lose feature of dumping stack trace on USR1 signal.
Will add a comment
There was a problem hiding this comment.
will remove error catching 👍
|
moved error handling to |
lib/3scale_toolbox/cli.rb
Outdated
|
|
||
| def error_watchdog(exit_on_error:) | ||
| # Set exit handler | ||
| %w[INT TERM].each do |signal| |
There was a problem hiding this comment.
This is still in the ErrorHandler module. It is something that is global and being executed in a block.
By moving it into the main module I did not simply mean move the ErrorHandler module contents to a different file, but move this code into the other module so it is obvious it is global.
| # Catches errors and prints nice diagnostic messages | ||
| def error_watchdog(exit_on_error:) | ||
| # Set exit handler | ||
| %w[INT TERM].each do |signal| |
There was a problem hiding this comment.
This is just moving this from one module from another. It does not address the fact that this is global action that is for example not reverted back.
IMO this class does not have one reason to change. It does error handling but also installs signal handlers for an interruption. Those are two different concepts that should be separate.
error_watchdog should basically just be yield + rescue. Installing signal handlers should be done in self.run before entering the error_watchdog.
lib/3scale_toolbox/cli.rb
Outdated
| else | ||
| print_error(error) | ||
| end | ||
| exit(1) if exit_on_error |
There was a problem hiding this comment.
This is passing a parameter 3 levels deep just to call exit. IMO this should be done in self.run. That is what I meant by doing it in the CLI module. Not moving the whole method, but just this action.
|
Signal handlers installed by CLI module, error handler logic in its own module and |
|
|
||
| # Catches errors and prints nice diagnostic messages | ||
| def error_watchdog | ||
| error = false |
There was a problem hiding this comment.
Why not?
def error_watchdog
yield
false
rescue StandardError, ScriptError => err
handle_error err
return err
end
exe/3scale
Outdated
| @@ -4,4 +4,4 @@ require '3scale_toolbox' | |||
|
|
|||
| args = ARGV.clone | |||
|
|
|||
| ThreeScaleToolbox::CLI.run args | |||
| exit(ThreeScaleToolbox::CLI.run(args) ? 1 : 0) | |||
There was a problem hiding this comment.
Seems a bit odd to signal error with truthy return value.
|
|
Main error handler for all commands
crash.logis created with app version, system information, installed gems and load paths