Skip to content

Commit e88e06b

Browse files
authored
Make hook context input reads thread-safe (#882)
## Summary This fixes a small race in `HookContext::Base#input_string`. Hook contexts currently cache stdin lazily with `@input_string ||= @input.read`. Since hooks are parallelized by default, two hooks sharing the same context can race during that first read. For hook types that receive real data on stdin, such as `pre-push`, one hook can consume the stream while another reads EOF and caches an empty string. If that happens, helpers like `pushed_refs` can silently see no refs even though Git did provide them. ## Changes - Add a mutex around the first `input_string` read so the stream is consumed once. - Keep the cached string shared by all callers after the initial read. - Add a spec that forces a concurrent read attempt without relying on flaky timing. This follows up on #881. ## Verification Focused checks pass locally: ```sh bundle exec rspec spec/overcommit/hook_context/base_spec.rb bundle exec rubocop lib/overcommit/hook_context/base.rb spec/overcommit/hook_context/base_spec.rb bundle exec overcommit --run ``` I also ran the full spec suite. It still has a few unrelated failures around git alias amendment detection on my machine, but this change's focused spec passes.
1 parent 141b0ae commit e88e06b

2 files changed

Lines changed: 59 additions & 1 deletion

File tree

lib/overcommit/hook_context/base.rb

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ def initialize(config, args, input, **options)
2424
@args = args
2525
@input = input
2626
@options = options
27+
@input_mutex = Mutex.new
2728
end
2829

2930
# Executes a command as if it were a regular git hook, passing all
@@ -95,7 +96,13 @@ def all_files
9596
#
9697
# @return [String]
9798
def input_string
98-
@input_string ||= @input.read
99+
return @input_string if defined?(@input_string)
100+
101+
@input_mutex.synchronize do
102+
@input_string = @input.read unless defined?(@input_string)
103+
end
104+
105+
@input_string
99106
end
100107

101108
# Returns an array of lines passed to the hook via the standard input

spec/overcommit/hook_context/base_spec.rb

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,57 @@
2626
it { should == ['line 1', 'line 2'] }
2727
end
2828

29+
describe '#input_string' do
30+
let(:input_class) do
31+
Class.new do
32+
attr_reader :read_count
33+
34+
def initialize(value)
35+
@value = value
36+
@read_count = 0
37+
@lock = Mutex.new
38+
@read_started = Queue.new
39+
@read_finished = Queue.new
40+
end
41+
42+
def read
43+
@lock.synchronize do
44+
@read_count += 1
45+
raise 'input stream was read more than once' if @read_count > 1
46+
end
47+
48+
@read_started << true
49+
@read_finished.pop
50+
@value
51+
end
52+
53+
def wait_until_reading
54+
@read_started.pop
55+
end
56+
57+
def finish_reading
58+
@read_finished << true
59+
end
60+
end
61+
end
62+
63+
let(:input) { input_class.new("line 1\nline 2\n") }
64+
65+
it 'shares one input stream read across concurrent callers' do
66+
first_reader = Thread.new { context.input_string }
67+
input.wait_until_reading
68+
69+
second_reader = Thread.new { context.input_string }
70+
Thread.pass until second_reader.status == 'sleep'
71+
72+
input.finish_reading
73+
results = [first_reader, second_reader].map(&:value)
74+
75+
results.should == Array.new(2, "line 1\nline 2\n")
76+
input.read_count.should == 1
77+
end
78+
end
79+
2980
describe '#post_fail_message' do
3081
subject { context.post_fail_message }
3182

0 commit comments

Comments
 (0)