+1 with one inlined and one outlined questions On Mon, 2010-10-18 at 16:04 -0700, Markus Roberts wrote: > The Puppet::Util.sync method was not thread safe and also leaked memory. I'm > not certain, but I believe the first is ironic and the second is merely a bug.
:) > This patch addresses the problem by 1) refactoring so the sync objects > are never returned (and thus no one can cache a reference to one) 2) adding > reference counting 3) deleting them when they are no longer needed 4) doing > the thread safty dance. > > It wasn't the first (or even second) solution considered, but it's the one > that I was able to make work in a way that I'm convinced is correct. Its > main advantage is that it puts all the tricky bits in one place. Quick ruby question: what about using a WeakRef to hold the sync object and completely get rid of the ref counting? Would that work? And I know testing threading code is always hard but all the refcounting stuff would benefit from at least a unit test. If possible having an integration test to check for the thread safeness of the code would be terrific :) > Signed-off-by: Markus Roberts <[email protected]> > --- > lib/puppet/daemon.rb | 4 ++-- > lib/puppet/network/server.rb | 4 ++-- > lib/puppet/util.rb | 25 ++++++++++++++++--------- > lib/puppet/util/file_locking.rb | 4 ++-- > spec/unit/daemon_spec.rb | 12 +++--------- > spec/unit/network/server_spec.rb | 13 +++---------- > spec/unit/util/file_locking_spec.rb | 26 ++++++-------------------- > 7 files changed, 34 insertions(+), 54 deletions(-) > > diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb > index ad0edd3..c76d63a 100755 > --- a/lib/puppet/daemon.rb > +++ b/lib/puppet/daemon.rb > @@ -43,7 +43,7 @@ class Puppet::Daemon > # Create a pidfile for our daemon, so we can be stopped and others > # don't try to start. > def create_pidfile > - Puppet::Util.sync(Puppet[:name]).synchronize(Sync::EX) do > + Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do > raise "Could not create PID file: #{pidfile}" unless > Puppet::Util::Pidlock.new(pidfile).lock > end > end > @@ -73,7 +73,7 @@ class Puppet::Daemon > > # Remove the pid file for our daemon. > def remove_pidfile > - Puppet::Util.sync(Puppet[:name]).synchronize(Sync::EX) do > + Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do > locker = Puppet::Util::Pidlock.new(pidfile) > locker.unlock or Puppet.err "Could not remove PID file #{pidfile}" if > locker.locked? > end > diff --git a/lib/puppet/network/server.rb b/lib/puppet/network/server.rb > index d424b9c..e4de07d 100644 > --- a/lib/puppet/network/server.rb > +++ b/lib/puppet/network/server.rb > @@ -32,14 +32,14 @@ class Puppet::Network::Server > # Create a pidfile for our daemon, so we can be stopped and others > # don't try to start. > def create_pidfile > - Puppet::Util.sync(Puppet[:name]).synchronize(Sync::EX) do > + Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do > raise "Could not create PID file: #{pidfile}" unless > Puppet::Util::Pidlock.new(pidfile).lock > end > end > > # Remove the pid file for our daemon. > def remove_pidfile > - Puppet::Util.sync(Puppet[:name]).synchronize(Sync::EX) do > + Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do > locker = Puppet::Util::Pidlock.new(pidfile) > locker.unlock or Puppet.err "Could not remove PID file #{pidfile}" if > locker.locked? > end > diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb > index f2eaf0d..1a5acaf 100644 > --- a/lib/puppet/util.rb > +++ b/lib/puppet/util.rb > @@ -3,6 +3,7 @@ > require 'puppet/util/monkey_patches' > require 'sync' > require 'puppet/external/lock' > +require 'monitor' > > module Puppet > # A command failed to execute. > @@ -17,8 +18,7 @@ module Util > require 'puppet/util/posix' > extend Puppet::Util::POSIX > > - # Create a hash to store the different sync objects. > - @@syncresources = {} > + @@sync_objects = {}.extend MonitorMixin > > def self.activerecord_version > if (defined?(::ActiveRecord) and defined?(::ActiveRecord::VERSION) and > defined?(::ActiveRecord::VERSION::MAJOR) and > defined?(::ActiveRecord::VERSION::MINOR)) > @@ -28,10 +28,19 @@ module Util > end > end > > - # Return the sync object associated with a given resource. > - def self.sync(resource) > - @@syncresources[resource] ||= Sync.new > - @@syncresources[resource] > + > + def self.synchronize_on(x,type) > + sync_object,users = 0,1 Why use variables here for constants? Doesn't it consume stack space for nothing useful? > + begin > + @@sync_objects.synchronize { > + (@@sync_objects[x] ||= [Sync.new,0])[users] += 1 > + } > + @@sync_objects[x][sync_object].synchronize(type) { yield } > + ensure > + @@sync_objects.synchronize { > + @@sync_objects.delete(x) unless (@@sync_objects[x][users] -= 1) > 0 > + } > + end > end > > # Change the process to a different user > @@ -359,9 +368,7 @@ module Util > > # Create an exclusive lock. > def threadlock(resource, type = Sync::EX) > - Puppet::Util.sync(resource).synchronize(type) do > - yield > - end > + Puppet::Util.synchronize_on(resource,type) { yield } > end > > # Because some modules provide their own version of this method. > diff --git a/lib/puppet/util/file_locking.rb b/lib/puppet/util/file_locking.rb > index 8b194ed..4cc2ae9 100644 > --- a/lib/puppet/util/file_locking.rb > +++ b/lib/puppet/util/file_locking.rb > @@ -6,7 +6,7 @@ module Puppet::Util::FileLocking > # Create a shared lock for reading > def readlock(file) > raise ArgumentError, "#{file} is not a file" unless !File.exists?(file) > or File.file?(file) > - Puppet::Util.sync(file).synchronize(Sync::SH) do > + Puppet::Util.synchronize_on(file,Sync::SH) do > File.open(file) { |f| > f.lock_shared { |lf| yield lf } > } > @@ -33,7 +33,7 @@ module Puppet::Util::FileLocking > end > end > > - Puppet::Util.sync(file).synchronize(Sync::EX) do > + Puppet::Util.synchronize_on(file,Sync::EX) do > File.open(file, "w", mode) do |rf| > rf.lock_exclusive do |lrf| > yield lrf > diff --git a/spec/unit/daemon_spec.rb b/spec/unit/daemon_spec.rb > index 1532073..e24db78 100755 > --- a/spec/unit/daemon_spec.rb > +++ b/spec/unit/daemon_spec.rb > @@ -1,4 +1,4 @@ > -#!/usr/bin/env ruby" > +#!/usr/bin/env ruby > > require File.dirname(__FILE__) + '/../spec_helper' > require 'puppet/daemon' > @@ -142,11 +142,7 @@ describe Puppet::Daemon do > describe "when creating its pidfile" do > it "should use an exclusive mutex" do > Puppet.settings.expects(:value).with(:name).returns "me" > - > - sync = mock 'sync' > - Puppet::Util.expects(:sync).with("me").returns sync > - > - sync.expects(:synchronize).with(Sync::EX) > + Puppet::Util.expects(:synchronize_on).with("me",Sync::EX) > @daemon.create_pidfile > end > > @@ -180,10 +176,8 @@ describe Puppet::Daemon do > it "should use an exclusive mutex" do > Puppet.settings.expects(:value).with(:name).returns "me" > > - sync = mock 'sync' > - Puppet::Util.expects(:sync).with("me").returns sync > + Puppet::Util.expects(:synchronize_on).with("me",Sync::EX) > > - sync.expects(:synchronize).with(Sync::EX) > @daemon.remove_pidfile > end > > diff --git a/spec/unit/network/server_spec.rb > b/spec/unit/network/server_spec.rb > index ccd9c08..c2496dc 100755 > --- a/spec/unit/network/server_spec.rb > +++ b/spec/unit/network/server_spec.rb > @@ -5,6 +5,7 @@ > > require File.dirname(__FILE__) + '/../../spec_helper' > require 'puppet/network/server' > +require 'puppet/network/handler' > > describe Puppet::Network::Server do > before do > @@ -161,11 +162,7 @@ describe Puppet::Network::Server do > describe "when creating its pidfile" do > it "should use an exclusive mutex" do > Puppet.settings.expects(:value).with(:name).returns "me" > - > - sync = mock 'sync' > - Puppet::Util.expects(:sync).with("me").returns sync > - > - sync.expects(:synchronize).with(Sync::EX) > + Puppet::Util.expects(:synchronize_on).with("me",Sync::EX) > @server.create_pidfile > end > > @@ -198,11 +195,7 @@ describe Puppet::Network::Server do > describe "when removing its pidfile" do > it "should use an exclusive mutex" do > Puppet.settings.expects(:value).with(:name).returns "me" > - > - sync = mock 'sync' > - Puppet::Util.expects(:sync).with("me").returns sync > - > - sync.expects(:synchronize).with(Sync::EX) > + Puppet::Util.expects(:synchronize_on).with("me",Sync::EX) > @server.remove_pidfile > end > > diff --git a/spec/unit/util/file_locking_spec.rb > b/spec/unit/util/file_locking_spec.rb > index 1005106..66e3a6f 100755 > --- a/spec/unit/util/file_locking_spec.rb > +++ b/spec/unit/util/file_locking_spec.rb > @@ -32,17 +32,12 @@ describe Puppet::Util::FileLocking do > end > > it "should use a global shared mutex" do > - @sync = mock 'sync' > - @sync.expects(:synchronize).with(Sync::SH).once > - Puppet::Util.expects(:sync).with('/file').returns @sync > - > + Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).once > Puppet::Util::FileLocking.readlock '/file' > end > > it "should use a shared lock on the file" do > - @sync = mock 'sync' > - @sync.stubs(:synchronize).yields > - Puppet::Util.expects(:sync).with('/file').returns @sync > + Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).yields > > fh = mock 'filehandle' > File.expects(:open).with("/file").yields fh > @@ -59,9 +54,7 @@ describe Puppet::Util::FileLocking do > end > > it "should create missing files" do > - @sync = mock 'sync' > - @sync.stubs(:synchronize).yields > - Puppet::Util.expects(:sync).with('/file').returns @sync > + Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).yields > > File.expects(:exists?).with('/file').returns false > File.expects(:open).with('/file').once > @@ -72,9 +65,7 @@ describe Puppet::Util::FileLocking do > > describe "when acquiring a write lock" do > before do > - @sync = mock 'sync' > - Puppet::Util.stubs(:sync).returns @sync > - @sync.stubs(:synchronize).yields > + Puppet::Util.stubs(:synchronize_on).yields > File.stubs(:file?).with('/file').returns true > File.stubs(:exists?).with('/file').returns true > end > @@ -88,10 +79,7 @@ describe Puppet::Util::FileLocking do > end > > it "should use a global exclusive mutex" do > - sync = mock 'sync' > - sync.expects(:synchronize).with(Sync::EX) > - Puppet::Util.expects(:sync).with("/file").returns sync > - > + Puppet::Util.expects(:synchronize_on).with("/file",Sync::EX) > Puppet::Util::FileLocking.writelock '/file' > end > > @@ -143,9 +131,7 @@ describe Puppet::Util::FileLocking do > end > > it "should create missing files" do > - @sync = mock 'sync' > - @sync.stubs(:synchronize).yields > - Puppet::Util.expects(:sync).with('/file').returns @sync > + Puppet::Util.expects(:synchronize_on).with('/file',Sync::EX).yields > > File.expects(:exists?).with('/file').returns false > File.expects(:open).with('/file', 'w', 0600).once > -- > 1.7.0.4 > -- 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.
