Look up a vlmeta name once per read

src/blosc2/blosc2_ext.pyx

@@ -237,6 +237,7 @@ cdef extern from "blosc2.h":
         BLOSC2_ERROR_SCHUNK_SPECIAL
         BLOSC2_ERROR_PLUGIN_IO
         BLOSC2_ERROR_FILE_REMOVE
+        BLOSC2_ERROR_NULL_POINTER
 
     ctypedef enum:
         BLOSC2_DEFINED_CODECS_START
@@ -5728,17 +5729,23 @@ cdef class vlmeta:
     @_handle_locked
     def get_vlmeta(self, name):
         name = name.encode("utf-8") if isinstance(name, str) else name
-        rc = blosc2_vlmeta_exists(self.schunk, name)
         cdef uint8_t* content
         cdef int32_t content_len
-        if rc < 0:
+        # One lookup (for a file-backed frame, each lookup reads the frame header).  These four errors
+        # come from reading the content; the others are lookup errors (not found, name too long, frame
+        # re-sync or lock), which were a KeyError from a blosc2_vlmeta_exists() call before and still are.
+        cdef int rc = blosc2_vlmeta_get(self.schunk, name, &content, &content_len)
+        if rc < 0 and rc not in (BLOSC2_ERROR_DATA, BLOSC2_ERROR_MEMORY_ALLOC, BLOSC2_ERROR_NULL_POINTER,
+                                 BLOSC2_ERROR_READ_BUFFER):
             raise KeyError
-        if rc >= 0:
-            rc = blosc2_vlmeta_get(self.schunk, name, &content, &content_len)
+        # On errors, blosc2_vlmeta_get() leaves nothing to free
         if rc < 0:
             _raise_callback_error()
             raise RuntimeError
-        return content[:content_len]
+        try:
+            return content[:content_len]
+        finally:
+            free(content)
 
     @_handle_locked
     def del_vlmeta(self, name):
@@ -5754,22 +5761,26 @@ cdef class vlmeta:
     @_handle_locked
     def get_names(self):
         cdef char** names = <char **> malloc(self.schunk.nvlmetalayers * sizeof (char *))
-        rc = blosc2_vlmeta_get_names(self.schunk, names)
-        if rc != self.schunk.nvlmetalayers:
-            raise RuntimeError
-        res = [names[i].decode("utf-8") for i in range(rc)]
-        return res
+        try:
+            rc = blosc2_vlmeta_get_names(self.schunk, names)
+            if rc != self.schunk.nvlmetalayers:
+                raise RuntimeError
+            return [names[i].decode("utf-8") for i in range(rc)]
+        finally:
+            free(names)
 
     @_handle_locked
     def to_dict(self):
         cdef char** names = <char **> malloc(self.schunk.nvlmetalayers * sizeof (char*))
-        rc = blosc2_vlmeta_get_names(self.schunk, names)
-        if rc != self.schunk.nvlmetalayers:
-            raise RuntimeError
-        res = {}
-        for i in range(rc):
-            res[names[i]] = unpackb(self.get_vlmeta(names[i]), list_hook=decode_tuple)
-        return res
+        try:
+            rc = blosc2_vlmeta_get_names(self.schunk, names)
+            if rc != self.schunk.nvlmetalayers:
+                raise RuntimeError
+            # Copies (as bytes keys): the names point into the vlmetalayers, which get_vlmeta() may re-sync
+            keys = [names[i] for i in range(rc)]
+        finally:
+            free(names)
+        return {key: unpackb(self.get_vlmeta(key), list_hook=decode_tuple) for key in keys}
 
 
 @_handle_locked
@@ -5782,20 +5793,25 @@ def meta__contains__(self, name):
 @_handle_locked
 def meta__getitem__(self, name):
     cdef blosc2_schunk *schunk = <blosc2_schunk *><uintptr_t> self.c_schunk
-    name = name.encode("utf-8") if isinstance(name, str) else name
+    name_ = name.encode("utf-8") if isinstance(name, str) else name
     cdef uint8_t *content
     cdef int32_t content_len
-    n = blosc2_meta_get(schunk, name, &content, &content_len)
-    res = PyBytes_FromStringAndSize(<char *> content, content_len)
-    free(content)
-
-    return res
+    cdef int n = blosc2_meta_get(schunk, name_, &content, &content_len)
+    # On errors, blosc2_meta_get() leaves nothing to free (content may be uninitialized)
+    if n == BLOSC2_ERROR_NOT_FOUND:
+        raise KeyError(f"{name} not found")
+    if n < 0:
+        raise RuntimeError("Could not get the metalayer")
+    try:
+        return PyBytes_FromStringAndSize(<char *> content, content_len)
+    finally:
+        free(content)
 
 @_handle_locked
 def meta__setitem__(self, name, content):
     cdef blosc2_schunk *schunk = <blosc2_schunk *><uintptr_t> self.c_schunk
-    name = name.encode("utf-8") if isinstance(name, str) else name
     old_content = meta__getitem__(self, name)
+    name = name.encode("utf-8") if isinstance(name, str) else name
     if len(old_content) != len(content):
         raise ValueError("The length of the content in a metalayer cannot change.")
     blosc2_meta_update(schunk, name, content, len(content))

tests/test_schunk.py

@@ -186,6 +186,15 @@ def test_schunk(contiguous, urlpath, mode, mmap_mode, nbytes, cparams, dparams,
     blosc2.remove_urlpath(urlpath)
 
 
+def test_schunk_meta_setitem_missing_name():
+    # This used to copy and free an uninitialized pointer, which aborted the process
+    schunk = blosc2.SChunk(meta={"test": 1})
+    with pytest.raises(KeyError, match="missing not found"):
+        schunk.meta["missing"] = 2
+    assert "missing" not in schunk.meta
+    assert schunk.meta["test"] == 1
+
+
 def test_schunk_info_has_human_sizes():
     schunk = blosc2.SChunk(chunksize=32)
     schunk.append_data(b"a" * 32)

tests/test_swmr.py

@@ -136,6 +136,21 @@ def test_vlmeta_follows(tmp_path):
     assert r.schunk.vlmeta["heartbeat"] == "brand-new and longer"
 
 
+def test_vlmeta_to_dict_follows(tmp_path):
+    # to_dict() used to keep pointers to the cached names across its first read, which re-syncs
+    # (frees and reloads) the vlmetalayers after another handle has rewritten them
+    urlpath = tmp_path / "vlmeta_to_dict.b2nd"
+    w = create_array(urlpath)
+    for i in range(8):
+        w.schunk.vlmeta[f"name{i}"] = i
+    r = blosc2.open(str(urlpath))
+
+    w.schunk.vlmeta["name0"] = "brand-new and longer"
+    expected = {f"name{i}".encode(): i for i in range(8)}
+    expected[b"name0"] = "brand-new and longer"
+    assert r.schunk.vlmeta.to_dict() == expected
+
+
 WRITER_SCRIPT = """
 import sys
 import numpy as np

tests/test_vlmeta.py

@@ -5,6 +5,9 @@
 # SPDX-License-Identifier: BSD-3-Clause
 #######################################################################
 
+import os
+import sys
+
 import numpy as np
 import pytest
 
@@ -150,3 +153,65 @@ def test_vlmeta_empty_list_roundtrip():
 
     assert schunk.vlmeta["empty"] == []
     assert schunk.vlmeta["nested"] == {"rows": [[], ["x"]]}
+
+
+def _rss_bytes():
+    with open("/proc/self/statm") as f:
+        return int(f.read().split()[1]) * os.sysconf("SC_PAGE_SIZE")
+
+
+def _rss_growth(op, n):
+    for _ in range(10):
+        op()
+    rss = _rss_bytes()
+    for _ in range(n):
+        op()
+    return _rss_bytes() - rss
+
+
+@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="reads the RSS from /proc")
+def test_vlmeta_read_frees_content():
+    # Each read used to leak the 1 MiB content (200 MiB here)
+    schunk = blosc2.SChunk()
+    schunk.vlmeta["big"] = bytes(2**20)
+    assert _rss_growth(lambda: schunk.vlmeta["big"], 200) < 50 * 2**20
+
+
+@pytest.mark.skipif(not sys.platform.startswith("linux"), reason="reads the RSS from /proc")
+def test_vlmeta_names_are_freed():
+    # Each listing used to leak an array of 4096 pointers (32 MiB here)
+    schunk = blosc2.SChunk()
+    for i in range(4096):
+        schunk.vlmeta[str(i)] = i
+    assert _rss_growth(lambda: list(schunk.vlmeta), 1000) < 8 * 2**20
+
+
+def _read_syscalls():
+    # Read syscalls (read, pread, ...) of this process so far
+    with open("/proc/self/io") as f:
+        return next(int(line.split()[1]) for line in f if line.startswith("syscr:"))
+
+
+@pytest.mark.skipif(not os.path.exists("/proc/self/io"), reason="needs Linux task I/O accounting")
+@pytest.mark.parametrize("contiguous", [True, False])
+def test_vlmeta_read_looks_name_up_once(tmp_path, contiguous):
+    # Each lookup re-reads the frame header from the file (one pread); reads used to look the name up twice
+    urlpath = str(tmp_path / "frame.b2frame")
+    schunk = blosc2.SChunk(urlpath=urlpath, contiguous=contiguous)
+    schunk.vlmeta["small"] = 1
+    reader = blosc2.open(urlpath, mode="r")
+    assert reader.vlmeta["small"] == 1
+    n = 100
+    syscr = _read_syscalls()
+    for _ in range(n):
+        reader.vlmeta["small"]
+    # One read syscall per vlmeta read (two before)
+    assert _read_syscalls() - syscr < 1.5 * n
+
+
+def test_vlmeta_name_too_long_is_keyerror():
+    # Longer than BLOSC2_METALAYER_NAME_MAXLEN: the lookup rejects it, so it cannot exist
+    schunk = blosc2.SChunk()
+    assert "x" * 40 not in schunk.vlmeta
+    with pytest.raises(KeyError):
+        schunk.vlmeta["x" * 40]