Repository navigation
Conversation
`Subprocess.spawn` captured the child's stdout and stderr in two Tempfiles and never closed them, so they were deleted by the garbage collector whenever it got round to it. On Windows that delete fails with `Errno::EACCES` if a process left behind by the command still holds the file open, and Ruby prints a backtrace from the finalizer on every hook run. Close and unlink both files in an `ensure` once the output has been read, so they are removed when `spawn` returns, including when reading fails. Fixes sds#899
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #899
Subprocess.spawncaptures the child's stdout and stderr in two Tempfiles and never closes them, so deleting them is left to the garbage collector. On Windows that delete fails withErrno::EACCESwhen a process left behind by the command (for example the innerruby.exeofbundle exec rspec) still has the file open, and Ruby prints the finalizer backtrace from the issue on every hook run.This closes and unlinks both files (
Tempfile#close!) in anensureonce the output has been read, so they are gone whenspawnreturns, including when reading the output raises. As suggested in the issue, this goes throughTempfile#unlinkrather thanFile.unlink, so a delete that Windows refuses is ignored instead of raising.spawn_detachedis unchanged: its child is still running when it returns, so its files are still left to the garbage collector.Tests
Two examples in
spec/overcommit/subprocess_spec.rbrecord the Tempfiles thatspawncreates and check that both are closed and unlinked (Tempfile#pathisnil) after it returns: one for a normal run, one where reading the output raises.The suite was run on macOS with Ruby 4.0.7:
bundle exec rspec spec/overcommit/subprocess_spec.rbwithlib/overcommit/subprocess.rbreverted to master:10 examples, 2 failures, both onexpected ... closed? to be truthy, got false. With the change:10 examples, 0 failures.bundle exec rspec:1657 examples, 4 failures. The same 4 examples fail on master (1655 examples, 4 failures): the git alias#amendment?examples inhook_context/commit_msg_spec.rb:134,140andhook_context/pre_commit_spec.rb:43,49, which look specific to this machine's Git (Apple Git 2.54).bundle exec rubocop: no offenses.Not run: anything on Windows, so the
Errno::EACCESwarning itself was not reproduced;bundle exec overcommit --run.