From 3acea82ac1d8452521614ae948847a3f639f7ca1 Mon Sep 17 00:00:00 2001 From: Mario Kostelac Date: Wed, 8 Jun 2016 14:51:30 +0200 Subject: [PATCH 1/4] Fix daemonization broken in #219 --- lib/shoryuken/cli.rb | 36 +++++++++++-------- spec/shoryuken/cli_spec.rb | 72 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 15 deletions(-) create mode 100644 spec/shoryuken/cli_spec.rb diff --git a/lib/shoryuken/cli.rb b/lib/shoryuken/cli.rb index 521bb591..b903a0c5 100644 --- a/lib/shoryuken/cli.rb +++ b/lib/shoryuken/cli.rb @@ -31,13 +31,13 @@ def run(args) options = parse_cli_args(args) - daemonize - write_pid - - load_celluloid + daemonize(options) + write_pid(options) EnvironmentLoader.load(options) + load_celluloid + require 'shoryuken/launcher' @launcher = Shoryuken::Launcher.new @@ -64,21 +64,27 @@ def run(args) private def load_celluloid - raise "Celluloid cannot be required until here, or it will break Shoryuken's daemonization" if defined?(::Celluloid) && Shoryuken.options[:daemon] - - # Celluloid can't be loaded until after we've daemonized - # because it spins up threads and creates locks which get - # into a very bad state if forked. require 'celluloid/autostart' Celluloid.logger = (Shoryuken.options[:verbose] ? Shoryuken.logger : nil) require 'shoryuken/manager' end - def daemonize - return unless Shoryuken.options[:daemon] + def celluloid_loaded? + defined?(::Celluloid) + end + + def daemonize(options) + return unless options[:daemon] - fail ArgumentError, "You really should set a logfile if you're going to daemonize" unless Shoryuken.options[:logfile] + fail ArgumentError, "You really should set a logfile if you're going to daemonize" unless options[:logfile] + + if celluloid_loaded? + # Celluloid can't be loaded until after we've daemonized + # because it spins up threads and creates locks which get + # into a very bad state if forked. + fail RuntimeError, "Celluloid cannot be required until here, or it will break Shoryuken's daemonization" + end files_to_reopen = [] ObjectSpace.each_object(File) do |file| @@ -96,7 +102,7 @@ def daemonize end [$stdout, $stderr].each do |io| - File.open(Shoryuken.options[:logfile], 'ab') do |f| + File.open(options[:logfile], 'ab') do |f| io.reopen(f) end io.sync = true @@ -104,8 +110,8 @@ def daemonize $stdin.reopen('/dev/null') end - def write_pid - if (path = Shoryuken.options[:pidfile]) + def write_pid(options) + if (path = options[:pidfile]) File.open(path, 'w') do |f| f.puts Process.pid end diff --git a/spec/shoryuken/cli_spec.rb b/spec/shoryuken/cli_spec.rb new file mode 100644 index 00000000..8ba9eec6 --- /dev/null +++ b/spec/shoryuken/cli_spec.rb @@ -0,0 +1,72 @@ +require 'spec_helper' +require 'shoryuken/cli' +require 'shoryuken/launcher' + +RSpec.describe Shoryuken::CLI do + + let(:cli) { Shoryuken::CLI.instance } + + before(:each) do + # make sure we do not bail + allow(cli).to receive(:exit) + + # make sure we do not mess with standard streams + allow_any_instance_of(IO).to receive(:reopen) + end + + describe '#run' do + + let(:launcher) { instance_double('Shoryuken::Launcher') } + + before(:each) do + allow(Shoryuken::Launcher).to receive(:new).and_return(launcher) + allow(launcher).to receive(:run).and_raise(Interrupt) + allow(launcher).to receive(:stop) + end + + it 'does not raise' do + expect{ cli.run([]) }.to_not raise_error + end + + it 'daemonizes with --daemon --logfile' do + expect(cli).to receive(:celluloid_loaded?).and_return(false) + expect(Process).to receive(:daemon) + cli.run(['--daemon', '--logfile', '/dev/null']) + end + + it 'does NOT daemonize with --daemon --logfile' do + expect(Process).to_not receive(:daemon) + cli.run(['--logfile', '/dev/null']) + end + + it 'writes PID file with --pidfile' do + pidfile = instance_double('File') + expect(File).to receive(:open).with('/dev/null', 'w').and_yield(pidfile) + expect(pidfile).to receive(:puts).with(Process.pid) + cli.run(['--pidfile', '/dev/null']) + end + end + + describe '#daemonize' do + + before(:each) do + allow(cli).to receive(:celluloid_loaded?).and_return(false) + end + + it 'raises if logfile is not set' do + expect{ cli.send(:daemonize, { daemon: true }) }.to raise_error(ArgumentError) + end + + it 'raises if Celluloid is already loaded' do + expect(cli).to receive(:celluloid_loaded?).and_return(true) + args = { daemon: true, logfile: '/dev/null' } + expect{ cli.send(:daemonize, args) }.to raise_error(RuntimeError) + end + + it 'calls Process.daemon' do + args = { daemon: true, logfile: '/dev/null' } + expect(Process).to receive(:daemon).with(true, true) + cli.send(:daemonize, args) + end + end +end From e7330148bd3dce98df156d8424e23b502800c434 Mon Sep 17 00:00:00 2001 From: Mario Kostelac Date: Wed, 8 Jun 2016 14:54:59 +0200 Subject: [PATCH 2/4] Fix style --- spec/shoryuken/cli_spec.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/spec/shoryuken/cli_spec.rb b/spec/shoryuken/cli_spec.rb index 8ba9eec6..f6141ff5 100644 --- a/spec/shoryuken/cli_spec.rb +++ b/spec/shoryuken/cli_spec.rb @@ -3,7 +3,6 @@ require 'shoryuken/launcher' RSpec.describe Shoryuken::CLI do - let(:cli) { Shoryuken::CLI.instance } before(:each) do @@ -15,7 +14,6 @@ end describe '#run' do - let(:launcher) { instance_double('Shoryuken::Launcher') } before(:each) do @@ -25,7 +23,7 @@ end it 'does not raise' do - expect{ cli.run([]) }.to_not raise_error + expect { cli.run([]) }.to_not raise_error end it 'daemonizes with --daemon --logfile' do @@ -48,13 +46,12 @@ end describe '#daemonize' do - before(:each) do allow(cli).to receive(:celluloid_loaded?).and_return(false) end it 'raises if logfile is not set' do - expect{ cli.send(:daemonize, { daemon: true }) }.to raise_error(ArgumentError) + expect { cli.send(:daemonize, { daemon: true }) }.to raise_error(ArgumentError) end it 'raises if Celluloid is already loaded' do From d0c4dda330c7454d4afa6136795880f3cca26430 Mon Sep 17 00:00:00 2001 From: Mario Kostelac Date: Wed, 8 Jun 2016 14:56:15 +0200 Subject: [PATCH 3/4] Fix style no 2 --- spec/shoryuken/cli_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/shoryuken/cli_spec.rb b/spec/shoryuken/cli_spec.rb index f6141ff5..964807f5 100644 --- a/spec/shoryuken/cli_spec.rb +++ b/spec/shoryuken/cli_spec.rb @@ -51,7 +51,7 @@ end it 'raises if logfile is not set' do - expect { cli.send(:daemonize, { daemon: true }) }.to raise_error(ArgumentError) + expect { cli.send(:daemonize, daemon: true) }.to raise_error(ArgumentError) end it 'raises if Celluloid is already loaded' do From c7659ef4eb1f75740257fc34dc7c450efb476a1c Mon Sep 17 00:00:00 2001 From: Mario Kostelac Date: Wed, 8 Jun 2016 14:58:11 +0200 Subject: [PATCH 4/4] Fix style no 3 --- lib/shoryuken/cli.rb | 2 +- spec/shoryuken/cli_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/shoryuken/cli.rb b/lib/shoryuken/cli.rb index b903a0c5..e70b6a62 100644 --- a/lib/shoryuken/cli.rb +++ b/lib/shoryuken/cli.rb @@ -83,7 +83,7 @@ def daemonize(options) # Celluloid can't be loaded until after we've daemonized # because it spins up threads and creates locks which get # into a very bad state if forked. - fail RuntimeError, "Celluloid cannot be required until here, or it will break Shoryuken's daemonization" + raise "Celluloid cannot be required until here, or it will break Shoryuken's daemonization" end files_to_reopen = [] diff --git a/spec/shoryuken/cli_spec.rb b/spec/shoryuken/cli_spec.rb index 964807f5..f6f4cb80 100644 --- a/spec/shoryuken/cli_spec.rb +++ b/spec/shoryuken/cli_spec.rb @@ -57,7 +57,7 @@ it 'raises if Celluloid is already loaded' do expect(cli).to receive(:celluloid_loaded?).and_return(true) args = { daemon: true, logfile: '/dev/null' } - expect{ cli.send(:daemonize, args) }.to raise_error(RuntimeError) + expect { cli.send(:daemonize, args) }.to raise_error(RuntimeError) end it 'calls Process.daemon' do