From cfea3f9a60168702e38f4aebd54d3755aeee0b38 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 28 Jun 2018 08:01:14 -0700 Subject: [PATCH 1/4] Add simple synchronization helper to source. Use during install. When installing cookbooks using infrastructure mode and the same git source is being used for different refs, a race condition can occur where the cached respository is modified while a copy is in progress. To prevent this the source should be locked during install to prevent any unexpected modifications. --- lib/batali/command/install.rb | 44 ++++++++++++++++++----------------- lib/batali/source.rb | 11 +++++++++ 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/lib/batali/command/install.rb b/lib/batali/command/install.rb index 71d399f..9561ad3 100644 --- a/lib/batali/command/install.rb +++ b/lib/batali/command/install.rb @@ -24,27 +24,29 @@ def execute! units_slice.map do |unit| Thread.new do ui.debug "Starting unit install for: #{unit.name}<#{unit.version}>" - if unit.source.respond_to?(:cache_path) - unit.source.cache_path = cache_directory( - Bogo::Utility.snake(unit.source.class.name.split("::").last) - ) - end - asset_path = unit.source.asset - final_path = Utility.join_path(install_path, unit.name) - if infrastructure? - final_path << "-#{unit.version}" - end - begin - FileUtils.cp_r( - Utility.join_path(asset_path, "."), - final_path - ) - ui.debug "Completed unit install for: #{unit.name}<#{unit.version}>" - rescue => e - ui.debug "Failed unit install for: #{unit.name}<#{unit.version}> - #{e.class}: #{e}" - raise - ensure - unit.source.clean_asset(asset_path) + unit.source.synchronize do + if unit.source.respond_to?(:cache_path) + unit.source.cache_path = cache_directory( + Bogo::Utility.snake(unit.source.class.name.split("::").last) + ) + end + asset_path = unit.source.asset + final_path = Utility.join_path(install_path, unit.name) + if infrastructure? + final_path << "-#{unit.version}" + end + begin + FileUtils.cp_r( + Utility.join_path(asset_path, "."), + final_path + ) + ui.debug "Completed unit install for: #{unit.name}<#{unit.version}>" + rescue => e + ui.debug "Failed unit install for: #{unit.name}<#{unit.version}> - #{e.class}: #{e}" + raise + ensure + unit.source.clean_asset(asset_path) + end end end end.map(&:join) diff --git a/lib/batali/source.rb b/lib/batali/source.rb index fb4d291..a15bd85 100644 --- a/lib/batali/source.rb +++ b/lib/batali/source.rb @@ -14,10 +14,21 @@ class Source < Utility attribute :type, String, :required => true, :default => lambda { self.name } # rubocop:disable Style/RedundantSelf def initialize(args = {}) + @lock = Mutex.new @cache_path = Utility.clean_path(args.delete(:cache_path)) super end + # Helper to synchronize access to this source. + # + # @yield Block to be executed + # @return [Object] + def synchronize + @lock.synchronize do + yield + end + end + # @return [String] def unit_version raise NotImplementedError.new "Abstract class" From 456cd2ae91ae0a13377bcfe87bd90875eec3b126 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 28 Jun 2018 08:26:26 -0700 Subject: [PATCH 2/4] Fix constant name --- lib/batali/utility.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/batali/utility.rb b/lib/batali/utility.rb index 17d5a6d..33e3ff0 100644 --- a/lib/batali/utility.rb +++ b/lib/batali/utility.rb @@ -12,10 +12,10 @@ class Utility < Grimoire::Utility def self.clean_path(path) if RUBY_PLATFORM =~ /mswin|mingw|windows/ && ENV["BATALI_DISABLE_UNC"].nil? - if !path.to_s.match(/^[A-Za-z]:/) && !path.start_with?(UNC_PATH) + if !path.to_s.match(/^[A-Za-z]:/) && !path.start_with?(UNC_PREFIX) path = File.expand_path(path.to_s) end - path = UNC_PREFIX + path unless path.start_with?(UNC_PATH) + path = UNC_PREFIX + path unless path.start_with?(UNC_PREFIX) end path end From 80d0989ca211f1497bda99d94941348fe2f68b84 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Thu, 28 Jun 2018 08:38:55 -0700 Subject: [PATCH 3/4] Only adjust path if something is provided --- lib/batali/utility.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/batali/utility.rb b/lib/batali/utility.rb index 33e3ff0..4362f75 100644 --- a/lib/batali/utility.rb +++ b/lib/batali/utility.rb @@ -11,11 +11,12 @@ class Utility < Grimoire::Utility # on platform in use def self.clean_path(path) if RUBY_PLATFORM =~ /mswin|mingw|windows/ && - ENV["BATALI_DISABLE_UNC"].nil? - if !path.to_s.match(/^[A-Za-z]:/) && !path.start_with?(UNC_PREFIX) + ENV["BATALI_DISABLE_UNC"].nil? && + path + if !path.to_s.match(/^[A-Za-z]:/) && !path.to_s.start_with?(UNC_PREFIX) path = File.expand_path(path.to_s) end - path = UNC_PREFIX + path unless path.start_with?(UNC_PREFIX) + path = UNC_PREFIX + path.to_s unless path.to_s.start_with?(UNC_PREFIX) end path end From c0ec06f7c356a04c8c369fadb0d0bb43734529d8 Mon Sep 17 00:00:00 2001 From: Chris Roberts Date: Fri, 29 Jun 2018 07:36:08 -0700 Subject: [PATCH 4/4] Provide path based locking on git sources for synchronization --- lib/batali/command/install.rb | 1 + lib/batali/git.rb | 18 ++++++++++++++++++ lib/batali/source.rb | 5 +---- lib/batali/source/git.rb | 6 ++++++ 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/lib/batali/command/install.rb b/lib/batali/command/install.rb index 9561ad3..9753711 100644 --- a/lib/batali/command/install.rb +++ b/lib/batali/command/install.rb @@ -24,6 +24,7 @@ def execute! units_slice.map do |unit| Thread.new do ui.debug "Starting unit install for: #{unit.name}<#{unit.version}>" + ui.debug "Unit source: #{unit.source.inspect}" unit.source.synchronize do if unit.source.respond_to?(:cache_path) unit.source.cache_path = cache_directory( diff --git a/lib/batali/git.rb b/lib/batali/git.rb index 8d0a0cc..376db54 100644 --- a/lib/batali/git.rb +++ b/lib/batali/git.rb @@ -49,6 +49,24 @@ def self.included(klass) klass.class_eval do attribute :url, String, :required => true, :equivalent => true attribute :ref, String, :required => true, :equivalent => true + + @@locks = {} + @@lock_init = Mutex.new + + def self.path_lock(path) + @@lock_init.synchronize do + if !@@locks[path] + @@locks[path] = Mutex.new + end + end + if block_given? + @@locks[path].synchronize do + yield + end + else + @@locks[path] + end + end end end end diff --git a/lib/batali/source.rb b/lib/batali/source.rb index a15bd85..f43b73f 100644 --- a/lib/batali/source.rb +++ b/lib/batali/source.rb @@ -14,7 +14,6 @@ class Source < Utility attribute :type, String, :required => true, :default => lambda { self.name } # rubocop:disable Style/RedundantSelf def initialize(args = {}) - @lock = Mutex.new @cache_path = Utility.clean_path(args.delete(:cache_path)) super end @@ -24,9 +23,7 @@ def initialize(args = {}) # @yield Block to be executed # @return [Object] def synchronize - @lock.synchronize do - yield - end + yield end # @return [String] diff --git a/lib/batali/source/git.rb b/lib/batali/source/git.rb index 872d662..a6f55ed 100644 --- a/lib/batali/source/git.rb +++ b/lib/batali/source/git.rb @@ -19,6 +19,12 @@ def initialize(*_, &block) self.path = Utility.clean_path(path) end + def synchronize + self.class.path_lock(path) do + yield + end + end + # @return [String] directory containing contents def asset clone_repository