Fix apt_key tempfile race condition #572
Conversation
|
This improves the situation, but is still missing something, don't merge. |
|
Ok, this should be good now. Tested by forcing GC right before running the apt-key command and cannot reproduce now. |
|
Could you squash this down into one commit please? |
The Ruby Tempfile class has a finalizer that removes the file when the GC runs. It's not predictible when the GC will run, so you have to ensure that the instance of the class stays in scope for as long as you need it. Unfortunately the tempfile method is returning just the filename of the temporary file, which means it goes out of scope when that method returns. This allows the GC to reap it at any time after return. In both CI and production environments we've seen this race fail, causing apt-key add to fail a small (2-3%) amount of the time. This changes the tempfile and source_to_file methods to return the underlying Tempfile object, pushing it up into the caller's scope. Both of the callers immediately use the object to get its filename and then open the file, eliminating the race. Tested this by adding 'GC.start; sleep(1)' immediately before the command is run, to give the GC plenty of time to remove the tempfile if it was going to.
claytono
force-pushed
the
master-tempfile-race-condition
branch
from
95ff7f9 to
ea6a84f
Compare
|
Done |
|
We have a triage tonight, I'll raise it there. |
|
closing this as it is a duplicate of #571 |
|
This isn't a duplicate, it's on a different branch. |
|
both are be merged into master though. or is that incorrect ? |
|
No, one is against master, the other is against the 1.8.x branch. |
|
Generally we would apply into master first, then all the changes would be merged into the release branch, prior to the new release going out. |
|
The other PR is against 1.8, the old release of APT. It's unlikely we'll ever release that. |
|
@tphoney Can someone at PL run the integration tests for this and come back to us with the result? |
|
@daenney i will get this into our sprint |
|
@daenney @HelenCampbell @tphoney Here is the acceptance test result from PCCI. |
|
Alright, so everything appears to work. Someone care to merge? |
|
Ran a recheck just in case... https://ci.voxpupuli.org/buildlogs/puppetlabs+puppetlabs-apt+572+1452115700+PASS |
…tion Fix apt_key tempfile race condition
The Ruby Tempfile class has a finalizer that removes the file when the
GC runs. It's not predictible when the GC will run, so you have to
ensure that the instance of the class stays in scope for as long as you
need it.
Unfortunately the tempfile method is returning just the filename of the
temporary file, which means it goes out of scope when that method
returns. This allows the GC to reap it at any time after return.
In both CI and production environments we've seen this race fail,
causing apt-key add to fail a small (2-3%) amount of the time.
This changes the tempfile and source_to_file methods to return the
underlying Tempfile object, pushing it up into the caller's scope. Both
of the callers immediately use the object to get its filename and then
open the file, eliminating the race.