Lazy DataTypeTree Hardening - #1831
Merged
Merged
Conversation
Validate Browse/Read result counts (a non-compliant server returning mismatched counts silently misaligned positional lookups, caching encoding ids against the wrong DataType), bound BrowseNext against servers that echo continuation points forever, and guard the empty results array it indexed unchecked. Also hoist the lenient IOP encoding-name matching, the HasEncoding browse construction, and the StructureDefinition DefaultEncodingId fallback into shared helpers so the eager and lazy paths can't drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
attemptedResolution was checked outside the lock and populated before attempting resolution, so a thread observing another thread's in-flight resolution returned a spurious null codec for a type that resolved milliseconds later. Record only completed failures and re-validate the fast-path checks under the write lock. Also adopt the shared getBinaryEncodingId helper in place of the duplicated DefaultEncodingId fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- catch RuntimeException in ensureResolved: query methods are documented to return null on resolution failure, but malformed server responses could throw through them mid-decode - bound inverse HasSubtype traversal with a visited set and depth cap; cyclic references looped forever while holding the write lock - pin resolution to the session it started on, like DataTypeTreeBuilder, so results spanning a reconnect are not cached - stop caching types whose mandatory attribute reads fail; a failed node now ends the path instead of silently attaching its descendants to the wrong parent - return a snapshot from getTreeNode: the live node exposed callers (e.g. DataTypeManagerFactory.DefaultInitializer) to concurrent modification during traversal; isSubtypeOf becomes a direct parent walk so internal subtype checks don't pay for snapshots - answer builtin-id queries from the superclass's static tables instead of triggering network resolution first - collapse the fourteen copied query overrides into a resolvedQuery helper and adopt the shared encoding helpers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DataTypeTreeFactory.lazy() combined with the default eager DataTypeManagerFactory registers zero custom codecs: the lazy tree's Structure node has no children to traverse at initialization time. Every dynamic decode then fails with nothing pointing at the misconfiguration. Also adopt the shared getBinaryEncodingId helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dynamic DataTypeManager and EncodingContext capture the tree they were built against (each dynamic codec embeds it for field-hint resolution), so a mid-session readDataTypeTree() or setDataTypeTreeFactory() left decodes running against the discarded tree. Cascade the reset in resetDataTypeTree(), source before derivatives, so a racing rebuild can never capture the old tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ClientDataType's encoding id and definition parameters and fields genuinely accept null but were unannotated. Remove a stale @SuppressWarnings("unused") from createSeedTree, and correct header years on files added in 2026. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
clearFailedResolutionsAllowsRetry asserted an always-true disjunction; deleting the call under test would not have failed it. Assert exact outcomes with a control assertion instead. Rename diagnosticTestForStructure to describe the behavior it tests, and extract the repeated tree construction into fixture helpers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kevinherron
force-pushed
the
lazy-data-type-tree-hardening
branch
from
July 29, 2026 23:29
7aef3c1 to
761ec30
Compare
kevinherron
changed the title
Free the server-side continuation point before aborting a BrowseNext sequence that exceeds the configured iteration limit.
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
1 participant
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.
Fixes a set of correctness, concurrency, and robustness issues found in a review of the lazy
DataTypeTree/DataTypeManagerimplementation, plus some deduplication and test cleanups. One commit per logical change; each commit compiles and passes tests on its own.Correctness / concurrency
getTreeNode()returned the live, lazily-mutated tree node (unlikegetRoot(), which snapshots). Callers traversing it — includingDataTypeManagerFactory.DefaultInitializer— racedresolvePath()'saddChild()and could seeConcurrentModificationException. It now returns a snapshot with the parent chain intact;isSubtypeOfis reimplemented as a direct parent walk so internal subtype checks don't pay for snapshots.LazyClientDataTypeManager:attemptedResolutionwas checked outside the lock and populated before attempting resolution, so a thread observing an in-flight resolution returned a spurious null codec. Only completed failures are recorded now, with fast-path checks re-validated under the write lock.DataTypeManagerandEncodingContextcapture the tree they were built against (each dynamic codec embeds it), soreadDataTypeTree()/setDataTypeTreeFactory()left decodes running against the discarded tree.resetDataTypeTree()now cascades, source before derivatives.ensureResolved()only caughtUaException, so RuntimeExceptions from malformed server responses escaped query methods documented to return null on resolution failure.DataTypeTreeBuilder, so results assembled across a reconnect are never cached.Robustness against non-compliant servers
BrowseNextis bounded against servers that echo continuation points forever, and no longer indexes an empty results array unchecked.Efficiency / cleanup
getBuiltinType(Int32)etc.) no longer trigger network resolution for answers the superclass resolves statically.DefaultEncodingIdfallback each existed in two copies across the eager and lazy paths; they're now shared helpers inClientBrowseUtils.resolvedQueryhelper.@Nullableannotations, header years, a stale@SuppressWarnings, and test cleanups (vacuous assertions replaced, fixtures extracted).🤖 Generated with Claude Code