Patchwork D6664: transaction: leave unfinished without crashing when not properly released

login
register
mail settings
Submitter phabricator
Date July 21, 2019, 3:01 p.m.
Message ID <differential-rev-PHID-DREV-ib5zcg7nitctfbzrttc5-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40994/
State Superseded
Headers show

Comments

phabricator - July 21, 2019, 3:01 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I think the transaction.__del__ is there just as a last resort in case
  we (or an extension) forgot to release the transaction. When that
  happens, the repo can (or will on Python 3?) get deleted before the
  transaction. This leads to a crash in test-devel-warnings.t on Python
  3 because we tried to access repo.dirstate, where repo was retried
  from a weak reference. There's not much we can do here, but let's at
  least avoid the crash. The user will have run `hg recover` afterwards
  regardless.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6664

AFFECTED FILES
  mercurial/localrepo.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 3, 2019, 8:41 p.m.
indygreg added a comment.


  The presence of `__del__` is a bit dangerous and I've debugged a handful of issues related to having `__del__` on `transaction`. We still leak repo objects in places due to circular references. It's super annoying.
  
  Ideally we would use context managers everywhere and we could remove `__del__` completely. I would strongly suggest that we make `__del__` deprecated and emit a warning if it is doing something meaningful, as I view the lack of the caller cleaning up state as a bug. Then we can remove `__del__` in some future release.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6664/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6664

To: martinvonz, #hg-reviewers
Cc: indygreg, mercurial-devel

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1893,6 +1893,12 @@ 
                       **pycompat.strkwargs(tr.hookargs))
         def releasefn(tr, success):
             repo = reporef()
+            if repo is None:
+                # If the repo has been GC'd (and this release function is being
+                # called from transaction.__del__), there's not much we can do,
+                # so just leave the unfinished transaction there and let the
+                # user run `hg recover`.
+                return
             if success:
                 # this should be explicitly invoked here, because
                 # in-memory changes aren't written out at closing