diff --git a/.chronus/changes/fix-model-new-thread-safety-2026-7-13-11-33-46.md b/.chronus/changes/fix-model-new-thread-safety-2026-7-13-11-33-46.md new file mode 100644 index 00000000000..a328a895ea9 --- /dev/null +++ b/.chronus/changes/fix-model-new-thread-safety-2026-7-13-11-33-46.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@typespec/http-client-python" +--- + +Fix a race condition in the generated `_model_base.py` where concurrent first-time model construction could raise `RuntimeError: dictionary changed size during iteration` in `Model.__new__`. Because deserialization swallows that error, affected responses were silently returned as raw dicts (or models with raw-dict nested fields) instead of deserialized models. The lazy metadata initialization is now thread-safe without locks: it iterates a snapshot of each class `__dict__` and publishes `_attr_to_rest_field` atomically, guarded by the class's own `__dict__`. diff --git a/packages/http-client-python/generator/pygen/codegen/templates/model_base.py.jinja2 b/packages/http-client-python/generator/pygen/codegen/templates/model_base.py.jinja2 index 54821400d89..fbe2638fc0e 100644 --- a/packages/http-client-python/generator/pygen/codegen/templates/model_base.py.jinja2 +++ b/packages/http-client-python/generator/pygen/codegen/templates/model_base.py.jinja2 @@ -881,9 +881,6 @@ def _build_xml_field_plan( # pylint: disable=docstring-missing-return, docstrin # pylint: enable=docstring-missing-param class Model(_MyMutableMapping): _is_model = True - # label whether current class's _attr_to_rest_field has been calculated - # could not see _attr_to_rest_field directly because subclass inherits it from parent class - _calculated: set[str] = set() def __init__(self, *args: typing.Any, **kwargs: typing.Any) -> None: class_name = self.__class__.__name__ @@ -1024,18 +1021,28 @@ class Model(_MyMutableMapping): return Model(self.__dict__) def __new__(cls, *args: typing.Any, **kwargs: typing.Any) -> Self: - if f"{cls.__module__}.{cls.__qualname__}" not in cls._calculated: + # First time we build this class, compute its field metadata and cache it on the class. + # Check `cls.__dict__` (this class's own attrs), not a shared set: subclasses inherit + # `_attr_to_rest_field`, so this is only True once *this* class is done. No lock needed. + if "_attr_to_rest_field" not in cls.__dict__: # we know the last nine classes in mro are going to be 'Model', '_MyMutableMapping', 'MutableMapping', # 'Mapping', 'Collection', 'Sized', 'Iterable', 'Container' and 'object' mros = cls.__mro__[:-9][::-1] # ignore parents, and reverse the mro order + # Read from a copy (`dict(...)`), not the original: another thread building a parent class + # may write its cache into that parent's attrs at the same time, and reading the original + # while it changes raises "dictionary changed size during iteration" (silently swallowed + # during deserialization, returning raw JSON). The copy has the same fields anyway. attr_to_rest_field: dict[str, _RestField] = { # map attribute name to rest_field property - k: v for mro_class in mros for k, v in mro_class.__dict__.items() if k[0] != "_" and hasattr(v, "_type") + k: v + for mro_class in mros + for k, v in dict(mro_class.__dict__).items() + if k[0] != "_" and hasattr(v, "_type") } annotations = { k: v for mro_class in mros if hasattr(mro_class, "__annotations__") - for k, v in mro_class.__annotations__.items() + for k, v in dict(mro_class.__annotations__).items() } for attr, rf in attr_to_rest_field.items(): rf._module = cls.__module__ @@ -1043,17 +1050,20 @@ class Model(_MyMutableMapping): rf._type = rf._get_deserialize_callable_from_annotation(annotations.get(attr, None)) if not rf._rest_name_input: rf._rest_name_input = attr - cls._attr_to_rest_field: dict[str, _RestField] = dict(attr_to_rest_field.items()) {% if code_model.has_padded_model_property %} cls._backcompat_attr_to_rest_field: dict[str, _RestField] = { - Model._get_backcompat_attribute_name(cls._attr_to_rest_field, attr): rf for attr, rf in cls - ._attr_to_rest_field.items() + Model._get_backcompat_attribute_name(attr_to_rest_field, attr): rf + for attr, rf in attr_to_rest_field.items() } {% endif %} # Build XML field plan for fast _init_from_xml (only for XML models) if getattr(cls, "_xml", None): cls._xml_field_plan = _build_xml_field_plan(cls, attr_to_rest_field) - cls._calculated.add(f"{cls.__module__}.{cls.__qualname__}") + # Set `_attr_to_rest_field` LAST: it's the "done" flag checked above, so the extra maps + # are set first and are ready before another thread sees it. Built as a local and assigned + # in one step, never mutated after - readers always see a complete map. Two threads racing + # here just build the same map twice (harmless), so no lock is needed. + cls._attr_to_rest_field: dict[str, _RestField] = dict(attr_to_rest_field) return super().__new__(cls) diff --git a/packages/http-client-python/tests/unit/test_model_base_concurrency.py b/packages/http-client-python/tests/unit/test_model_base_concurrency.py new file mode 100644 index 00000000000..c15ca2cacf3 --- /dev/null +++ b/packages/http-client-python/tests/unit/test_model_base_concurrency.py @@ -0,0 +1,116 @@ +# ------------------------------------ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. +# ------------------------------------ +"""Concurrency regression tests for ``Model`` lazy metadata initialization. + +Regression coverage for https://github.com/microsoft/typespec/issues/11234 +(upstream Azure/azure-sdk-for-python#47426). + +``Model.__new__`` lazily computes each class's ``_attr_to_rest_field`` metadata on +first construction. The original implementation iterated a live class ``__dict__`` +while another thread published its own metadata into that same ``__dict__``, raising +``RuntimeError: dictionary changed size during iteration``. That error was then +swallowed by ``_deserialize_default``, so concurrent first-time deserialization +silently returned raw ``dict`` objects (or models with raw-dict nested fields) +instead of fully deserialized models. + +Each trial below defines *fresh* model classes, so their lazy metadata is +uninitialized - exactly like the first response of a freshly started process - then +deserializes the same payload from many threads at once. +""" +import copy +import sys +import threading +from typing import List, Optional + +import pytest + +from specialwords._utils.model_base import Model, rest_field, _deserialize + + +N_THREADS = 32 +N_TRIALS = 20 + + +def _run_concurrent_deserialization(model_cls, payload, verify): + """Deserialize ``payload`` into ``model_cls`` from N_THREADS threads simultaneously. + + Returns the list of per-thread corruption descriptions (empty when all results + are fully deserialized). + """ + results = [None] * N_THREADS + barrier = threading.Barrier(N_THREADS) + + def work(i): + # Independent input per thread, like real HTTP responses. + local_payload = copy.deepcopy(payload) + # Release all threads together to maximize contention on the first-time + # lazy initialization in Model.__new__. + barrier.wait() + results[i] = _deserialize(model_cls, local_payload) + + threads = [threading.Thread(target=work, args=(i,)) for i in range(N_THREADS)] + for t in threads: + t.start() + for t in threads: + t.join() + + corruptions = [] + for r in results: + problem = verify(r) + if problem is not None: + corruptions.append(problem) + return corruptions + + +def test_concurrent_first_deserialization_is_thread_safe(): + """First-time concurrent deserialization always returns fully deserialized models.""" + + # Widen the race window (the original bug reproduces at the default interval too, + # this just makes it deterministic). Restored in the finally block. + old_switch_interval = sys.getswitchinterval() + sys.setswitchinterval(1e-6) + try: + for trial in range(N_TRIALS): + # Fresh classes each trial => uninitialized lazy metadata, like a new process. + class Inner(Model): + name: str = rest_field() + value: int = rest_field() + + class Outer(Model): + inner: Inner = rest_field() + items: List[Inner] = rest_field() + maybe: Optional[Inner] = rest_field() + + payload = { + "inner": {"name": "a", "value": 1}, + "items": [{"name": "b", "value": 2}, {"name": "c", "value": 3}], + "maybe": {"name": "d", "value": 4}, + } + + def verify(result): + if not isinstance(result, Outer): + return f"got {type(result).__name__} instead of Outer" + try: + if not isinstance(result.inner, Inner): + return f"inner is {type(result.inner).__name__}, not Inner" + if (result.inner.name, result.inner.value) != ("a", 1): + return f"inner has wrong values: {result.inner}" + if not all(isinstance(it, Inner) for it in result.items): + return "items contains a raw dict instead of Inner" + if [it.name for it in result.items] != ["b", "c"]: + return f"items has wrong values: {result.items}" + if not isinstance(result.maybe, Inner): + return f"maybe is {type(result.maybe).__name__}, not Inner" + except AttributeError as exc: # nested field is a raw dict + return f"partially deserialized model: {exc}" + return None + + corruptions = _run_concurrent_deserialization(Outer, payload, verify) + assert not corruptions, ( + f"trial {trial}: {len(corruptions)}/{N_THREADS} results corrupted, " + f"e.g. {corruptions[0]}" + ) + finally: + sys.setswitchinterval(old_switch_interval)