Hi! I ran into a strange pre-push behavior while debugging a custom hook, and I think it comes down to a small race in HookContext::Base#input_string.
Right now it memoizes stdin like this:
def input_string
@input_string ||= @input.read
end
Since hooks are parallelized by default, two hooks that share the same context can call this at the same time. For hook types that get real data on stdin, like pre-push, that means both threads may try to read the same input stream.
The failure mode is subtle:
- one thread reads the actual Git pre-push refs
- another thread reads EOF and gets
""
- if the empty read is assigned last,
@input_string stays cached as ""
For pre-push, Git sends refs through stdin:
<local ref> <local sha1> <remote ref> <remote sha1>
So if the empty string wins the race, helpers like pushed_refs can return an empty list even though Git did send refs. In my case this made a pre-push check silently skip the work it was supposed to do.
I think the fix can be pretty small: protect the first read with a mutex, so the stream is only consumed once and every hook gets the same cached input.
Something like:
def input_string
return @input_string if defined?(@input_string)
@input_mutex.synchronize do
@input_string = @input.read unless defined?(@input_string)
end
@input_string
end
I also put together a test that avoids relying on flaky timing: it uses a fake input object that blocks the first read, starts a second thread, and fails if the underlying input gets read more than once.
I have a branch here if that helps:
https://github.com/Felix-LeeSM/overcommit/tree/fix/thread-safe-hook-input
Focused spec, RuboCop, and bundle exec overcommit --run pass locally. The full spec suite has a few unrelated failures around git alias amendment detection on my machine.
Would you be open to a PR for this?
Hi! I ran into a strange pre-push behavior while debugging a custom hook, and I think it comes down to a small race in
HookContext::Base#input_string.Right now it memoizes stdin like this:
Since hooks are parallelized by default, two hooks that share the same context can call this at the same time. For hook types that get real data on stdin, like
pre-push, that means both threads may try to read the same input stream.The failure mode is subtle:
""@input_stringstays cached as""For
pre-push, Git sends refs through stdin:So if the empty string wins the race, helpers like
pushed_refscan return an empty list even though Git did send refs. In my case this made a pre-push check silently skip the work it was supposed to do.I think the fix can be pretty small: protect the first read with a mutex, so the stream is only consumed once and every hook gets the same cached input.
Something like:
I also put together a test that avoids relying on flaky timing: it uses a fake input object that blocks the first
read, starts a second thread, and fails if the underlying input gets read more than once.I have a branch here if that helps:
https://github.com/Felix-LeeSM/overcommit/tree/fix/thread-safe-hook-input
Focused spec, RuboCop, and
bundle exec overcommit --runpass locally. The full spec suite has a few unrelated failures around git alias amendment detection on my machine.Would you be open to a PR for this?