anton-vinogradov commented on PR #13262:
URL: https://github.com/apache/ignite/pull/13262#issuecomment-4811808585
Thanks for the round of fixes — re-reviewed at `7c75b1a`.
**The main blocker is genuinely resolved.** `markNode` now returns the real
`node.toString()` (wrapped in `new String(...)` for identity) instead of the
literal `GridToStringNode` placeholder, so the dangerous `"Foo{" +
S.toString(...) + "}"` concatenation case now degrades to the correct content
rather than corrupting the log. I verified parity with master:
```
wrapped : Parent [child=WRAP[Child [v=42]], name=p]
unwrapped: Parent2 [child=PlainChild [v=7]]
```
Cycle handling and the original NPE repro (self-ref sweep across the
`HEAD_LEN` boundary, pad 7975–7985) also pass with no NPE. Dropping
`recoverObject` and the typo/placeholder fixes look good. 👍
A few things still open:
1. **`CATCHED_NODES` (`GridToStringNode:38`) is still `static
ConcurrentHashMap<Thread, …>`.** Your "`clear()` is always in `finally`" answer
addresses the *leak*, but not the *design*: this map is only ever touched by
its owning thread, and `LAST_CONSTRUCTED_…` / `OBJECT_REGISTRY` in the same
class are already `ThreadLocal`. A `ThreadLocal` here is leak-free by
construction and drops the per-call `Thread`-keyed lookup — please make it
consistent.
2. **`markNode` eagerly materializes the whole child subtree into a String**
(`new String(node.toString())`) just to use it as a map key. That's exactly the
eagerness that weakens the point of `SBLimitedLength` — bounding the *cost* of
`toString` on large graphs, not just the output length. The streaming baseline
stopped doing work once the head filled; this renders every nested `S.toString`
in full first. I'd still like a JMH before/after on a large/deep graph (or at
least an explicit acknowledgement of the trade-off in the ticket).
3. **`SBLimitedLength.i(int,String)` else-branch still dereferences `tail`
with no guard** (`:277`). The "offset ≥ head ⇒ tail already exists" invariant
is plausible, but it's implicit. Cheapest fix is to document it with `assert
tail != null` so a future change can't silently reintroduce the NPE class this
PR is fixing.
On the bigger picture: I understand you'd rather frame this as
"`handleRecursion` is incorrect" than "fix the NPE", and the rewrite is
meaningfully safer now. But the core concern stands — this is a ~1700-line
architectural rewrite of the `toString` engine (two parallel nested-call
mechanisms, `markNode` vs `appendInnerBuffer`) riding on an NPE ticket, with an
unmeasured perf change on exactly the large-graph path the limiter exists for.
I still think the cleanest path is the localized `SBLimitedLength` insert fix +
regression test for this ticket, and the node-tree as a separate change with a
JMH story — but that's a maintainer call, so I'll defer to the others on scope.
--
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]
