( )⚙ D6664 transaction: leave unfinished without crashing when not properly released

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6664

transaction: leave unfinished without crashing when not properly released
ClosedPublic

Authored by martinvonz on Jul 21 2019, 11:01 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG36c692eb28d3: transaction: leave unfinished without crashing when not properly released
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.

Diff Detail

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

Event Timeline

martinvonz created this revision.Jul 21 2019, 11:01 AM
martinvonz added a commit: rHG36c692eb28d3: transaction: leave unfinished without crashing when not properly released.Jul 31 2019, 7:46 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG36c692eb28d3: transaction: leave unfinished without crashing when not properly released (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.
indygreg added a subscriber: indygreg.Aug 3 2019, 4:41 PM

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.

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (6 lines)

Diff 16100

mercurial/localrepo.py

args.update(phases.preparehookargs(node, old, new)) args.update(phases.preparehookargs(node, old, new))
repo.hook('pretxnclose-phase', throw=True, repo.hook('pretxnclose-phase', throw=True,
**pycompat.strkwargs(args)) **pycompat.strkwargs(args))
repo.hook('pretxnclose', throw=True, repo.hook('pretxnclose', throw=True,
**pycompat.strkwargs(tr.hookargs)) **pycompat.strkwargs(tr.hookargs))
def releasefn(tr, success): def releasefn(tr, success):
repo = reporef() 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: if success:
# this should be explicitly invoked here, because # this should be explicitly invoked here, because
# in-memory changes aren't written out at closing # in-memory changes aren't written out at closing
# transaction, if tr.addfilegenerator (via # transaction, if tr.addfilegenerator (via
# dirstate.write or so) isn't invoked while # dirstate.write or so) isn't invoked while
# transaction running # transaction running
repo.dirstate.write(None) repo.dirstate.write(None)
else: else: