diff --git a/lib/snap_diff/vcs.rb b/lib/snap_diff/vcs.rb index 87205df3..a5b34081 100644 --- a/lib/snap_diff/vcs.rb +++ b/lib/snap_diff/vcs.rb @@ -6,6 +6,7 @@ module SnapDiff module Vcs @git_roots = {} + @git_roots_lock = Mutex.new def self.checkout_vcs(root, screenshot_path, checkout_path) root_path = root.to_s @@ -39,11 +40,22 @@ def self.checkout_vcs(root, screenshot_path, checkout_path) # while the suite runs, so remember it per directory. `false` (not a repo) # is remembered too: that is the every-assertion answer for anyone whose # screenshots live outside a git checkout. + # + # Synchronized because the lookup itself is what must not be duplicated: + # MRI releases the GVL for the whole of `Open3.capture3`, so eight threads + # asking about one root all miss `key?` before any of them writes -- eight + # spawns, the exact cost this cache exists to remove. Threads are the + # default parallel mode on JRuby, which has no GVL to make the Hash write + # safe either. Holding the lock across the spawn is deliberate: callers + # almost always share one root, so the other threads wait once and then + # read the cache, which is the outcome we want. def self.git_root_for(root_path) - return @git_roots[root_path] if @git_roots.key?(root_path) + @git_roots_lock.synchronize do + next @git_roots[root_path] if @git_roots.key?(root_path) - git_root, _, status = Open3.capture3("git", "-C", root_path, "rev-parse", "--show-toplevel") - @git_roots[root_path] = status.success? && git_root.chomp + git_root, _, status = Open3.capture3("git", "-C", root_path, "rev-parse", "--show-toplevel") + @git_roots[root_path] = status.success? && git_root.chomp + end end end end diff --git a/test/unit/vcs_test.rb b/test/unit/vcs_test.rb index 72588620..83ab98b3 100644 --- a/test/unit/vcs_test.rb +++ b/test/unit/vcs_test.rb @@ -54,4 +54,36 @@ def status.success? = true assert_equal 1, calls end + + # Concurrent callers must share the cached answer, not each spawn their own + # git. MRI releases the GVL for the duration of `Open3.capture3`, so without + # synchronization every thread misses `key?` before any thread writes -- + # measured 8 spawns for 8 threads asking about ONE root, i.e. the cache + # bought nothing in exactly the mode (`parallelize(with: :threads)`) that is + # JRuby's default. On JRuby there is no GVL at all, so the unsynchronized + # Hash write is unsafe as well as wasteful. + test "#checkout_vcs shares one git lookup across concurrent callers" do + root = @tmp_dir / "concurrent_root_#{Time.now.nsec}" + FileUtils.mkdir_p(root) + screenshot_path = file_fixture("images/a.png") + base_screenshot_path = Pathname.new(@base_screenshot.path) + + calls = 0 + counter_lock = Mutex.new + status = Object.new + def status.success? = true + counting_rev_parse = ->(*) { + counter_lock.synchronize { calls += 1 } + sleep 0.01 # stand in for the real subprocess, which releases the GVL + ["#{PROJECT_ROOT}\n", "", status] + } + + Open3.stub(:capture3, counting_rev_parse) do + 8.times.map { + Thread.new { SnapDiff::Vcs.checkout_vcs(root, screenshot_path, base_screenshot_path) } + }.each(&:join) + end + + assert_equal 1, calls, "concurrent callers must share one git lookup, not spawn one each" + end end