Fix races during IDS metadata construction

docs/source/changelog.rst

@@ -3,6 +3,16 @@
 Changelog
 =========
 
+Unreleased
+----------
+
+Bug fixes
+'''''''''
+
+- :issue:`130`: prevent concurrent metadata construction from changing the global
+  ``IDSMetadata`` class or exposing an incomplete node type map. This does not
+  provide general thread safety for IDS access or IMAS-Core operations.
+
 What's new in IMAS-Python 2.3.0
 -------------------------------
 

docs/source/metadata.rst

@@ -8,6 +8,15 @@ associated with elements in the IDS, such as coordinate information, units, etc.
 IMAS-Python provides the :py:class:`~imas.ids_metadata.IDSMetadata` API for
 interacting with this metadata.
 
+Metadata attributes are read-only and may be shared by independent IDS instances.
+Constructing metadata for another IDS does not disable this protection.
+
+.. note::
+    This does not make IMAS-Python generally thread-safe. Do not access the same
+    IDS or its substructures simultaneously from multiple threads. IMAS-Core
+    ``put``, ``get``, ``serialize`` and ``deserialize`` operations are not
+    thread-safe either.
+
 On this page you find several examples for querying and using the metadata of
 IDS elements.
 

imas/ids_metadata.py

@@ -67,14 +67,7 @@ def get_toplevel_metadata(structure_xml: Element) -> "IDSMetadata":
     if not _type_map:
         _build_type_map()
 
-    # Delete the custom __setattr__ so __init__ can assign values:
-    orig_setattr = IDSMetadata.__setattr__
-    del IDSMetadata.__setattr__
-    try:
-        return IDSMetadata(structure_xml, "", None)
-    finally:
-        # Always restore the custom __setattr__ to avoid accidental data changes
-        IDSMetadata.__setattr__ = orig_setattr
+    return IDSMetadata(structure_xml, "", None)
 
 
 _type_map: Dict[Tuple[Optional[IDSDataType], int], Type] = {}
@@ -98,18 +91,24 @@ def _build_type_map():
     from imas.ids_structure import IDSStructure
     from imas.ids_toplevel import IDSToplevel
 
-    _type_map[(None, 0)] = IDSToplevel
-    _type_map[(IDSDataType.STRUCTURE, 0)] = IDSStructure
-    _type_map[(IDSDataType.STRUCT_ARRAY, 1)] = IDSStructArray
-    _type_map[(IDSDataType.STR, 0)] = IDSString0D
-    _type_map[(IDSDataType.STR, 1)] = IDSString1D
-    _type_map[(IDSDataType.INT, 0)] = IDSInt0D
-    _type_map[(IDSDataType.FLT, 0)] = IDSFloat0D
-    _type_map[(IDSDataType.CPX, 0)] = IDSComplex0D
+    type_map = {
+        (None, 0): IDSToplevel,
+        (IDSDataType.STRUCTURE, 0): IDSStructure,
+        (IDSDataType.STRUCT_ARRAY, 1): IDSStructArray,
+        (IDSDataType.STR, 0): IDSString0D,
+        (IDSDataType.STR, 1): IDSString1D,
+        (IDSDataType.INT, 0): IDSInt0D,
+        (IDSDataType.FLT, 0): IDSFloat0D,
+        (IDSDataType.CPX, 0): IDSComplex0D,
+    }
     for dim in range(1, 7):
-        _type_map[(IDSDataType.INT, dim)] = IDSNumericArray
-        _type_map[(IDSDataType.FLT, dim)] = IDSNumericArray
-        _type_map[(IDSDataType.CPX, dim)] = IDSNumericArray
+        type_map[(IDSDataType.INT, dim)] = IDSNumericArray
+        type_map[(IDSDataType.FLT, dim)] = IDSNumericArray
+        type_map[(IDSDataType.CPX, dim)] = IDSNumericArray
+
+    # Concurrent constructors must only see a complete type map.
+    global _type_map
+    _type_map = type_map
 
 
 class IDSMetadata:
@@ -257,11 +256,16 @@ def __init__(
         # AL expects ndim of STR types to be one more (STR_0D is 1D array of chars)
         self._al_ndim = self.ndim + (self.data_type is IDSDataType.STR)
 
+        # Freeze this node without changing the class or other metadata instances.
+        object.__setattr__(self, "_initialized", True)
+
     def __repr__(self) -> str:
         return f"<IDSMetadata for '{self.name}'>"
 
     def __setattr__(self, name: str, value: Any) -> None:
-        raise RuntimeError("Cannot set attribute: IDSMetadata is read-only.")
+        if self.__dict__.get("_initialized", False):
+            raise RuntimeError("Cannot set attribute: IDSMetadata is read-only.")
+        object.__setattr__(self, name, value)
 
     def __delattr__(self, name: str) -> None:
         raise RuntimeError("Cannot delete attribute: IDSMetadata is read-only.")

imas/test/test_hash.py

@@ -27,7 +27,7 @@ def test_hash_str1d(minimal):
         "multiple entries to test!",
     ]
     minimal.str_1d = string_list
-    hashes = list(map(xxh3_64_digest, string_list))
+    hashes = [xxh3_64_digest(value.encode("utf-8")) for value in string_list]
     expected = xxh3_64_digest(struct.pack("<Q", len(string_list)) + b"".join(hashes))
     assert expected == b"\x98\x011\x9dx+\x0e\xc0"
     assert imas.util.calc_hash(minimal.str_1d) == expected

imas/test/test_ids_metadata.py

@@ -1,9 +1,13 @@
+from concurrent.futures import ThreadPoolExecutor
 from copy import deepcopy
+from threading import Barrier, Event
 
 import pytest
 
+from imas import ids_metadata
 from imas.ids_factory import IDSFactory
-from imas.ids_metadata import IDSType, get_toplevel_metadata
+from imas.ids_metadata import IDSMetadata, IDSType, get_toplevel_metadata
+from imas.util import idsdiffgen
 
 
 def test_metadata_cache(fake_structure_xml):
@@ -35,6 +39,154 @@ def test_metadata_immutable(fake_structure_xml):
         del meta.name
 
 
+def assert_metadata_immutable(meta):
+    for node in (meta, meta["ids_properties"], meta["ids_properties/comment"]):
+        with pytest.raises(RuntimeError, match="IDSMetadata is read-only"):
+            node.name = node.name
+        with pytest.raises(RuntimeError, match="IDSMetadata is read-only"):
+            node.new_attribute = True
+        with pytest.raises(RuntimeError, match="IDSMetadata is read-only"):
+            del node.name
+
+
+@pytest.fixture
+def paused_metadata_init(monkeypatch):
+    """Pause the first build at a child node, with bounded waits and cleanup."""
+    factory = IDSFactory("3.39.0")
+    # Fresh XML elements ensure cache misses regardless of test order.
+    factory._ids_elements = {
+        name: deepcopy(factory._ids_elements[name])
+        for name in ("core_profiles", "equilibrium")
+    }
+    first_child = factory._ids_elements["core_profiles"][0]
+    entered = Event()
+    resume = Event()
+    original_init = IDSMetadata.__init__
+    original_setattr = IDSMetadata.__setattr__
+
+    def init(self, structure_xml, context_path, parent_meta):
+        if structure_xml is first_child and not entered.is_set():
+            entered.set()
+            assert resume.wait(10), "Metadata construction was not resumed"
+        original_init(self, structure_xml, context_path, parent_meta)
+
+    # Restore the class even when running this regression against the old code.
+    monkeypatch.setattr(IDSMetadata, "__setattr__", original_setattr)
+    monkeypatch.setattr(IDSMetadata, "__init__", init)
+    with ThreadPoolExecutor(max_workers=2) as executor:
+        first = executor.submit(factory.core_profiles)
+        try:
+            assert entered.wait(10), "Metadata construction did not reach a child"
+            yield factory, executor, first, resume
+        finally:
+            resume.set()
+
+
+@pytest.mark.parametrize("second_name", ["equilibrium", "core_profiles"])
+def test_concurrent_metadata_construction(paused_metadata_init, second_name):
+    factory, executor, first, resume = paused_metadata_init
+    second = executor.submit(factory.new, second_name).result(timeout=10)
+    assert second.metadata.name == second_name
+    assert_metadata_immutable(second.metadata)
+    resume.set()
+    first = first.result(timeout=10)
+    assert first.metadata.name == "core_profiles"
+    assert_metadata_immutable(first.metadata)
+    assert_metadata_immutable(second.metadata)
+
+
+def test_metadata_immutable_during_construction(fake_structure_xml, monkeypatch):
+    existing = get_toplevel_metadata(fake_structure_xml)
+    entered = Event()
+    resume = Event()
+    original_init = IDSMetadata.__init__
+
+    def init(self, structure_xml, context_path, parent_meta):
+        if parent_meta is None:
+            entered.set()
+            assert resume.wait(10), "Metadata construction was not resumed"
+        original_init(self, structure_xml, context_path, parent_meta)
+
+    monkeypatch.setattr(IDSMetadata, "__init__", init)
+    with ThreadPoolExecutor(max_workers=1) as executor:
+        future = executor.submit(get_toplevel_metadata, deepcopy(fake_structure_xml))
+        try:
+            assert entered.wait(10), "Metadata construction did not start"
+            assert_metadata_immutable(existing)
+        finally:
+            resume.set()
+        assert_metadata_immutable(future.result(timeout=10))
+    assert_metadata_immutable(existing)
+
+
+def test_metadata_failed_construction(fake_structure_xml):
+    existing = get_toplevel_metadata(deepcopy(fake_structure_xml))
+    child = fake_structure_xml[0]
+    child.set("maxoccur", "invalid")
+    with pytest.raises(ValueError):
+        get_toplevel_metadata(fake_structure_xml)
+    assert_metadata_immutable(existing)
+    del child.attrib["maxoccur"]
+    assert_metadata_immutable(get_toplevel_metadata(fake_structure_xml))
+
+
+def test_concurrent_metadata_type_map_initialization(fake_structure_xml, monkeypatch):
+    entered = Event()
+    resume = Event()
+
+    def paused_range(*args):
+        # Stop before the numeric array types are added to the table.
+        if args == (1, 7) and not entered.is_set():
+            entered.set()
+            assert resume.wait(10), "Type map initialization was not resumed"
+        return range(*args)
+
+    monkeypatch.setattr(ids_metadata, "_type_map", {})
+    monkeypatch.setattr(ids_metadata, "range", paused_range, raising=False)
+    with ThreadPoolExecutor(max_workers=2) as executor:
+        first = executor.submit(get_toplevel_metadata, fake_structure_xml)
+        try:
+            assert entered.wait(10), "Type map initialization did not start"
+            second = executor.submit(
+                get_toplevel_metadata, deepcopy(fake_structure_xml)
+            ).result(timeout=10)
+        finally:
+            resume.set()
+        assert_metadata_immutable(first.result(timeout=10))
+    assert_metadata_immutable(second)
+
+
+@pytest.mark.parametrize("version, ion_name", [("3.39.0", "label"), ("4.0.0", "name")])
+def test_independent_ids_filled_concurrently(version, ion_name):
+    workers = 4
+    barrier = Barrier(workers, timeout=10)
+
+    def fill(index, parallel=False):
+        if parallel:
+            barrier.wait()
+        ids = IDSFactory(version).core_profiles()
+        ids.ids_properties.homogeneous_time = 1
+        ids.ids_properties.comment = f"Independent IDS {index}"
+        ids.time = [0.0, 1.0]
+        ids.profiles_1d.resize(2)
+        for profile in ids.profiles_1d:
+            profile.grid.rho_tor_norm = [0.0, 0.5, 1.0]
+            profile.electrons.temperature = [index + 1.0, index + 2.0, index + 3.0]
+            profile.ion.resize(1)
+            setattr(profile.ion[0], ion_name, f"D{index}")
+            profile.ion[0].density = [index + 4.0, index + 5.0, index + 6.0]
+        ids.validate()
+        return ids
+
+    expected = [fill(index) for index in range(workers)]
+    with ThreadPoolExecutor(max_workers=workers) as executor:
+        futures = [executor.submit(fill, index, True) for index in range(workers)]
+        actual = [future.result(timeout=10) for future in futures]
+    for serial, parallel in zip(expected, actual):
+        assert list(idsdiffgen(serial, parallel)) == []
+        assert_metadata_immutable(parallel.metadata)
+
+
 def test_ids_type():
     assert not IDSType.NONE.is_dynamic
     assert not IDSType.CONSTANT.is_dynamic