diff options
| -rw-r--r-- | lib/gitlab/git/storage/forked_storage_check.rb | 34 | ||||
| -rw-r--r-- | spec/lib/gitlab/git/storage/forked_storage_check_spec.rb | 15 |
2 files changed, 27 insertions, 22 deletions
diff --git a/lib/gitlab/git/storage/forked_storage_check.rb b/lib/gitlab/git/storage/forked_storage_check.rb index 0e9e7ba596b..91d8241f17b 100644 --- a/lib/gitlab/git/storage/forked_storage_check.rb +++ b/lib/gitlab/git/storage/forked_storage_check.rb @@ -11,7 +11,7 @@ module Gitlab end def timeout_check(path, timeout_seconds) - filesystem_check_pid = check_filesystem_in_fork(path) + filesystem_check_pid = check_filesystem_in_process(path) deadline = timeout_seconds.seconds.from_now.utc wait_time = 0.01 @@ -31,27 +31,23 @@ module Gitlab status end - # This call forks out into a process, that process will then be replaced - # With an `exec` call, since we fork out into a shell, we can create a - # child process without needing an ActiveRecord-connection. + # This will spawn a new 2 processes to do the check: + # The outer child (waiter) will spawn another child process (stater). # - # Inside the shell, we use `& wait` to fork another child. We do this - # to prevent leaving a zombie process when the parent gets killed by the - # timeout. - # - # https://stackoverflow.com/questions/27892975/what-causes-activerecord-breaking-postgres-connection-after-forking - # https://stackoverflow.com/questions/22012943/activerecordstatementinvalid-runtimeerror-the-connection-cannot-be-reused-in - def check_filesystem_in_fork(path) - fork do - STDOUT.reopen('/dev/null') - STDERR.reopen('/dev/null') - - exec("(#{test_script(path)}) & wait %1") - end + # The stater is the process is performing the actual filesystem check + # the check might hang if the filesystem is acting up. + # In this case we will send a `KILL` to the waiter, which will still + # be responsive while the stater is hanging. + def check_filesystem_in_process(path) + spawn('ruby', '-e', ruby_check, path, [:out, :err] => '/dev/null') end - def test_script(path) - "testpath=$(realpath #{Shellwords.escape(path)}) && stat \"$testpath\"" + def ruby_check + <<~RUBY_FILESYSTEM_CHECK + inner_pid = fork { File.stat(ARGV.first) } + Process.waitpid(inner_pid) + exit $?.exitstatus + RUBY_FILESYSTEM_CHECK end end end diff --git a/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb b/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb index 20cee123ede..12366151f44 100644 --- a/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb +++ b/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb @@ -17,11 +17,20 @@ describe Gitlab::Git::Storage::ForkedStorageCheck, skip_database_cleaner: true d end it 'returns false when the check takes to long' do - allow(described_class).to receive(:check_filesystem_in_fork) do - fork { sleep 10 } + # We're forking a process here that takes too long + # It will be killed it's parent process will be killed by it's parent + # and waited for inside `Gitlab::Git::Storage::ForkedStorageCheck.timeout_check` + allow(described_class).to receive(:check_filesystem_in_process) do + Process.spawn("sleep 10") end + result = true - expect(described_class.storage_available?(existing_path, 0.5)).to be_falsey + runtime = Benchmark.realtime do + result = described_class.storage_available?(existing_path, 0.5) + end + + expect(result).to be_falsey + expect(runtime).to be < 1.0 end describe 'when using paths with spaces' do |