You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@ignite.apache.org by "EgorBaranovEnjoysTyping (via GitHub)" <gi...@apache.org> on 2026/06/22 06:10:14 UTC

[PR] IGNITE-28747 GridToStringBuilder#handleRecursion may cause NPE. Colle… [ignite]

EgorBaranovEnjoysTyping opened a new pull request, #13262:
URL: https://github.com/apache/ignite/pull/13262

   Collect toString as tree, avoid extra allocations and problems in recursion resolution
   Problems solved:
   Any recursive call in toString method would be handled
   Allocate less memory
   Memory wouldn't be kept for a whole thread life


-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] IGNITE-28747 GridToStringBuilder#handleRecursion may cause NPE. Colle… [ignite]

Posted by "anton-vinogradov (via GitHub)" <gi...@apache.org>.

anton-vinogradov commented on code in PR #13262:
URL: https://github.com/apache/ignite/pull/13262#discussion_r3483045395


##########
modules/core/src/test/java/org/apache/ignite/internal/util/tostring/GridToStringBuilderSelfTest.java:
##########
@@ -204,6 +205,106 @@ public void testToStringCheckObjectRecursionPrevention() throws Exception {
         fut4.get(3_000);
     }
 
+
+    /** */
+    @Test
+    public void testNPE() {
+        boolean success;
+        try {
+            SBLimitedLength sbLimitedLength = new SBLimitedLength(256);
+            sbLimitedLength.initLimit(new SBLengthLimit());
+            sbLimitedLength.a("a".repeat(7999));
+            sbLimitedLength.i(7000, "asd");
+            sbLimitedLength.a("a".repeat(10));
+            success = true;
+        } catch (NullPointerException ignored) {
+            success = false;
+        }
+        Assert.assertTrue(success);
+    }

Review Comment:
   JUnit 4 isn't the blocker here — Ignite ships its own `GridTestUtils.assertThrows(log, callable, cls, msg)` / `assertThrowsWithCause(...)`, used all over the test base (`CdcSelfTest`, `NodeWithFilterRestartTest`, …). So you can assert the throw directly instead of the `boolean` + `catch (NPE)` bypass, which keeps the full stack trace on failure and won't silently swallow a different exception. (As an aside, JUnit 4.13 also added `org.junit.Assert.assertThrows`.)



##########
modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringNode.java:
##########
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.util.tostring;
+
+import java.util.IdentityHashMap;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.internal.util.GridStringBuilder;
+
+import static org.apache.ignite.internal.util.tostring.NodeRecursionMonitor.OBJECT_REGISTRY;
+
+/**
+ * The abstract base class for all nodes in the string representation tree.
+ * Defines the common interface for appending a node's value to a string builder
+ * and provides static factory and utility methods for all subclasses.
+ */
+public abstract class GridToStringNode {
+    /**
+     * A thread-local cache for nodes, used to handle references of
+     * inner toString() calls by mapping temporary markers to actual nodes.
+     */
+    static final ConcurrentHashMap<Thread, IdentityHashMap<String, GridToStringNode>> CATCHED_NODES
+            = new ConcurrentHashMap<>();
+
+    /** Last constructed node. */
+    static final ThreadLocal<GridToStringNode> LAST_CONSTRUCTED_GRID_TO_STRING_NODE = new ThreadLocal<>();
+
+    /** The name of the property this node represents. */
+    String propName;
+
+    /** Inner buffer. For inner calls. */
+    StringBuilder innerBuf = new StringBuilder(0);
+
+    /**
+     * Base constructor.
+     * @param propName The name of the property.
+     */
+    GridToStringNode(String propName) {
+        this.propName = propName;
+    }
+
+    /**
+     * Creates a root node for a string value.
+     * @param str The string value.
+     * @param addNodes Additional child nodes to include.
+     * @return A new root node.
+     */
+    static GridToStringNode getRootNode(String str, List<GridToStringNode> addNodes) {
+        return new GridToStringObjectNode(str, addNodes);
+    }
+
+    /**
+     * Creates a root node for an object.
+     * Checks for recursion and creates either a termination node or a full object node.
+     * @param obj The object to represent.
+     * @param cls The class of the object.
+     * @param addNodes Additional child nodes to include.
+     * @return A new root node.
+     */
+    static GridToStringNode getRootNode(Object obj, Class<?> cls, List<GridToStringNode> addNodes) {
+        return recursionTermination(obj)
+                .orElseGet(() -> new GridToStringObjectNode(null, obj, cls, addNodes));
+    }
+
+    /**
+     * Marks a node in the thread-local cache with a unique, empty string key.
+     * Used to handle references during object graph traversal.
+     * @param node The node to mark.
+     * @return The unique marker string.
+     */
+    static String markNode(GridToStringNode node) {
+        String result = new String(GridToStringNode.class.getSimpleName());

Review Comment:
   1) Agreed on `new String(...)` — you do need a fresh identity because `toString()` may hand back an interned/pooled instance. That part is fine.
   
   2) But `Optional.orElseThrow()` is not a Java `assert`: it throws `NoSuchElementException` unconditionally (it's not gated by `-ea`), and that throw escapes from inside `toString()` — exactly the "toString must never throw" failure mode the original blanket try/catch existed to prevent. If the intent is only to document the "init() ran" invariant, use `assert identities().isPresent()` (or `orElseThrow(() -> new AssertionError(...))`), so it's a no-op in production instead of being able to turn a logging call into a thrown exception.



##########
modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringBuilder.java:
##########
@@ -1994,87 +1497,13 @@ public static <T> String joinToString(
         return buf.toString();
     }
 
-    /**
-     * Checks that object is already saved.
-     * In positive case this method inserts hash to the saved object entry (if needed) and name@hash for current entry.
-     * Further toString operations are not needed for current object.
-     *
-     * @param buf String builder buffer.
-     * @param obj Object.
-     * @param cls Class.
-     * @param svdObjs Map with saved objects to handle recursion.
-     * @return {@code True} if object is already saved and name@hash was added to buffer.
-     * {@code False} if it wasn't saved previously and it should be saved.
-     */
-    private static boolean handleRecursion(
-        SBLimitedLength buf,
-        Object obj,
-        @NotNull Class cls,
-        IdentityHashMap<Object, EntryReference> svdObjs
-    ) {
-        EntryReference ref = svdObjs.get(obj);
-
-        if (ref == null)
-            return false;
-
-        int pos = ref.pos;
-
-        String name = cls.getSimpleName();
-        String hash = identity(obj);
-        String savedName = name + hash;
-        String charsAtPos = buf.impl().substring(pos, pos + savedName.length());
-
-        if (!buf.isOverflowed() && !savedName.equals(charsAtPos)) {
-            if (charsAtPos.startsWith(cls.getSimpleName())) {
-                buf.i(pos + name.length(), hash);
-
-                incValues(svdObjs, obj, hash.length());
-            }
-            else
-                ref.hashNeeded = true;
-        }
-
-        buf.a(savedName);
-
-        return true;
-    }
-
-    /**
-     * Increment positions of already presented objects afterward given object.
-     *
-     * @param svdObjs Map with objects already presented in the buffer.
-     * @param obj Object.
-     * @param hashLen Length of the object's hash.
-     */
-    private static void incValues(IdentityHashMap<Object, EntryReference> svdObjs, Object obj, int hashLen) {
-        int baseline = svdObjs.get(obj).pos;
-
-        for (IdentityHashMap.Entry<Object, EntryReference> entry : svdObjs.entrySet()) {
-            EntryReference ref = entry.getValue();
-
-            int pos = ref.pos;
-
-            if (pos > baseline)
-                ref.pos = pos + hashLen;
-        }
-    }
-
-    /**
-     *
-     */
-    private static class EntryReference {
-        /** Position. */
-        int pos;
-
-        /** First object entry needs hash to be written. */
-        boolean hashNeeded;
-
-        /**
-         * @param pos Position.
-         */
-        private EntryReference(int pos) {
-            this.pos = pos;
-            hashNeeded = false;
-        }
+    /** */
+    private static <T extends Throwable> String handleThrowable(T throwable) {
+        StringWriter strWriter = new StringWriter();
+        PrintWriter printWriter = new PrintWriter(strWriter);
+        throwable.printStackTrace(printWriter);
+        printWriter.flush();
+        printWriter.close();
+        return strWriter.toString();
     }

Review Comment:
   This is actually a behavioral change from master: there the catch did `throw new IgniteException(e)` ("No other option here") — it propagated the failure, it did not render the exception into the result. `handleThrowable` now returns the **full stack trace as the string value**, so a failing `toString()` dumps a multi-line trace into wherever the value was being logged (and can surface internal/sensitive detail — Copilot's point). I'd either keep master's rethrow, or return a short `cls + ": " + message` summary and let the trace go through the logger, rather than inlining the whole trace as the field value.



##########
modules/commons/src/main/java/org/apache/ignite/internal/util/tostring/GridToStringNode.java:
##########
@@ -0,0 +1,195 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.util.tostring;
+
+import java.util.IdentityHashMap;
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.ignite.internal.util.GridStringBuilder;
+
+import static org.apache.ignite.internal.util.tostring.NodeRecursionMonitor.OBJECT_REGISTRY;
+
+/**
+ * The abstract base class for all nodes in the string representation tree.
+ * Defines the common interface for appending a node's value to a string builder
+ * and provides static factory and utility methods for all subclasses.
+ */
+public abstract class GridToStringNode {
+    /**
+     * A thread-local cache for nodes, used to handle references of
+     * inner toString() calls by mapping temporary markers to actual nodes.
+     */
+    static final ConcurrentHashMap<Thread, IdentityHashMap<String, GridToStringNode>> CATCHED_NODES

Review Comment:
   The `finally`-clear handles the *leak* (given perfectly balanced init/clear), but the design point stands: this map is only ever touched by its owning thread, so `ConcurrentHashMap<Thread, …>` is pure overhead over a `ThreadLocal` — and `LAST_CONSTRUCTED_…` / `OBJECT_REGISTRY` right next to it are already `ThreadLocal`. A `ThreadLocal<IdentityHashMap<…>>` is leak-free by construction and removes the per-call thread lookup. Also the `CATCHED` → `CACHED` rename (Copilot's nit) doesn't look applied yet.



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org