Re: (tomcat) branch main updated: Code review fixes

[Mark Thomas]

On 20/07/2026 15:42, Rémy Maucherat wrote:
On Mon, Jul 20, 2026 at 12:01 PM Mark Thomas <ma...@apache.org> wrote:

On 15/07/2026 16:06, r...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.

rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
       new 60008aaaf7 Code review fixes
60008aaaf7 is described below

commit 60008aaaf73a5e864f382f4667d576ea2d5b2f2a
Author: remm <r...@apache.org>
AuthorDate: Wed Jul 15 17:06:29 2026 +0200

      Code review fixes

<snip/>

diff --git a/java/org/apache/catalina/util/ResourceSet.java 
b/java/org/apache/catalina/util/ResourceSet.java
index f7a82f7451..8fcc5cdc81 100644
--- a/java/org/apache/catalina/util/ResourceSet.java
+++ b/java/org/apache/catalina/util/ResourceSet.java

<snip/>

@@ -221,6 +222,23 @@ public final class ResourceSet<T> extends HashSet<T> {
       }


+    @Override
+    public java.util.Spliterator<T> spliterator() {
+        if (locked) {
+            java.util.Spliterator<T> base = super.spliterator();
+            return new java.util.Spliterator<T>() {
+                @Override public boolean tryAdvance(java.util.function.Consumer<? 
super T> action) { return base.tryAdvance(action); }
+                @Override public java.util.Spliterator<T> trySplit() { return 
null; }
+                @Override public long estimateSize() { return 
base.estimateSize(); }
+                @Override public int characteristics() { return 
base.characteristics() & ~java.util.Spliterator.SORTED; }
+                @Override public void 
forEachRemaining(java.util.function.Consumer<? super T> action) { 
base.forEachRemaining(action); }
+                @Override public Comparator<? super T> getComparator() { 
return base.getComparator(); }
+            };
+        }
+        return super.spliterator();
+    }

What was the intention here? The above code is a NO-OP. ResourceSet
extends HashSet which never returns the SORTED characteristic. Also, it
is not clear why this is conditional on the status of locked.

I did wonder if the intention was to add IMMTABLE when the Set was
locked but the logic for that is sufficiently different that I am really
not sure.

Code review had a defect like this:
"ResourceSet overrides iterator() to return a LockedIterator wh
locked (line 230-235), which throws on remove(). However,
spliterator() is not overridden. When the set is locked, calling
spliterator() returns the inherited HashSet.spliterator(), whose
iterator allows remove() operations, bypassing the lock."

I don't have experience using spliterator. iterator() returns a
LockedIterator when locked, and a regular iterator otherwise.

That looks like an hallucination. HashSet.spliterator() does not have an 
iterator. Nor does it have a remove() method.

I don't see a way to mutate the underlying Set via the object returned 
from spliterator().

I'm going to remove that code as unnecessary.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org