From 0d33861130742913ffa84a00870fd9a87557aff0 Mon Sep 17 00:00:00 2001 From: Andrew Sullivan Cant Date: Sun, 19 Mar 2017 02:09:07 -0400 Subject: [PATCH 1/2] Fix restart handling in Manager Add the proper rescue handler for Restarts, which will properly call the retry and restart the Celluloid actors. Some additional changes: * extract the Celluloid actor handling to a facade class, so that the Manager is less responsible and easier to test * add a log error message check into the integration test --- CHANGELOG | 1 + lib/gitdocs.rb | 1 + lib/gitdocs/celluloid_facade.rb | 73 ++++++++++++++++++++ lib/gitdocs/manager.rb | 105 +++++++---------------------- test/integration/test_helper.rb | 6 ++ test/unit/celluloid_facade_test.rb | 11 +++ test/unit/manager_test.rb | 98 ++++++++++++++++++++++++++- 7 files changed, 213 insertions(+), 82 deletions(-) create mode 100644 lib/gitdocs/celluloid_facade.rb create mode 100644 test/unit/celluloid_facade_test.rb diff --git a/CHANGELOG b/CHANGELOG index bf8987b..a618e86 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ * fix gem dependencies to preserve Ruby v2.0.0 compatibility (@acant) * add --host flag for setting the address for the web interface (@acant) + * fix unexpected exit when changing settings through web UI (@acant) 0.6.2 (July 5th 2016) diff --git a/lib/gitdocs.rb b/lib/gitdocs.rb index c8f67c0..00c9287 100644 --- a/lib/gitdocs.rb +++ b/lib/gitdocs.rb @@ -18,6 +18,7 @@ require 'gitdocs/configuration' require 'gitdocs/cli' require 'gitdocs/manager' +require 'gitdocs/celluloid_facade' require 'gitdocs/synchronizer' require 'gitdocs/notifier' require 'gitdocs/git_notifier' diff --git a/lib/gitdocs/celluloid_facade.rb b/lib/gitdocs/celluloid_facade.rb new file mode 100644 index 0000000..044e635 --- /dev/null +++ b/lib/gitdocs/celluloid_facade.rb @@ -0,0 +1,73 @@ +# -*- encoding : utf-8 -*- + +module Gitdocs + class CelluloidFascade + # @param [String] host + # @param [Integer] port + def initialize(host, port) + @host = host + @port = port + end + + # @return [void] + def start + Celluloid.boot unless Celluloid.running? + @supervisor = Celluloid::SupervisionGroup.run! + + # Start the web server ################################################### + app = + Rack::Builder.new do + use Rack::Static, + urls: %w(/css /js /img /doc), + root: File.expand_path('../public', __FILE__) + use Rack::MethodOverride + map('/settings') { run SettingsApp } + map('/') { run BrowserApp } + end + @supervisor.add( + Reel::Rack::Server, + as: :reel_rack_server, + args: [ + app, + { + Host: @host, + Port: @port, + quiet: true + } + ] + ) + + # Start the synchronizers ################################################ + Share.all.each do |share| + @supervisor.add( + Synchronizer, as: share.id.to_s, args: [share] + ) + end + + # Start the repository listeners ######################################### + @listener = + Listen.to( + *Share.paths, + ignore: /#{File::SEPARATOR}\.git#{File::SEPARATOR}/ + ) do |modified, added, removed| + all_changes = modified + added + removed + changed_repository_paths = + Share.paths.select do |directory| + all_changes.any? { |x| x.start_with?(directory) } + end + + changed_repository_paths.each do |directory| + actor_id = Share.find_by_path(directory).id.to_s + Celluloid::Actor[actor_id].async.synchronize + end + end + @listener.start + end + + # @return [void] + def terminate + @listener.stop if @listener + @supervisor.terminate if @supervisor + end + end +end diff --git a/lib/gitdocs/manager.rb b/lib/gitdocs/manager.rb index 74c13ab..e513713 100644 --- a/lib/gitdocs/manager.rb +++ b/lib/gitdocs/manager.rb @@ -30,88 +30,33 @@ def start(host, port) "Using configuration root: '#{Initializer.root_dirname}'" ) - Celluloid.boot unless Celluloid.running? - @supervisor = Celluloid::SupervisionGroup.run! - - # Start the web server ################################################### - app = - Rack::Builder.new do - use Rack::Static, - urls: %w(/css /js /img /doc), - root: File.expand_path('../public', __FILE__) - use Rack::MethodOverride - map('/settings') { run SettingsApp } - map('/') { run BrowserApp } - end - @supervisor.add( - Reel::Rack::Server, - as: :reel_rack_server, - args: [ - app, - { - Host: host, - Port: port, - quiet: true - } - ] - ) - - # Start the synchronizers ################################################ - @synchronization_supervisor = Celluloid::SupervisionGroup.run! - Share.all.each do |share| - @synchronization_supervisor.add( - Synchronizer, as: share.id.to_s, args: [share] + @celluloid = Gitdocs::CelluloidFascade.new(host, port) + + begin + @celluloid.start + # ... and wait ########################################################### + sleep + rescue Restart + Gitdocs.log_info('Restarting actors...') + @celluloid.terminate + retry + rescue Interrupt, SystemExit + Gitdocs.log_info('Interrupt received...') + @celluloid.terminate + rescue Exception => e # rubocop:disable RescueException + Gitdocs.log_error("#{e.inspect} - #{e.message}") + Gitdocs.log_error(e.backtrace.join("\n")) + Notifier.error( + 'Unexpected exit', + 'Something went wrong. Please see the log for details.' ) - end - - # Start the repository listeners ######################################### - @listener = - Listen.to( - *Share.paths, - ignore: /#{File::SEPARATOR}\.git#{File::SEPARATOR}/ - ) do |modified, added, removed| - all_changes = modified + added + removed - changed_repository_paths = - Share.paths.select do |directory| - all_changes.any? { |x| x.start_with?(directory) } - end - - changed_repository_paths.each do |directory| - actor_id = Share.find_by_path(directory).id.to_s - Celluloid::Actor[actor_id].async.synchronize - end - end - @listener.start - - # ... and wait ########################################################### - sleep + @celluloid.terminate - rescue Interrupt, SystemExit - Gitdocs.log_info('Interrupt received...') - rescue Exception => e # rubocop:disable RescueException - Gitdocs.log_error( - "#{e.class.inspect} - #{e.inspect} - #{e.message.inspect}" - ) - Gitdocs.log_error(e.backtrace.join("\n")) - Notifier.error( - 'Unexpected exit', - 'Something went wrong. Please see the log for details.' - ) - raise - ensure - Gitdocs.log_info('stopping listeners...') - @listener.stop if @listener - - Gitdocs.log_info('stopping synchronizers...') - @synchronization_supervisor.terminate if @synchronization_supervisor - - Gitdocs.log_info('terminate supervisor...') - @supervisor.terminate if @supervisor - - Gitdocs.log_info('disconnect notifier...') - Notifier.disconnect - - Gitdocs.log_info("Gitdocs is terminating...goodbye\n\n") + raise + ensure + Notifier.disconnect + Gitdocs.log_info("Gitdocs is terminating...goodbye\n\n") + end end end end diff --git a/test/integration/test_helper.rb b/test/integration/test_helper.rb index 95841f3..d5aaf43 100644 --- a/test/integration/test_helper.rb +++ b/test/integration/test_helper.rb @@ -89,6 +89,12 @@ def before_setup end def teardown + # Verify that there are not errors in the log file. + log_filename = File.join(abs_current_dir, '.gitdocs', 'log') + refute( + File.read(log_filename).include?('ERROR'), 'Unexpected ERROR in log' + ) if File.exist?(log_filename) + Capybara.reset_sessions! Capybara.use_default_driver end diff --git a/test/unit/celluloid_facade_test.rb b/test/unit/celluloid_facade_test.rb new file mode 100644 index 0000000..de95e44 --- /dev/null +++ b/test/unit/celluloid_facade_test.rb @@ -0,0 +1,11 @@ +# -*- encoding : utf-8 -*- + +require File.expand_path('../test_helper', __FILE__) + +describe 'Gitdocs::CelluloidFacade' do + let(:celluloid_facade) { Gitdocs::CelluloidFacade.new(:host, :port) } + + # TODO: describe '#start' do + + # TODO: describe '#terminate' do +end diff --git a/test/unit/manager_test.rb b/test/unit/manager_test.rb index c45399c..b437cc1 100644 --- a/test/unit/manager_test.rb +++ b/test/unit/manager_test.rb @@ -14,7 +14,16 @@ it { subject } end - # TODO: describe '.restart_synchronization' do + describe '.restart_synchronization' do + subject { Gitdocs::Manager.restart_synchronization } + + before do + Thread.expects(:main).returns(thread = mock) + thread.expects(:raise).with(Gitdocs::Restart, 'restarting ... ') + end + + it { subject } + end describe '.listen_method' do subject { Gitdocs::Manager.listen_method } @@ -32,5 +41,90 @@ end end - # TODO: describe '.start' + let(:manager) { Gitdocs::Manager.new } + + describe '.start' do + subject { manager.start(:host, :port) } + + let(:celluloid_fascade) { mock } + before do + Gitdocs::Initializer.stubs(:root_dirname).returns(:root_dirname) + Gitdocs.expects(:log_info).with("Starting Gitdocs v#{Gitdocs::VERSION}...") + Gitdocs.expects(:log_info).with("Using configuration root: 'root_dirname'") + + Gitdocs::CelluloidFascade.expects(:new) + .with(:host, :port) + .returns(celluloid_fascade) +# celluloid_fascade.expects(:start) +# manager.expects(:sleep).raises(expected_exception) + end + + describe 'restart' do + before do + celluloid_fascade.expects(:start).twice + celluloid_fascade.expects(:terminate)#.twice + manager.stubs(:sleep).raises(Gitdocs::Restart).then.returns(:result) + + Gitdocs.expects(:log_info).with('Restarting actors...') + Gitdocs::Notifier.expects(:disconnect) + Gitdocs.expects(:log_info).with("Gitdocs is terminating...goodbye\n\n") + end + + it { subject.must_equal(:result) } + end + + describe 'exit' do + before do + celluloid_fascade.expects(:start) + manager.expects(:sleep).raises(expected_exception) + end + + describe 'Interrupt' do + let(:expected_exception) { Interrupt } + + before do + Gitdocs.expects(:log_info).with('Interrupt received...') + celluloid_fascade.expects(:terminate) + Gitdocs::Notifier.expects(:disconnect) + Gitdocs.expects(:log_info).with("Gitdocs is terminating...goodbye\n\n") + end + + it { subject } + end + + describe 'SystemExit' do + let(:expected_exception) { SystemExit } + + before do + Gitdocs.expects(:log_info).with('Interrupt received...') + celluloid_fascade.expects(:terminate) + Gitdocs::Notifier.expects(:disconnect) + Gitdocs.expects(:log_info).with("Gitdocs is terminating...goodbye\n\n") + end + + it { subject } + end + + describe 'unexpected Exception' do + let(:expected_exception) { Exception.new } + + before do + expected_exception.stubs(:backtrace).returns(%w(foo bar)) + expected_exception.stubs(:message).returns(:message) + + Gitdocs.expects(:log_error).with("#{expected_exception.inspect} - message") + Gitdocs.expects(:log_error).with("foo\nbar") + Gitdocs::Notifier.expects(:error).with( + 'Unexpected exit', + 'Something went wrong. Please see the log for details.' + ) + celluloid_fascade.expects(:terminate) + Gitdocs::Notifier.expects(:disconnect) + Gitdocs.expects(:log_info).with("Gitdocs is terminating...goodbye\n\n") + end + + it { proc { subject }.must_raise(Exception) } + end + end + end end From 78b2c8837f3e8500f198bad92d344f1f8e06a03b Mon Sep 17 00:00:00 2001 From: Andrew Sullivan Cant Date: Fri, 14 Apr 2017 13:58:29 -0400 Subject: [PATCH 2/2] Cleaning up some warnings --- lib/gitdocs.rb | 3 +-- lib/gitdocs/browser_app.rb | 4 ++-- lib/gitdocs/cli.rb | 4 ++-- test/unit/notifier_test.rb | 5 ++++- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/gitdocs.rb b/lib/gitdocs.rb index 00c9287..7cddb7c 100644 --- a/lib/gitdocs.rb +++ b/lib/gitdocs.rb @@ -65,8 +65,6 @@ def self.log_error(message) ############################################################################## - private_class_method - # @return [void] def self.init_log return if @initialized @@ -77,4 +75,5 @@ def self.init_log Celluloid.logger.level = Initializer.verbose ? Logger::DEBUG : Logger::INFO @initialized = true end + private_class_method :init_log end diff --git a/lib/gitdocs/browser_app.rb b/lib/gitdocs/browser_app.rb index b7a544e..a4ec041 100644 --- a/lib/gitdocs/browser_app.rb +++ b/lib/gitdocs/browser_app.rb @@ -1,7 +1,7 @@ # -*- encoding : utf-8 -*- require 'sinatra/base' -require 'uri' +require 'cgi' require 'haml' require 'mimetype_fu' require 'gitdocs/rendering_helper' @@ -27,7 +27,7 @@ def repository def path halt(404) unless repository @path ||= Repository::Path.new( - repository, URI.decode(params[:splat].first) + repository, CGI.unescape(params[:splat].first) ) end end diff --git a/lib/gitdocs/cli.rb b/lib/gitdocs/cli.rb index 6e9e345..09259ab 100644 --- a/lib/gitdocs/cli.rb +++ b/lib/gitdocs/cli.rb @@ -67,7 +67,7 @@ def restart method_option :pid, type: :string, aliases: '-P' method_option :interval, type: :numeric, aliases: '-i', default: 15 method_option :notification, type: :boolean, aliases: '-n', default: true - method_option :sync, type: :boolean, aliases: '-s', default: 'full' + method_option :sync, type: :string, aliases: '-s', default: 'full', enum: %w(full fetch) def add(path) Share.create_by_path!( normalize_path(path), @@ -83,7 +83,7 @@ def add(path) method_option :pid, type: :string, aliases: '-P' method_option :interval, type: :numeric, aliases: '-i', default: 15 method_option :notification, type: :boolean, aliases: '-n', default: true - method_option :sync, type: :boolean, aliases: '-s', default: 'full' + method_option :sync, type: :string, aliases: '-s', default: 'full', enum: %w(full fetch) def create(path, remote) Repository.clone(path, remote) Share.create_by_path!( diff --git a/test/unit/notifier_test.rb b/test/unit/notifier_test.rb index fdbb0d0..1a4d881 100644 --- a/test/unit/notifier_test.rb +++ b/test/unit/notifier_test.rb @@ -84,7 +84,10 @@ subject { notifier.disconnect } describe 'not connected' do - before { subject } + before do + notifier.instance_variable_set(:@notifier, nil) + subject + end it { notifier.instance_variable_get(:@notifier).must_equal(nil) } end