diff --git a/lib/overcommit/subprocess.rb b/lib/overcommit/subprocess.rb index 88ef362e..8493efab 100644 --- a/lib/overcommit/subprocess.rb +++ b/lib/overcommit/subprocess.rb @@ -50,6 +50,11 @@ def spawn(args, options = {}) out.rewind Result.new(process.exit_code, out.read, err.read) + ensure + # Delete the output files now rather than leaving it to the garbage + # collector, which on Windows fails (and prints a backtrace) if a process + # left behind by the command still has them open. + [out, err].each { |stream| stream&.close! } end # Spawns a new process in the background using the given array of diff --git a/spec/overcommit/subprocess_spec.rb b/spec/overcommit/subprocess_spec.rb index 07503d25..4a11f7b5 100644 --- a/spec/overcommit/subprocess_spec.rb +++ b/spec/overcommit/subprocess_spec.rb @@ -106,6 +106,53 @@ def received_argv(result) received_argv(subject).should == ['', 'after'] end end + + context 'once the process has finished' do + # Regression test for https://github.com/sds/overcommit/issues/899, where + # the temporary files capturing the output were left for the garbage + # collector to delete. On Windows that delete fails with `Errno::EACCES` + # when a process left behind by the command still holds the file open, + # and Ruby prints a backtrace from the finalizer on every hook run. + let(:tempfiles) { [] } + + # Records every Tempfile created, passing each one to the block (if given) + # so a test can stub it. + def record_tempfiles(&block) + ::Tempfile.stub(:new).and_wrap_original do |original, *args| + original.call(*args).tap do |tempfile| + tempfiles << tempfile + block&.call(tempfile) + end + end + end + + def expect_tempfiles_deleted + tempfiles.size.should == 2 + tempfiles.each do |tempfile| + tempfile.should be_closed + tempfile.path.should be_nil # Tempfile#path is nil once unlinked + end + end + + it 'closes and deletes the files that captured its output' do + record_tempfiles + + result = described_class.spawn([ruby, '-e', 'STDOUT.print "out"; STDERR.print "err"']) + + result.stdout.should == 'out' + result.stderr.should == 'err' + expect_tempfiles_deleted + end + + it 'closes and deletes the files when reading the output fails' do + record_tempfiles { |tempfile| tempfile.stub(:read).and_raise(IOError, 'read failed') } + + expect { described_class.spawn([ruby, '-e', 'STDOUT.print "out"']) }. + to raise_error(IOError, 'read failed') + + expect_tempfiles_deleted + end + end end # Subprocess no longer branches on platform, but this guards against