EgorBaranovEnjoysTyping commented on PR #13262:
URL: https://github.com/apache/ignite/pull/13262#issuecomment-4830824180
Fix Summary
1. NPE in handleRecursion
Root cause: handleRecursion(SBLimitedLength buf, Object obj, Class cls,
IdentityHashMap<Object, EntryReference> svdObjs) accesses
buf.impl().substring(pos, pos + savedName.length()) where pos comes from
EntryReference.pos. When an object is encountered for the first time,
svdObjs.get(obj) returns null, but the method returns early. However, when the
buffer is shared across nested toString() calls (via
ThreadLocal<SBLimitedLength>), the pos stored in EntryReference can
point to an invalid offset after the buffer is reset or reused, causing
NullPointerException when buf.impl() returns null.
Fix: Replaced the mutable buffer-based approach with a tree-of-nodes
architecture (GridToStringNode). Recursion is tracked via NodeRecursionMonitor
(ThreadLocal registry), and each node appends itself
independently — no shared mutable buffer, no position tracking.
---
2. Inconsistent identity hash assignment
Root cause: In the original code, the identity hash of an object was
only inserted into the buffer at a previously recorded position (buf.i(pos +
name.length(), hash)). When toString() was called directly
on an object (top-level), ref.hashNeeded was set to true, but the hash
was never appended because there was no prior occurrence at a known position.
However, when the same object appeared nested inside
another object's toString(), the hash was inserted for both the
recursive termination node and the original occurrence. This resulted in
inconsistent output: the top-level object had no hash, while nested
objects did.
Fix: The new GridToStringRecursionTerminationNode immediately marks the
parent NodeRecursionMonitor with setHashRequired(), and the parent node appends
its hash during appendNode() via
appendHashIfRequired(sb). This ensures consistent hash output regardless
of call depth.
---
3. StringIndexOutOfBoundsException in handleRecursion
Root cause: handleRecursion calculates charsAtPos =
buf.impl().substring(pos, pos + savedName.length()) where savedName =
cls.getSimpleName() + identity(obj). If pos + savedName.length() exceeds the
current buffer length, substring() throws
StringIndexOutOfBoundsException. This happens when the buffer has grown past
pos but not enough to contain the full savedName string at that offset, or when
the
buffer was truncated due to the head limit.
Fix: Eliminated position-based buffer manipulation entirely. The node
tree approach builds the string representation by traversing nodes, so no
substring operations at arbitrary positions are needed.
---
4. Incorrect string truncation at the head-tail boundary in SBLengthLimit
Root cause: The original onWrite method used Math.min(sb.length(),
HEAD_LEN + 1) to determine the head length when creating the tail. The
overflowed() check used sb.impl().length() > HEAD_LEN (strict
greater-than), which caused an off-by-one error: the head could exceed
HEAD_LEN by one character before the tail was created. Additionally,
sb.setLength(newSbLen) was called, but the parent class's
setLength behavior didn't account for the tail split correctly,
resulting in characters being lost or duplicated at the boundary.
Fix: Changed overflowed() to use >= (sb.length() >=
getHeadLengthLimit()), replaced HEAD_LEN + 1 with getHeadLengthLimit(), and
used sb.impl().setLength() directly to avoid interference from overridden
methods. Added length() override in SBLimitedLength that correctly
accounts for tail.getSkipped() + tail.length().
---
5. Missing insert() methods in SBLimitedLength and CircularStringBuilder
Root cause: SBLimitedLength didn't override the insert() family of
methods from GridStringBuilder. When handleRecursion called buf.i(pos +
name.length(), hash) to insert the identity hash, the parent
class's insert() operated on the underlying buffer without respecting
the head-tail split, corrupting the output or causing
StringIndexOutOfBoundsException.
Fix: Implemented i(int offset, String str) in SBLimitedLength that
correctly redirects inserts to the tail when the offset is beyond the head
limit. Implemented insert(int offset, String valToInsert) in
CircularStringBuilder with proper circular buffer handling. Methods that
delete/modify the head (d(), r(), setLength()) explicitly throw
UnsupportedOperationException.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
