Skip to content

Read _expire_at only once in has_expired()#1045

Merged
Kludex merged 1 commit into
pydantic:mainfrom
mbeijen:fix-expire-at-race
Jun 26, 2026
Merged

Read _expire_at only once in has_expired()#1045
Kludex merged 1 commit into
pydantic:mainfrom
mbeijen:fix-expire-at-race

Conversation

@mbeijen

@mbeijen mbeijen commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Backport of encode/httpcore#1090.

Summary

In has_expired() the _expire_at attribute was read twice: once for the is not None guard and once for the comparison. On free-threaded builds another thread may reset it to None between the two reads, raising a TypeError. Read it once into a local instead.

Review in cubic

Backport of encode/httpcore#1090.

In has_expired() the _expire_at attribute was read twice: once for the
`is not None` guard and once for the comparison. On free-threaded builds
another thread may reset it to None between the two reads, raising a
TypeError. Read it once into a local instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot 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.

No issues found across 4 files

Re-trigger cubic

@codspeed-hq

codspeed-hq Bot commented Jun 25, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 15 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing mbeijen:fix-expire-at-race (ff2953e) with main (82b9e2d)

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Kludex

Kludex commented Jun 25, 2026

Copy link
Copy Markdown
Member

And is locking a bad idea here?

@mbeijen

mbeijen commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

And is locking a bad idea here?

I don't think locking helps here. has_expired() is sync but _state_lock is an AsyncLock, so locking would mean making it (and is_available/is_idle and their pool callers) async which is big change for little gain. These are meant to be cheap lock-free snapshots anyway.

The only real bug was reading _expire_at twice and racing with the = None reset on free-threaded builds, which crashes

@Kludex Kludex merged commit 70fac53 into pydantic:main Jun 26, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants