Skip to content

fix(http-client-python): make Model.__new__ lazy init thread-safe - #11236

Open
Libba Lawrence (l0lawrence) wants to merge 6 commits into
mainfrom
fix-python-model-new-thread-safety
Open

Libba Lawrence (l0lawrence) wants to merge 6 commits into
mainfrom
fix-python-model-new-thread-safety

Conversation

@l0lawrence

@l0lawrence Libba Lawrence (l0lawrence) commented Jul 13, 2026 •

Copy link
Copy Markdown
Member

Fixes #11234 (from Azure/azure-sdk-for-python#47426).

What was going wrong

Every generated model class builds a small table of its fields (_attr_to_rest_field) the first time you create one of its objects, then caches it on the class.

If your program creates the first objects of several related models at the same time from multiple threads (for example, an app handling its first requests from a thread pool right after startup), that first-time setup could crash with RuntimeError: dictionary changed size during iteration — one thread was reading a parent class's attributes while another thread was writing the cache onto that same parent.

The crash never surfaced, because deserialization catches any error and falls back to returning the raw JSON. So instead of an error, you'd silently get back a plain dict (or a model with raw-dict fields inside) instead of a real model object — only on the first few responses after startup, only under concurrency. Very hard to diagnose.

The fix (no locks)

Reworked the first-time setup in Model.__new__ so it's safe when several threads hit it at once:

  1. Read from a copy. We now copy each class's attributes before reading them (dict(mro_class.__dict__)), so another thread writing the cache can't change what we're reading mid-loop. This removes the crash. The copy has all the same fields, because a class's fields are fixed when it's defined — only cached, _-prefixed values get added later, and those are filtered out anyway.
  2. Save the finished table last, in one step. The extra maps (backcompat, XML plan) are saved first; _attr_to_rest_field is built fully in a local variable and assigned last. So the moment another thread sees it, everything is ready — no half-built state.
  3. Use the cache itself as the "done" flag. We check "_attr_to_rest_field" not in cls.__dict__ (this class's own attributes) instead of a shared set that every class wrote into. This is correct per-class and removes another piece of unsynchronized shared state.

Out of scope (follow-up)

Deserialization still silently swallows any error and returns the raw JSON. Fixing the race removes this trigger, but that broad catch would hide future errors the same way — worth a separate discussion rather than changing fallback behavior across every SDK here.

@microsoft-github-policy-service microsoft-github-policy-service Bot added the emitter:client:python Issue for the Python client emitter: @typespec/http-client-python label Jul 13, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jul 13, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@typespec/http-client-python@11236

commit: d14af9b

@l0lawrence Libba Lawrence (l0lawrence) changed the title fix(http-client-python): make Model.__new__ lazy init thread-safe without locks fix(http-client-python): make Model.__new__ lazy init thread-safe Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

All changed packages have been documented.

  • ✅ @typespec/http-client-python
Show changes

@typespec/http-client-python - fix ✏️

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__.

@azure-sdk-automation

azure-sdk-automation Bot commented Jul 13, 2026 •

Copy link
Copy Markdown

You can try these changes here

🛝 Playground 🌐 Website 🛝 VSCode Extension

@l0lawrence
Libba Lawrence (l0lawrence) force-pushed the fix-python-model-new-thread-safety branch from d7347da to 10c448d Compare July 13, 2026 20:03
…hout locks

Concurrent first-time model construction could raise
"RuntimeError: dictionary changed size during iteration" in Model.__new__,
which deserialization silently swallowed, returning raw dicts instead of
models. Iterate a snapshot of each class __dict__ and publish
_attr_to_rest_field atomically, guarded by the class's own __dict__.

Fixes #11234

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@l0lawrence
Libba Lawrence (l0lawrence) force-pushed the fix-python-model-new-thread-safety branch from 10c448d to 4451ec6 Compare July 13, 2026 20:05
@github-actions

github-actions Bot commented Jul 24, 2026 •

Copy link
Copy Markdown
Contributor

Python emitter diff

Baseline gh:e27676a164a0bdbe14b4052ba1a5f50d5555c642 vs this PR.

Diff summary: 210 file(s), +3998 / -1898

Rendered diff: inline on the run summary, or the emitter-diff-html artifact.

Informational check (eng/emitter-diff); does not block the PR.

@microsoft-github-policy-service microsoft-github-policy-service Bot added the stale Mark a PR that hasn't been recently updated and will be closed. label Aug 11, 2026
@microsoft-github-policy-service

Copy link
Copy Markdown
Contributor

Hi @Libba Lawrence (@l0lawrence). Your PR has had no update for 14 days and it is marked as a stale PR. If it is not updated within 14 additional days, the PR will automatically be closed. If you want to refresh the PR, please remove the stale label.

@microsoft-github-policy-service

Copy link
Copy Markdown
Contributor

Hi @Libba Lawrence (@l0lawrence). The PR will be closed since the PR has no update for 28 days. If this is still relevant please reopen.

@iscai-msft iscai-msft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, wondering if you've noticed a perf hit?

@l0lawrence

Libba Lawrence (l0lawrence) commented Nov 10, 2026 •

Copy link
Copy Markdown
Member Author

looks good, wondering if you've noticed a perf hit?

I can regenerate an sdk and test it

7–11% slowdown for individual polymorphic models in the synthetic benchmark

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A critical concurrency race remains in discriminated-model deserialization: cache assignment can interrupt iteration over the class dictionary, causing a suppressed RuntimeError and a raw-dict result; the reader needs a snapshot and discriminated-model regression coverage.

1 open issue
What changed in this PR

This PR addresses a race in the Python emitter’s generated model initialization that can silently return raw dictionaries during concurrent first-time deserialization.

Changes:

  • Snapshot class attributes before scanning field metadata.
  • Publish the completed field cache last and use it as the per-class initialization flag.
  • Add concurrent deserialization regression coverage and a changelog entry.
File Description
packages/​http-client-python/​tests/​unit/​test_model_base_concurrency.py Adds concurrent first-use tests with nested models.
packages/​http-client-python/​generator/​pygen/​codegen/​templates/​model_base.py.jinja2 Updates lazy metadata initialization and cache publication.
.chronus/​changes/​fix-model-new-thread-safety-2026-7-13-11-33-46.md Documents the race-condition fix for the Python emitter.

Review effort: Balanced

@microsoft-github-policy-service microsoft-github-policy-service Bot removed the stale Mark a PR that hasn't been recently updated and will be closed. label Nov 10, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings November 10, 2026 21:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues are reported for the Python model-initialization concurrency fix.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

emitter:client:python Issue for the Python client emitter: @typespec/http-client-python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[http-client-python] model.__new__ investigation

3 participants