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.

Sign up for GitHub

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

Jump to bottom

(PUP-1635) Make daemon.rb signals Ruby 2.x safe #3586

Merged
merged 1 commit into from Feb 25, 2015
Merged

(PUP-1635) Make daemon.rb signals Ruby 2.x safe #3586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 13 additions & 4 deletions lib/puppet/daemon.rb
View file
Open in desktop
Expand Up @@ -21,11 +21,14 @@
#
# @api private
class Puppet::Daemon
SIGNAL_CHECK_INTERVAL = 5

attr_accessor :agent, :server, :argv

def initialize(pidfile, scheduler = Puppet::Scheduler::Scheduler.new())
@scheduler = scheduler
@pidfile = pidfile
@signals = []
end

def daemonname
Expand Down Expand Up @@ -109,8 +112,8 @@ def set_signal_traps
signals.update({:HUP => :restart, :USR1 => :reload, :USR2 => :reopen_logs }) unless Puppet.features.microsoft_windows?
signals.each do |signal, method|
Signal.trap(signal) do
Puppet.notice "Caught #{signal}; calling #{method}"
send(method)
Puppet.notice "Caught #{signal}; storing #{method}"
@signals << method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't thread safe and could lead to a signal being lost. The signal handler and the consumer threads need to synchronize access to the shared Array. In practice that won't happen with MRI ruby because of its GIL. But it could happen on other ruby implementations, e.g. JRuby.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep we came to the same conclusion in conversation lost in the default github view because of a subsequent push :)

end
end
end
Expand Down Expand Up @@ -173,10 +176,16 @@ def run_event_loop
end
end

signal_loop = Puppet::Scheduler.create_job(SIGNAL_CHECK_INTERVAL) do
while method = @signals.shift
Puppet.notice "Processing #{method}"
send(method)
end
end

reparse_run.disable if Puppet[:filetimeout] == 0
agent_run.disable unless agent

@scheduler.run_loop([reparse_run, agent_run])
@scheduler.run_loop([reparse_run, agent_run, signal_loop])
end
end

2 changes: 0 additions & 2 deletions spec/unit/daemon_spec.rb
View file
Open in desktop
Expand Up @@ -53,8 +53,6 @@ def run_loop(jobs)

Puppet.expects(:notice)

daemon.expects(method)

daemon.set_signal_traps
end
end
Expand Down