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

Catch exceptions in hooks defined by users #2155

Merged

Conversation

andrykonchin
Copy link
Contributor

@andrykonchin andrykonchin commented Mar 7, 2020

Description

Suppress exceptions in user defined hooks (like on_worker_boot). We have just to rescue and log them.

Example of logged message:

WARNING hook on_restart failed with exception (RuntimeError) Error from hook
% Traceback (most recent call last):
	14: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest/parallel.rb:33:in `block (2 levels) in start'
	13: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest.rb:1026:in `run_one_method'
	12: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest/test.rb:93:in `run'
	11: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest/test.rb:211:in `with_info_handler'
	10: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest.rb:365:in `on_signal'
	 9: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest/test.rb:94:in `block in run'
	 8: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest.rb:270:in `time_it'
	 7: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest/test.rb:95:in `block (2 levels) in run'
	 6: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest/test.rb:195:in `capture_exceptions'
	 5: from /Users/andrykonchin/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/minitest-5.14.0/lib/minitest/test.rb:98:in `block (3 levels) in run'
	 4: from /Users/andrykonchin/projects/puma/test/test_config.rb:226:in `test_run_hooks_and_exception'
	 3: from /Users/andrykonchin/projects/puma/lib/puma/configuration.rb:279:in `run_hooks'
	 2: from /Users/andrykonchin/projects/puma/lib/puma/configuration.rb:279:in `each'
	 1: from /Users/andrykonchin/projects/puma/lib/puma/configuration.rb:281:in `block in run_hooks'
/Users/andrykonchin/projects/puma/test/test_config.rb:220:in `block (2 levels) in test_run_hooks_and_exception': Error from hook (RuntimeError)

Closes #1551

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@nateberkopec
Copy link
Member

Makes sense and LGTM.

@nateberkopec nateberkopec added feature waiting-for-changes Waiting on changes from the requestor labels Mar 7, 2020
@andrykonchin andrykonchin force-pushed the 1551-handle-exceptions-in-hooks branch from 529adbd to 60d4a70 Compare March 8, 2020 21:24
@andrykonchin
Copy link
Contributor Author

andrykonchin commented Mar 8, 2020

@nateberkopec

There is only one issue with logging. I looked through the project and found out there are two options

  • to directly write into STDERR
  • to use Events (which could write either to STDOUT/STDERR or to strings).

I prefer the second one because it's easier to check in tests and Events provides more flexible interface - we can optionally print exception stack trace as debug message.

Please let me know if you prefer the first option.

@andrykonchin andrykonchin force-pushed the 1551-handle-exceptions-in-hooks branch from fb84056 to 55eddb7 Compare March 8, 2020 21:55
@andrykonchin andrykonchin changed the title [WIP] Catch exceptions in hooks defined by users Catch exceptions in hooks defined by users Mar 8, 2020
@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Mar 8, 2020
History.md Outdated Show resolved Hide resolved
test/test_config.rb Outdated Show resolved Hide resolved
test/test_config.rb Outdated Show resolved Hide resolved
@nateberkopec nateberkopec added waiting-for-changes Waiting on changes from the requestor and removed waiting-for-review Waiting on review from anyone labels Mar 9, 2020
@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Mar 9, 2020
@nateberkopec nateberkopec merged commit 7827a6c into puma:master Mar 10, 2020
@nateberkopec
Copy link
Member

Thanks for your contribution @andrykonchin!

@andrykonchin andrykonchin deleted the 1551-handle-exceptions-in-hooks branch March 10, 2020 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception handling for on_worker_boot
2 participants