Atomic object writes #157

Merged
merged 2 commits into from Feb 23, 2013

Conversation

Projects
None yet
2 participants
Contributor

peff commented Feb 22, 2013

We occasionally see corrupted empty loose object files on GitHub after a machine failover. The problem is that Grit is not as careful with its object writes as Git itself. This PR should fix that.

/cc @rtomayko

peff added some commits Feb 22, 2013

make object writes atomic
When Grit writes a loose object via the LooseStorage class,
it just opens the object file and starts writing. This works
most of the time, but can be a problem in some corner cases,
including:

  1. If another process tries to write the same object
     simultaneously, the writes may be interleaved and the
     object can be corrupted.

  2. If another process tries to read the object
     simultaneously, it may see the object in a half-written
     state.

  3. If the process or machine crashes during the write, we
     may leave a half-written corrupt object.

This can be solved by writing the object to a temporary file
and linking it into place. This is the same strategy used by
git itself.
fsync loose objects before moving into place
When we write a loose object to disk, we simply close the
file object before moving it into place. If the machine
crashes shortly after our write, the contents may not have
been committed to disk (depending your filesystem, usually
the metadata is, and you end up with a corrupt, zero-length
loose object file).

This is especially bad because we report that the object is
successfully written, which means we may have updated refs
to point to it. A corrupt object at that point means not
only does the operation fail, but the repository is left in
a corrupted and unusable state.

We can fix this by calling fsync on the object file before
linking it into place. Between this and the previous commit,
our object writing should now behave exactly like git's
internal routines.
Collaborator

rtomayko commented Feb 23, 2013

This looks great.

rtomayko added a commit that referenced this pull request Feb 23, 2013

@rtomayko rtomayko merged commit 58621d9 into mojombo:master Feb 23, 2013

Collaborator

rtomayko commented Feb 23, 2013

Oops this is failing under Ruby 1.8.7 with:

ArgumentError: wrong number of arguments (3 for 2)
    /usr/share/rbenv/versions/ree-1.8.7-2012.02+github1/lib/ruby/1.8/tempfile.rb:184
    /usr/share/rbenv/versions/ree-1.8.7-2012.02+github1/lib/ruby/1.8/tempfile.rb:184
    /usr/share/rbenv/versions/ree-1.8.7-2012.02+github1/lib/ruby/1.8/tempfile.rb:184
    vendor/internal-gems/grit/lib/grit/git-ruby/internal/loose.rb:67
Collaborator

rtomayko commented Feb 23, 2013

So 1.8.7 doesn't take the extra options argument. The mode default is:

Creates a temporary file of mode 0600 in the temporary directory, opens it with mode “w+”, and returns a Tempfile object which represents the created temporary file. A Tempfile object can be treated just like a normal File object.

The w+ mode is:

Read-write, truncates existing file to zero length or creates a new file for reading and writing.

Seems like maybe we could drop the argument entirely and roll with that. The other huge issue here though is encodings. If we drop the wb we'll likely run into encoding compatibility errors when writing binary data due to the attempted conversion to utf-8.

Collaborator

rtomayko commented Feb 23, 2013

Fixed this up in b49a6ff. Should be fine.

Contributor

peff commented Feb 23, 2013

Yeah, exactly, I was worried about losing the "b" flag. Your fix looks good. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment