[PATCH 2 of 2] Reorder rename operations to minimise risk of leaving repository in unknown state

Steve Borho steve at borho.org
Sun Oct 4 12:26:58 CDT 2009


On Sun, Oct 4, 2009 at 11:38 AM, Adrian Buehlmann <adrian at cadifra.com>wrote:

> On 04.10.2009 18:12, Steve Borho wrote:
> > On Sun, Oct 4, 2009 at 5:17 AM, Adrian Buehlmann <adrian at cadifra.com>
> wrote:
> >
> >> On 03.10.2009 22:42, Steve Borho wrote:
> >>> At some point we have to depend on Mercurial's journaling to rollback
> >>> incomplete transactions when exceptions occur.  Fortunately most
> >> operations
> >>> are append-only and not rename / replace.
> >> The critical thing is the word "most" in our context here.
> >>
> >> I fear the reordering done by the patch might have the potential to
> >> make things worser in revlog.checkinlinesize.
> >>
> >> If we reorder the renames (i.e. put the unlink at the end), then the new
> >> indexfile will have been put in place by fp.rename(), but line 1044 in
> >> revlog.py ("tr.replace(...)") will not have been reached if the rename
> >> aborts
> >> (due to the os.unlink failing, inflicted by the scanner holding the
> >> temporary open by denying other processes to delete it).
> >>
> >> Excerpt from revlog.py (lines 1033 to 1045):
> >>
> >>        fp = self.opener(self.indexfile, 'w', atomictemp=True)
> >>        self.version &= ~(REVLOGNGINLINEDATA)
> >>        self._inline = False
> >>        for i in self:
> >>            e = self._io.packentry(self.index[i], self.node,
> self.version,
> >> i)
> >>            fp.write(e)
> >>
> >>        # if we don't call rename, the temp file will never replace the
> >>        # real index
> >>        fp.rename()
> >>
> >>        tr.replace(self.indexfile, trindex * self._io.size)
> >>        self._chunkclear()
> >>
> >> So the new index file will be put in place but it will not be recorded
> >> in the transaction.
> >>
> >> Wouldn't this leave the repo in an inconsistent state if the transaction
> >> is then rolled back (because of the abort)?
> >>
> >
> > In general, aborting with new contents is always going to be better than
> > aborting with no contents, which is the result without the patch.
> >
> > If the choice is between aborting and not aborting on the unlink, I think
> I
> > still prefer abort.
>
> Ok. I'll give up arguing soon.
>
> One last thing. The transaction will rollback the newly written data file
> and the 'new' (not-rolled-back) indexfile will be a non-inlined indexfile,
> referencing into a then non-existent data-file.
>
> So, I think the damage left in that case will be bigger than what we
> currently
> have in that case: your data is gone forever.
>
> With the current code, you would have a chance to look into the repo and
> just
> rename the temporary mentioned in the traceback back to its previous name.
>

I think I can live with catching errors from the unlink call, but there
aren't a lot of other good options for reporting errors to the user from
that level of the code.  Especially on Windows where TortoiseHg is directing
stderr to a bit bucket to work around misbehaviors between py2exe and
GTK+[1].

--
Steve Borho

[1] GTK+ emits meaningless warnings to stderr, py2exe pops up error window
when app exits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://selenic.com/pipermail/mercurial-devel/attachments/20091004/37077fbc/attachment.htm 


More information about the Mercurial-devel mailing list