From d1eccbf98f979aeb04978cda9374f25a52b7083b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 21 Mar 2013 21:55:42 -0700 Subject: [PATCH] SafeChdir all the things for thread safety --- lib/vagrant/action/general/package.rb | 3 +- lib/vagrant/box.rb | 3 +- lib/vagrant/util/safe_chdir.rb | 34 ++++++++++++++++++ lib/vagrant/util/subprocess.rb | 9 ++--- test/unit/vagrant/util/safe_chdir_test.rb | 43 +++++++++++++++++++++++ 5 files changed, 84 insertions(+), 8 deletions(-) create mode 100644 lib/vagrant/util/safe_chdir.rb create mode 100644 test/unit/vagrant/util/safe_chdir_test.rb diff --git a/lib/vagrant/action/general/package.rb b/lib/vagrant/action/general/package.rb index 450c34844..08215ef2b 100644 --- a/lib/vagrant/action/general/package.rb +++ b/lib/vagrant/action/general/package.rb @@ -1,5 +1,6 @@ require 'fileutils' +require 'vagrant/util/safe_chdir' require 'vagrant/util/subprocess' module Vagrant @@ -83,7 +84,7 @@ module Vagrant output_path = tar_path.to_s # Switch into that directory and package everything up - Dir.chdir(@env["package.directory"]) do + Util::SafeChdir.safe_chdir(@env["package.directory"]) do # Find all the files in our current directory and tar it up! files = Dir.glob(File.join(".", "**", "*")) diff --git a/lib/vagrant/box.rb b/lib/vagrant/box.rb index 56daad812..407d2f59b 100644 --- a/lib/vagrant/box.rb +++ b/lib/vagrant/box.rb @@ -4,6 +4,7 @@ require "json" require "log4r" require "vagrant/util/platform" +require "vagrant/util/safe_chdir" require "vagrant/util/subprocess" module Vagrant @@ -71,7 +72,7 @@ module Vagrant def repackage(path) @logger.debug("Repackaging box '#{@name}' to: #{path}") - Dir.chdir(@directory) do + Util::SafeChdir.safe_chdir(@directory) do # Find all the files in our current directory and tar it up! files = Dir.glob(File.join(".", "**", "*")) diff --git a/lib/vagrant/util/safe_chdir.rb b/lib/vagrant/util/safe_chdir.rb new file mode 100644 index 000000000..386415d7c --- /dev/null +++ b/lib/vagrant/util/safe_chdir.rb @@ -0,0 +1,34 @@ +require 'thread' + +module Vagrant + module Util + class SafeChdir + @@chdir_lock = Mutex.new + @@lock_holder = nil + + # Safely changes directory of this process by putting a lock around + # it so that it is thread safe. This will yield a block and when the + # block exits it changes back to the original directory. + # + # @param [String] dir Dir to change to temporarily + def self.safe_chdir(dir) + lock = @@chdir_lock + + begin + @@chdir_lock.synchronize {} + rescue ThreadError + # If we already hold the lock, just create a new lock so we + # definitely don't block and don't get an error. + lock = Mutex.new + end + + lock.synchronize do + Dir.chdir(dir) do + return yield + end + end + end + end + end +end + diff --git a/lib/vagrant/util/subprocess.rb b/lib/vagrant/util/subprocess.rb index 20d103738..4379304b8 100644 --- a/lib/vagrant/util/subprocess.rb +++ b/lib/vagrant/util/subprocess.rb @@ -4,6 +4,7 @@ require 'childprocess' require 'log4r' require 'vagrant/util/platform' +require 'vagrant/util/safe_chdir' module Vagrant module Util @@ -14,8 +15,6 @@ module Vagrant # from the subprocess in real time, by simply passing a block to # the execute method. class Subprocess - @@chdir_lock = Mutex.new - # The chunk size for reading from subprocess IO. READ_CHUNK_SIZE = 4096 @@ -76,10 +75,8 @@ module Vagrant # Start the process begin - @@chdir_lock.synchronize do - Dir.chdir(workdir) do - process.start - end + SafeChdir.safe_chdir(workdir) do + process.start end rescue ChildProcess::LaunchError => ex # Raise our own version of the error so that users of the class diff --git a/test/unit/vagrant/util/safe_chdir_test.rb b/test/unit/vagrant/util/safe_chdir_test.rb new file mode 100644 index 000000000..ae022a420 --- /dev/null +++ b/test/unit/vagrant/util/safe_chdir_test.rb @@ -0,0 +1,43 @@ +require 'tmpdir' + +require File.expand_path("../../../base", __FILE__) + +require 'vagrant/util/safe_chdir' + +describe Vagrant::Util::SafeChdir do + it "should change directories" do + expected = nil + result = nil + temp_dir = Dir.mktmpdir + + Dir.chdir(temp_dir) do + expected = Dir.pwd + end + + described_class.safe_chdir(temp_dir) do + result = Dir.pwd + end + + result.should == expected + end + + it "should allow recursive chdir" do + expected = nil + result = nil + temp_path = Dir.mktmpdir + + Dir.chdir(temp_path) do + expected = Dir.pwd + end + + expect do + described_class.safe_chdir(Dir.mktmpdir) do + described_class.safe_chdir(temp_path) do + result = Dir.pwd + end + end + end.to_not raise_error + + result.should == expected + end +end