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

On 20/07/2026 15:42, Rémy Maucherat wrote:
> On Mon, Jul 20, 2026 at 12:01 PM Mark Thomas <[email protected]> wrote:
>>
>> On 15/07/2026 16:06, [email protected] 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 <[email protected]>
>>> 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