From 046f9fe39281d73781e24e43010908a912914c14 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Thu, 16 Nov 2023 14:40:06 +0100 Subject: [PATCH 1/2] LockRoster.modify: no KeyError if element was already gone, fixes #7937 The intention of LockRoster.modify(key, REMOVE) is to remove self.id. Using set.discard will just ignore it if self.id is not present there anymore. Previously, using set.remove triggered a KeyError that has been frequently seen in tracebacks of teardowns involving Repository.__del__ and Repository.__exit__. I added a REMOVE2 op to serve one caller that needs to get the KeyError if self.id was not present. Thanks to @herrmanntom for the workaround! --- src/borg/locking.py | 9 +++++++-- src/borg/testsuite/locking.py | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/borg/locking.py b/src/borg/locking.py index ca64eeb06..59508180d 100644 --- a/src/borg/locking.py +++ b/src/borg/locking.py @@ -8,7 +8,7 @@ from . import platform from .helpers import Error, ErrorWithTraceback from .logger import create_logger -ADD, REMOVE = "add", "remove" +ADD, REMOVE, REMOVE2 = "add", "remove", "remove2" SHARED, EXCLUSIVE = "shared", "exclusive" logger = create_logger(__name__) @@ -326,6 +326,11 @@ class LockRoster: if op == ADD: elements.add(self.id) elif op == REMOVE: + # note: we ignore it if the element is already not present anymore. + # this has been frequently seen in teardowns involving Repository.__del__ and Repository.__exit__. + elements.discard(self.id) + elif op == REMOVE2: + # needed for callers that do not want to ignore. elements.remove(self.id) else: raise ValueError("Unknown LockRoster op %r" % op) @@ -340,7 +345,7 @@ class LockRoster: killing, self.kill_stale_locks = self.kill_stale_locks, False try: try: - self.modify(key, REMOVE) + self.modify(key, REMOVE2) except KeyError: # entry was not there, so no need to add a new one, but still update our id self.id = new_id diff --git a/src/borg/testsuite/locking.py b/src/borg/testsuite/locking.py index ea1bdb5f5..131fdef3a 100644 --- a/src/borg/testsuite/locking.py +++ b/src/borg/testsuite/locking.py @@ -280,7 +280,7 @@ class TestLock: assert roster.get(SHARED) == {our_id} assert roster.get(EXCLUSIVE) == set() assert roster.get(SHARED) == set() - with pytest.raises(KeyError): + with pytest.raises(NotLocked): dead_lock.release() with Lock(lockpath, id=cant_know_if_dead_id, exclusive=True): From 2d86b7e1ac24c3413f885ee08b815d5553945ed9 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sat, 18 Nov 2023 18:31:43 +0100 Subject: [PATCH 2/2] add type annotation for mypy --- src/borg/repository.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/borg/repository.py b/src/borg/repository.py index bdc801b45..d16c461f1 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -11,6 +11,7 @@ from configparser import ConfigParser from datetime import datetime, timezone from functools import partial from itertools import islice +from typing import Callable, DefaultDict from .constants import * # NOQA from .hashindex import NSIndexEntry, NSIndex, NSIndex1, hashindex_variant @@ -48,7 +49,7 @@ TAG_PUT2 = 3 # may not be able to handle the new tags. MAX_TAG_ID = 15 -FreeSpace = partial(defaultdict, int) +FreeSpace: Callable[[], DefaultDict] = partial(defaultdict, int) def header_size(tag):