Page MenuHomePhabricator

transaction: leave unfinished without crashing when not properly released
ClosedPublic

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

Details

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
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

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.