fix(node): guard node_ptr_mut with RefCell::try_borrow_mut, not Rc::strong_count#203
Merged
Conversation
…trong_count node_ptr_mut rejected mutation whenever Rc::strong_count exceeded NODE_RC_MAX_GUARD (default 2). But strong_count counts live Node clones, which is not an aliasing conflict: the owning Document caches one wrapper per node and callers hold their own handles, so any non-trivial document trips it (e.g. heavily-shared trees reach clone counts in the thousands), producing spurious 'Can not mutably reference a shared Node' errors -- and borrow_mut could still panic on the very conflict it was meant to catch. All clones to one node share a single RefCell, so RefCell::try_borrow_mut is the exact per-node exclusion check: it fails only on an active re-entrant borrow, never on benign clone count. NODE_RC_MAX_GUARD and set_node_rc_guard become deprecated no-ops (kept for API compatibility); node_ptr_mut never consults them. The mutability_guards test is rewritten to assert the corrected behavior (benign clones succeed; the setter has no effect). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Summary
Node::node_ptr_mutgated mutable access onRc::strong_count <= NODE_RC_MAX_GUARD(default 2). That threshold counts liveNodeclones, which is not an aliasing conflict — the owningDocumentcaches one wrapper per node (_wrap/ptr_as_optionreturn a clone of the cachedRc<RefCell<_Node>>) and callers hold their own handles, so any non-trivial document exceeds it. The result was spuriousCan not mutably reference a shared Nodeerrors on perfectly valid documents (deeply-shared trees reach clone counts in the thousands), andborrow_mutcould still panic on the genuine conflict the guard was meant to catch.All clones of a node share a single
RefCell, soRefCell::try_borrow_mutis the exact per-node exclusion check: it returnsErronly when the node is actively borrowed (a real re-entrant aliasing conflict), and never on benign clone count.Changes
node_ptr_mutnow usestry_borrow_mut(sameResultsignature, so no caller changes).NODE_RC_MAX_GUARD/set_node_rc_guardare deprecated no-ops, retained for API compatibility; the crate never consults them (set_node_rc_guardhas an empty body).tests/mutability_guards.rsrewritten to assert the corrected behavior: benign clones mutate fine, and the deprecated setter has no effect (locks in the no-op).node_ptr_mut: the shared-RefCellinvariant holds for linked nodes; unlinked/imported nodes are evicted from the cache (set_unlinked/import_node, intentional vs pointer reuse) and re-wrap to an independentRefCell— unchanged from before. This guards the Rust wrapper, not the C node.Testing
cargo testgreen;cargo clippyclean for the changed code. Verified downstream against a large arXiv→XML corpus (latexml-oxide): the spurious "shared Node" errors disappear with no regressions (full downstream suite green), and the formerset_node_rc_guard(8192)workaround is no longer needed.🤖 Generated with Claude Code