diff --git a/CHANGELOG.md b/CHANGELOG.md index fb1cce6f..c8ddb42a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Changelog ## 3.8.0.dev +* Fix: storage/multifilesystem: depth:1 PROPFIND no longer re-runs the filesystem collision check (path_to_filesystem) for every item in a collection; this made listing large collections O(n^2) on file systems not detected as collision-free +* Improve: storage/multifilesystem: avoid redundant stat() calls per item in get/upload when use_mtime_and_size_for_item_cache is enabled ## 3.7.8 * Fix: time-range filter on a VTODO having DTSTART/DUE and also CREATED/COMPLETED used the CREATED->COMPLETED duration instead of the DTSTART->DUE one, so completed tasks were missing from (or wrongly returned by) calendar-query REPORT results diff --git a/radicale/storage/multifilesystem/discover.py b/radicale/storage/multifilesystem/discover.py index ff564b79..ea0e57b0 100644 --- a/radicale/storage/multifilesystem/discover.py +++ b/radicale/storage/multifilesystem/discover.py @@ -88,7 +88,9 @@ class StoragePartDiscover(StorageBase): for href in collection._list(): with child_context_manager(sane_path, href): - item = collection._get(href) + # We don't need to check for collisions, because the file + # names are from _list() (os.scandir). + item = collection._get(href, verify_href=False) if item is not None: yield item diff --git a/radicale/storage/multifilesystem/get.py b/radicale/storage/multifilesystem/get.py index 7d5101fe..679157c2 100644 --- a/radicale/storage/multifilesystem/get.py +++ b/radicale/storage/multifilesystem/get.py @@ -68,6 +68,7 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock, return None else: path = os.path.join(self._filesystem_path, href) + item_stat: Optional[os.stat_result] = None try: if self._storage._use_mtime_and_size_for_item_cache is True: # try to avoid "open" @@ -78,6 +79,9 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock, raise IsADirectoryError(path) if not os.access(path, os.R_OK): raise PermissionError(path) + # Single stat() reused below for both the cache hash and + # last_modified, instead of stat-ing the same path 3 times. + item_stat = os.stat(path) else: with open(path, "rb") as f: # early read of the content @@ -95,7 +99,8 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock, # The hash of the component in the file system. This is used to check, # if the entry in the cache is still valid. if self._storage._use_mtime_and_size_for_item_cache is True: - cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns) + assert item_stat is not None + cache_hash = self._item_cache_mtime_and_size(item_stat.st_size, item_stat.st_mtime_ns) if self._storage._debug_cache_actions is True: logger.debug("Item cache check for: %r with mtime and size %r", path, cache_hash) else: @@ -149,7 +154,8 @@ class CollectionPartGet(CollectionPartCache, CollectionPartLock, logger.debug("Item cache hit for: %r", path) last_modified = time.strftime( "%a, %d %b %Y %H:%M:%S GMT", - time.gmtime(os.path.getmtime(path))) + time.gmtime(item_stat.st_mtime if item_stat is not None + else os.path.getmtime(path))) # Don't keep reference to ``vobject_item``, because it requires a lot # of memory. return radicale_item.Item( diff --git a/radicale/storage/multifilesystem/upload.py b/radicale/storage/multifilesystem/upload.py index 358b7004..48d626d5 100644 --- a/radicale/storage/multifilesystem/upload.py +++ b/radicale/storage/multifilesystem/upload.py @@ -50,7 +50,8 @@ class CollectionPartUpload(CollectionPartGet, CollectionPartCache, (href, self.path, e)) from e # store cache file if self._storage._use_mtime_and_size_for_item_cache is True: - cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns) + path_stat = os.stat(path) + cache_hash = self._item_cache_mtime_and_size(path_stat.st_size, path_stat.st_mtime_ns) if self._storage._debug_cache_actions is True: logger.debug("Item cache store for: %r with mtime and size %r", path, cache_hash) else: @@ -121,7 +122,8 @@ class CollectionPartUpload(CollectionPartGet, CollectionPartCache, # store cache file if self._storage._use_mtime_and_size_for_item_cache is True: - cache_hash = self._item_cache_mtime_and_size(os.stat(path).st_size, os.stat(path).st_mtime_ns) + path_stat = os.stat(path) + cache_hash = self._item_cache_mtime_and_size(path_stat.st_size, path_stat.st_mtime_ns) if self._storage._debug_cache_actions is True: logger.debug("Item cache store for: %r with mtime and size %r", path, cache_hash) else: diff --git a/radicale/tests/test_storage.py b/radicale/tests/test_storage.py index d2eb0f80..57014fc7 100644 --- a/radicale/tests/test_storage.py +++ b/radicale/tests/test_storage.py @@ -344,6 +344,41 @@ class TestMultiFileSystem(BaseTest): logs = caplog.messages assert len([log for log in logs if "File name collision" in log]) == 2 + def test_propfind_depth1_does_not_check_collisions_per_item(self) -> None: + """Listing a collection (PROPFIND depth:1) must not re-run the + filesystem collision check (path_to_filesystem) once per item. + """ + self.mkcalendar("/calendar.ics/") + item_count = 20 + for index in range(item_count): + uid = "event%d" % index + event = ("BEGIN:VCALENDAR\r\nVERSION:2.0\r\n" + "BEGIN:VEVENT\r\nUID:%s\r\nSUMMARY:Event %d\r\n" + "DTSTART:20130901T180000Z\r\nEND:VEVENT\r\n" + "END:VCALENDAR\r\n" % (uid, index)) + self.put("/calendar.ics/%s.ics" % uid, event) + + call_count = 0 + original_path_to_filesystem = pathutils.path_to_filesystem + + def counting_path_to_filesystem(*args, **kwargs): + nonlocal call_count + call_count += 1 + return original_path_to_filesystem(*args, **kwargs) + + with pytest.MonkeyPatch.context() as monkeypatch: + monkeypatch.setattr(pathutils, "path_to_filesystem", + counting_path_to_filesystem) + _, responses = self.propfind("/calendar.ics/", """\ + + + + +""", HTTP_DEPTH="1") + + assert len(responses) == item_count + 1 + assert call_count < item_count + @pytest.mark.skipif(not shutil.which("flock"), reason="flock command not found") @pytest.mark.skipif(radicale.log.logger.getEffectiveLevel() == logging.INFO, reason="requires loglevel DEBUG") def test_hook_placeholders_PUT(self, caplog) -> None: