Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugin: Write out state file via create-new-then-rename #7550

Merged
merged 1 commit into from
Jul 19, 2016

Conversation

cgwalters
Copy link
Contributor

This makes updates atomic, so it's useful generally. However
I'm making this change specifically to support rpm-ostree.

For more information, see:
https://bugzilla.redhat.com/show_bug.cgi?id=1352152

tmpfile = Tempfile.new(@path.basename.to_s, @path.dirname.to_s)
tmpfile.write(JSON.dump(@data))
tmpfile.close
File.rename(tmpfile.path, @path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, so there's a reason we don't do this...

  1. File.rename has historically been a problem on Windows
  2. File.rename does not work across multiple volumes, particularly if the volumes are different formats
  3. The tmpfile could be removed by GC before the mv takes place (aka there's a slight race between lines 97 and 98 here)

I think it's probably a better idea to do something like:

Tempfile.open("vagrant-plugins") do |f|
  f.binmode
  f.write(JSON.dump(@data))
  f.close
  FileUtils.mv(f.path, @path)
end

What do you think?

Copy link
Contributor Author

@cgwalters cgwalters Jul 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re 2: The tempfile is created in the same directory as the target
Re 3: I'll be honest and say this is my first Ruby patch, but I have a strong suspicion it can't be removed by GC because the local variable holds a strong reference across the function scope

But yes, this is just an implementation of a general pattern for "replace file atomically" which is harder on Windows indeed. I'm an upstream developer for GLib where for g_file_set_contents() we do https://git.gnome.org/browse/glib/tree/glib/gfileutils.c#n1217

It's the sort of thing I'd expect Ruby to have in the standard library, but if it exists I couldn't find it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think FileUtils.mv handles this better than File.rename

@cgwalters
Copy link
Contributor Author

Okay, updated ⬆️ to use FileUtils.mv

@sethvargo sethvargo self-assigned this Jul 7, 2016
@sethvargo sethvargo added this to the 1.8.5 milestone Jul 7, 2016
@cgwalters
Copy link
Contributor Author

cgwalters commented Jul 7, 2016

FileUtils.mv will use rename() on Unix which does unlink the
tempfile, and on Windows it will to after deleting the original.

The tempfile does still call unlink in its cleanup on the old name,
but that's relatively harmless...not sure offhand if there's a way to
avoid that.

(There has to be some Ruby codebase that does things like this right and
has a "write file atomically" utility function but I don't know Ruby
well enough to know which codebases are good to look at)

On Thu, Jul 7, 2016, at 03:19 PM, Seth Vargo wrote:

This is still going to leave the tempfile lying around.
The Ruby docs are wrong... you have to call #unlink on the file to
remove it, but the block form that I posted in my earlier comment
automatically does that.
 

@cgwalters
Copy link
Contributor Author

Sorry, when reworking this I introduced a regression - we need to chmod back to 0644. Will fix.

This makes updates atomic, so it's useful generally.  However
I'm making this change specifically to support rpm-ostree.

For more information, see:
https://bugzilla.redhat.com/show_bug.cgi?id=1352152
@sethvargo sethvargo merged commit ea17ad4 into hashicorp:master Jul 19, 2016
@ghost ghost locked and limited conversation to collaborators Apr 4, 2020
@ghost ghost unassigned sethvargo Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants