This is an archive of the discontinued Mercurial Phabricator instance.

Differential D586

remotefilelog: make pack cleanup more robust
ClosedPublic

Authored by durham on Aug 31 2017, 6:07 PM.
Tags
None
Subscribers
ms2316

Details

Reviewers
ms2316
Group Reviewers
Restricted Project
Commits
rFBHGXf52d7e0b51d4: remotefilelog: make pack cleanup more robust
Summary

We're seeing exceptions from the unlink cleanup logic hide the actual exception
from the code. In order to debug the actual exception, we need to eat the
cleanup exceptions and in any place where we eat an exception inside of a catch
block, we need to manually rethrow the original exception, otherwise we end up
throwing the second caught exception (from the cleanup logic).

Test Plan

Ran the tests. This is a particular odd edge case, so I need to run
these bits in the production environment where it was occuring before I can
understand why it was happening.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

durham created this revision.Aug 31 2017, 6:07 PM
ms2316 added a subscriber: ms2316.Sep 5 2017, 11:48 AM
ms2316 added a comment.Sep 5 2017, 1:05 PM
This comment was removed by ms2316.
remotefilelog/basepack.py
322

If self.opener.rename(...) throws an exception then the 'sha + SUFFIX' file is not created? I am not sure if it is created or not, therefore I would add unlink 'sha + SUFFIX' files here just in case.

323–324

What if self._cleantemppacks() raises an exception? Won't we re-raise the exception from _cleantemppacks() on line 324 instead of the exception in the code?

ms2316 added inline comments.Sep 5 2017, 1:21 PM
remotefilelog/basepack.py
323–324

... instead of the exception on line 321? ***

ms2316 accepted this revision.EditedSep 5 2017, 2:15 PM

What are the unit test failures? And the comment on 322.

remotefilelog/basepack.py
323–324

No it wont. Never mind

This revision is now accepted and ready to land.Sep 5 2017, 2:15 PM
durham added a comment.Sep 5 2017, 2:27 PM

The unit test failures are just my development environment not liking to run unit tests from inside arc. The tests pass when I run them myself.

remotefilelog/basepack.py
322

Yea, if rename throws then the file was not created. I don't want to blindly delete the file, since other hg processes may have created the exact same pack file (which might be why this process's rename failed).

ms2316 added a comment.Sep 5 2017, 2:29 PM

Looks good to me then

Closed by commit rFBHGXf52d7e0b51d4: remotefilelog: make pack cleanup more robust (authored by durham). · Explain WhySep 6 2017, 9:54 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mremotefilelog/basepack.py (34 lines)

Diff 1649

remotefilelog/basepack.py

def __exit__(self, exc_type, exc_value, traceback): def __exit__(self, exc_type, exc_value, traceback):
if exc_type is None: if exc_type is None:
self.close() self.close()
else: else:
self.abort() self.abort()
def abort(self): def abort(self):
# Unclean exit # Unclean exit
try: self._cleantemppacks()
self.opener.unlink(self.packpath)
self.opener.unlink(self.idxpath)
except Exception:
pass
def writeraw(self, data): def writeraw(self, data):
self.packfp.write(data) self.packfp.write(data)
self.sha.update(data) self.sha.update(data)
def close(self, ledger=None): def close(self, ledger=None):
if self._closed: if self._closed:
return return
try: try:
sha = self.sha.hexdigest() sha = self.sha.hexdigest()
self.packfp.close() self.packfp.close()
self.writeindex() self.writeindex()
if len(self.entries) == 0: if len(self.entries) == 0:
# Empty pack # Empty pack
self.opener.unlink(self.packpath) self._cleantemppacks()
self.opener.unlink(self.idxpath)
self._closed = True self._closed = True
return None return None
self.opener.rename(self.packpath, sha + self.PACKSUFFIX) self.opener.rename(self.packpath, sha + self.PACKSUFFIX)
try:
self.opener.rename(self.idxpath, sha + self.INDEXSUFFIX) self.opener.rename(self.idxpath, sha + self.INDEXSUFFIX)
except Exception: except Exception as ex:
for path in [self.packpath, self.idxpath,
sha + self.PACKSUFFIX, sha + self.INDEXSUFFIX]:
try: try:
self.opener.unlink(path) self.opener.unlink(sha + self.PACKSUFFIX)
except Exception: except Exception:
pass pass
# Throw exception 'ex' explicitly since a normal 'raise' would
# potentially throw an exception from the unlink cleanup.
raise ex
except Exception:
# Clean up temp packs in all exception cases
ms2316Unsubmitted
Not Done

If self.opener.rename(...) throws an exception then the 'sha + SUFFIX' file is not created? I am not sure if it is created or not, therefore I would add unlink 'sha + SUFFIX' files here just in case.

ms2316: If self.opener.rename(...) throws an exception then the 'sha + SUFFIX' file is not created? I…
durhamAuthorUnsubmitted
Not Done

Yea, if rename throws then the file was not created. I don't want to blindly delete the file, since other hg processes may have created the exact same pack file (which might be why this process's rename failed).

durham: Yea, if rename throws then the file was not created. I don't want to blindly delete the file…
self._cleantemppacks()
raise raise
ms2316Unsubmitted
Not Done

What if self._cleantemppacks() raises an exception? Won't we re-raise the exception from _cleantemppacks() on line 324 instead of the exception in the code?

ms2316: What if self._cleantemppacks() raises an exception? Won't we re-raise the exception from…
ms2316Unsubmitted
Not Done

... instead of the exception on line 321? ***

ms2316: ... instead of the exception on line 321? ***
ms2316Unsubmitted
Not Done

No it wont. Never mind

ms2316: No it wont. Never mind
self._closed = True self._closed = True
result = self.opener.join(sha) result = self.opener.join(sha)
if ledger: if ledger:
ledger.addcreated(result) ledger.addcreated(result)
return result return result
def _cleantemppacks(self):
try:
self.opener.unlink(self.packpath)
except Exception:
pass
try:
self.opener.unlink(self.idxpath)
except Exception:
pass
def writeindex(self): def writeindex(self):
rawindex = '' rawindex = ''
largefanout = len(self.entries) > SMALLFANOUTCUTOFF largefanout = len(self.entries) > SMALLFANOUTCUTOFF
if largefanout: if largefanout:
params = indexparams(LARGEFANOUTPREFIX, self.VERSION) params = indexparams(LARGEFANOUTPREFIX, self.VERSION)
else: else:
params = indexparams(SMALLFANOUTPREFIX, self.VERSION) params = indexparams(SMALLFANOUTPREFIX, self.VERSION)