Handle premature deletion of temporary tar files

[tylerhunt]

This fixes the error reported in #133, in which creating an image could result in an error like:

No such file or directory @ unlink_internal - /var/folders/b9/3dxpcnmn7r7ff8dnvysqlf_80000gn/T/out20140603-32965-kdhtif

In this case, the image is built fine, but by the time Container#build_from_dir reaches the ensure block where the temporary Tar file is closed, the temporary file has already been removed. While I was unable to produce a failing test case, this was a reproducible issue in my own app, and this fix appears to resolve it.

I believe the issue is that Ruby's automatic unlinking of the temporary file is kicking in at some point while the build is in progress (maybe during GC, though GC.start alone didn't seem to trigger it my test), since there is no longer a reference to the Tempfile instance after the Util#create_dir_tar method has returned.

[tombh]

Specs are failing for the same reason I found in #140: Rubygems is defaulting to Rspec 3 now. So need to pin to Rspec <3.0.0.

[tylerhunt]

The other option would be pinning to RSpec ~> 3.0 and adding the rspec-its gem as a dependency. I had started doing this work, as well, but figured I'd hold off until I got feedback on how to best proceed.

[bfulton]

@tylerhunt that sounds like a good approach. Would you like to open as a separate pull?

[bfulton]

Nmind, you already did! 😄

#145

[markkendall]

For additional context, I've verified that the original error happens fairly consistently in Ruby 2.1.1 and 2.1.2, but not in 2.0.0. I'm assuming something changed in the GC behavior starting in Ruby 2.1.

[tylerhunt]

Thanks, @markkendall. Added a spec that fails under Ruby 2.1.

[tlunter]

Interestingly, here, we're opening the Tempfile, which opens with w+ per the documentation. This should be perfectly fine for reading and writing, right? Any reason we need to have a Tempfile and File?

Also, it appears when you run Tempfile#close, it unlinks and removes the tempfile, likely where this issue is stemming from. I wonder if we need that FileUtils.rm given the #close method.

[tylerhunt]

I tried getting rid of the secondary file altogether, but the Tempfile is used like this:

Archive::Tar::Minitar.pack('.', tempfile)

And that #pack method is calling #close on the file.

I also tried doing tempfile.reopen(tempfile.path, 'r'), but this returns a File instance and not a Tempfile, so it wouldn't have the same auto-unlinking behavior.

[tlunter]

Interesting. So, it seems from the docs that Tempfile#close doesn't delete the file, since unlink_now defaults to false. So, doing the File.open is essentially the same as the tempfile.open (I think/hope).

The garbage collector must be removing it since it doesn't know the file is still open. It seems the force might be the only way for this to work. Container#build_from_dir doesn't seem to fail despite no file actually being in place?

[tylerhunt]

@tlunter Do you mean Image#build_from_dir? Not sure what you're asking here. It's always worked when the file did exist. The failure case was when the Ruby GC kicked off and removed the temporary file before the call to FileUtils.rm(tar.path). Adding the force: true option to the call means it won't error out if the file doesn't actually exist.

[bfulton]

@tylerhunt do you know what the build failures here are due to yet?

[tlunter]

@tylerhunt, sorry. I did mean Image#build_from_dir. My question is, do you see us ever running into an issue where the file is removed before even being closed and removed by FileUtils? I don't want to run into an issue down the line where the file we're trying to build is deleted by the GC before we pass it to docker.

[tylerhunt]

@bfulton Pretty sure they're all related to RSpec 3. I'll rebase this branch once #146 has been merged.

Ahh, I see what you're saying now, @tlunter. Yes, that may be a possibility. Here are a couple possible ways this could be resolved:

1. Create a new Tempfile with the contents of the original Tempfile inside the Util#create_dir_tar method, and return that instead of a normal File object. Since we'd be returning a Tempfile instance, it wouldn't be garbage collected until all its references were gone. The downside here is the additional overhead of copying the file.
2. Stop using Tempfile altogether. We could still use Dir::Tmpname to generate a temporary file name. The downsides here are the loss of the automatic unlinking and the additional complexity of having to generate the temporary filename ourselves.

[bfulton]

Thanks @tylerhunt and @tlunter. This is closed through #147.