Fix thread-unsafe init cache rebuild and unmask errors - #1171
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1171 +/- ##
==========================================
+ Coverage 86.73% 86.75% +0.01%
==========================================
Files 9 9
Lines 5321 5336 +15
==========================================
+ Hits 4615 4629 +14
- Misses 706 707 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hoxbro
reviewed
Aug 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Concurrent code that temporarily toggles Parameter slots (the pattern Panel uses in
panel.util.parameters.edit_readonly) can crash with a completely misleading error:_cls_parametersobviously does exist, which makes this very hard to diagnose.Root cause
Two independent issues compound each other.
1. The init cache rebuild is not thread safe. Assigning
constant,instantiateordefault_factoryon a Parameter callsParameter._invalidate_init_cache(), which resetsparams_to_deepcopy,params_to_refandparams_with_default_factoryon the owner's_ClassPrivatetoNone. The cached-parameters branch of_cls_parametersrebuilt thoselists by assigning
[]to each attribute and then appending to them through the attributewhile iterating the parameters. If another thread invalidated the caches part way through
that loop, the next
private.params_to_ref.append(pobj)hitNoneand raisedAttributeError: 'NoneType' object has no attribute 'append'.2.
Parameters.__getattr__masked the real error._cls_parametersis a property, andan
AttributeErrorraised inside a property getter is indistinguishable from a missingattribute: CPython clears it and dispatches to
__getattr__.Parameters.__getattr__thenevaluated
attr in self_._cls_parameters, which on this retry succeeded (the three cacheattributes had already been reassigned by the failed call), found that
'_cls_parameters'is not a parameter name, and raised the bogus "has no attribute" error. The real exception
and its traceback were gone, with no
__context__to follow. Had the retry failed too, theresult would have been a
RecursionErrorinstead.Panel triggers this because
_stateis a process-wide singleton whosebusycounter isupdated from multiple threads, and every
edit_readonly(state)enter and exit flipsconstanton every one of its parameters.Changes
Parameter.__setattr__no longer invalidates the init caches when one of the cacheattributes is set to the value it already has. The caches are derived purely from those
attributes, so a no-op set cannot change them. This removes the invalidation entirely for
the save/restore pattern above, which also avoids repeatedly rebuilding the caches for
every parameter of the class.
Parameters._cls_parametersnow builds the three lists in locals and only publishes themonce complete, matching what the cold path already did. A concurrent invalidation can no
longer be observed mid-rebuild.
Parameters.__getattr__no longer masks errors raised by this class' own descriptors. Itreads the cached class parameters directly from
_param__private.paramsrather than goingthrough the
_cls_parametersproperty, so it cannot recurse into the property that sent itthere, and it falls back to re-invoking the descriptor via the new
_invoke_descriptorhelper when the requested attribute is not a parameter but does exist on the class. The
real error then propagates with its own traceback.
Notes
Reading
_param__private.paramsdirectly in__getattr__also skips the derived cachechecks that the property performs but that
__getattr__does not need, soobj.param.<name>access gets slightly faster (roughly 490ns to 450ns per access locally).
Tests
Four tests in
tests/testparameterizedobject.py, all failing before this change:test_no_op_slot_set_does_not_invalidate_init_cachetest_cls_parameters_rebuild_survives_concurrent_invalidation, a deterministicsimulation of an invalidation landing in the middle of the rebuild loop
test_cls_parameters_rebuild_is_thread_safe, four threads running theedit_readonlysave/flip/restore pattern, which reproduced the original error in 6 of 6 runs before the
fix
test_param_namespace_getattr_does_not_mask_descriptor_errorsVerified end to end against Panel as well: eight threads hammering
state._add_busy_event/_remove_busy_eventreproduce the reported_cls_parameterserrorwithin seconds on
mainand run clean with this change.Note that
edit_readonlyremains unsafe under concurrency for a separate reason (twooverlapping calls can snapshot each other's temporarily relaxed
readonly/constantvalues and restore the wrong ones). That needs fixing on the Panel side.
AI Disclosure
Fix developed with assistance from Claude Opus 5