Please review pull request #174: Facter freeze fix opened by (timurbatyrshin)
Description:
I've stumbled recently on puppet hanging occasionally in Ubuntu 10.04 run on t1.micro x64 EC2 instace.
Say, 1 out of 3 complex and long puppet apply runs got frozen with process zombified aside.
While hanging it was impossible to terminate puppet through usual Ctrl-C however it honored SIGTERM.
Tracing back the freeze have lead to this code. After removing Thread::exclusive section the freezes disappeared. These lines were introduced through a series of commits:
https://github.com/puppetlabs/facter/commit/c2aa5086ab55da9c708d962b84a1b85404fc6329 (for processor.rb)
https://github.com/puppetlabs/facter/commit/a633aebab4dc4d07119a619c21cad6a719552083 (for memory.rb)
I see no reason for running the code here in exclusive threads as it only does simple file read and calculation using local variables and does not address anything outside its own block. The original commits neither do clarify the reason for using exclusive threads here.
- Opened: Mon Feb 20 12:22:09 UTC 2012
- Based on: puppetlabs:master (6a59c77117d8acee595565764554dad2796eea36)
- Requested merge: timurbatyrshin:facter-freeze-fix (9ff4453b2f0843df887ae1ec2098a4336291df0c)
Diff follows:
diff --git a/lib/facter/util/memory.rb b/lib/facter/util/memory.rb
index 09a8166..96d8b4c 100644
--- a/lib/facter/util/memory.rb
+++ b/lib/facter/util/memory.rb
@@ -3,23 +3,19 @@
##
module Facter::Memory
- require 'thread'
-
def self.meminfo_number(tag)
memsize = ""
- Thread::exclusive do
- size, scale = [0, ""]
- File.readlines("/proc/meminfo").each do |l|
- size, scale = [$1.to_f, $2] if l =~ /^#{tag}:\s+(\d+)\s+(\S+)/
- # MemoryFree == memfree + cached + buffers
- # (assume scales are all the same as memfree)
- if tag == "MemFree" &&
- l =~ /^(?:Buffers|Cached):\s+(\d+)\s+(?:\S+)/
- size += $1.to_f
- end
+ size, scale = [0, ""]
+ File.readlines("/proc/meminfo").each do |l|
+ size, scale = [$1.to_f, $2] if l =~ /^#{tag}:\s+(\d+)\s+(\S+)/
+ # MemoryFree == memfree + cached + buffers
+ # (assume scales are all the same as memfree)
+ if tag == "MemFree" &&
+ l =~ /^(?:Buffers|Cached):\s+(\d+)\s+(?:\S+)/
+ size += $1.to_f
end
- memsize = scale_number(size, scale)
end
+ memsize = scale_number(size, scale)
memsize
end
diff --git a/lib/facter/util/processor.rb b/lib/facter/util/processor.rb
index 220b209..fd4c4e8 100644
--- a/lib/facter/util/processor.rb
+++ b/lib/facter/util/processor.rb
@@ -8,55 +8,47 @@ def self.enum_cpuinfo
model = Facter.value(:architecture)
case model
when "x86_64", "amd64", "i386", /parisc/, "hppa", "ia64"
- Thread::exclusive do
- File.readlines(cpuinfo).each do |l|
- if l =~ /processor\s+:\s+(\d+)/
- processor_num = $1.to_i
- elsif l =~ /model name\s+:\s+(.*)\s*$/
- processor_list[processor_num] = $1 unless processor_num == -1
- processor_num = -1
- elsif l =~ /processor\s+(\d+):\s+(.*)/
- processor_num = $1.to_i
- processor_list[processor_num] = $2 unless processor_num == -1
- end
+ File.readlines(cpuinfo).each do |l|
+ if l =~ /processor\s+:\s+(\d+)/
+ processor_num = $1.to_i
+ elsif l =~ /model name\s+:\s+(.*)\s*$/
+ processor_list[processor_num] = $1 unless processor_num == -1
+ processor_num = -1
+ elsif l =~ /processor\s+(\d+):\s+(.*)/
+ processor_num = $1.to_i
+ processor_list[processor_num] = $2 unless processor_num == -1
end
end
when "ppc64"
- Thread::exclusive do
- File.readlines(cpuinfo).each do |l|
- if l =~ /processor\s+:\s+(\d+)/
- processor_num = $1.to_i
- elsif l =~ /cpu\s+:\s+(.*)\s*$/
- processor_list[processor_num] = $1 unless processor_num == -1
- processor_num = -1
- end
+ File.readlines(cpuinfo).each do |l|
+ if l =~ /processor\s+:\s+(\d+)/
+ processor_num = $1.to_i
+ elsif l =~ /cpu\s+:\s+(.*)\s*$/
+ processor_list[processor_num] = $1 unless processor_num == -1
+ processor_num = -1
end
end
when /arm/
- Thread::exclusive do
- File.readlines(cpuinfo).each do |l|
- if l =~ /Processor\s+:\s+(.+)/
+ File.readlines(cpuinfo).each do |l|
+ if l =~ /Processor\s+:\s+(.+)/
+ processor_num += 1
+ processor_list[processor_num] = $1.chomp
+ elsif l =~ /processor\s+:\s+(\d+)\s*$/
+ proc_num = $1.to_i
+ if proc_num != 0
processor_num += 1
- processor_list[processor_num] = $1.chomp
- elsif l =~ /processor\s+:\s+(\d+)\s*$/
- proc_num = $1.to_i
- if proc_num != 0
- processor_num += 1
- processor_list[processor_num] = processor_list[processor_num-1]
- end
+ processor_list[processor_num] = processor_list[processor_num-1]
end
end
end
when /sparc/
- Thread::exclusive do
- File.readlines(cpuinfo).each do |l|
- if l =~ /cpu\s+:\s+(.*)\s*$/
- processor_num += 1
- processor_list[processor_num] = $1
- end
+ File.readlines(cpuinfo).each do |l|
+ if l =~ /cpu\s+:\s+(.*)\s*$/
+ processor_num += 1
+ processor_list[processor_num] = $1
end
end
end
@@ -67,18 +59,16 @@ def self.enum_cpuinfo
def self.enum_lsdev
processor_num = -1
processor_list = {}
- Thread::exclusive do
- procs = Facter::Util::Resolution.exec('lsdev -Cc processor')
- if procs
- procs.each_line do |proc|
- if proc =~ /^proc(\d+)/
- processor_num = $1.to_i
- # Not retrieving the frequency since AIX 4.3.3 doesn't support the
- # attribute and some people still use the OS.
- proctype = Facter::Util::Resolution.exec('lsattr -El proc0 -a type')
- if proctype =~ /^type\s+(\S+)\s+/
- processor_list[processor_num] = $1
- end
+ procs = Facter::Util::Resolution.exec('lsdev -Cc processor')
+ if procs
+ procs.each_line do |proc|
+ if proc =~ /^proc(\d+)/
+ processor_num = $1.to_i
+ # Not retrieving the frequency since AIX 4.3.3 doesn't support the
+ # attribute and some people still use the OS.
+ proctype = Facter::Util::Resolution.exec('lsattr -El proc0 -a type')
+ if proctype =~ /^type\s+(\S+)\s+/
+ processor_list[processor_num] = $1
end
end
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
