RFR: 8292317: Missing null check for Iterator.forEachRemaining implementations [v2]

[Marcono1234]

On Thu, 17 Nov 2022 06:05:40 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which proposes to fix https://bugs.openjdk.org/browse/JDK-8292317?
>> 
>> The `java.util.Iterator` has a `forEachRemaining(Consumer<? super E> action)` method. As per its contract, the implementations are expected to throw a `NullPointerException` if the passed `action` is `null`.
>> 
>> `java.util.Collections` has a couple of places where it wraps the passed `action` into another and passes that wrapped `action` to the underlying `java.util.Iterator`'s default `forEachRemaining` implementation which is:
>> 
>> 
>> Objects.requireNonNull(action);
>>   while (hasNext())
>>       action.accept(next());
>> 
>> Since the passed `action` is now a non-null wrapper, the implementation goes ahead and advances the iterator to the next entry and invokes on the non-null `action` to carry out its action. That non-null wrapper action then calls the `null` user passed action and runs into an expected `NullPointerException`. However, at this point the iterator has already advanced and that is what the bug is.
>> 
>> The commit in this PR introduces a trivial null check on the `action` very early in the call even before wrapping such an `action`. This prevents any further logic to execute if `action` is null.
>> 
>> New test methods have been added to the existing test class `test/jdk/java/util/Collections/DelegatingIteratorForEachRemaining.java`. This test class was introduced in https://bugs.openjdk.org/browse/JDK-8205184 where this delegation logic was added for the `forEachRemaining` methods. These new test methods reproduce the failure and verify the fix.
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review suggestion - simplify test

src/java.base/share/classes/java/util/Collections.java line 1718:

> 1716:                     public void forEachRemaining(Consumer<? super Map.Entry<K, V>> action) {
> 1717:                         Objects.requireNonNull(action);
> 1718:                         i.forEachRemaining(entryConsumer(action));

As pointed out in the bug report description, it might be better to add the `null` check to `entryConsumer`. That would avoid code duplication for the `null` checks for all the current callers of `entryConsumer`.

(This comment only applies here; for `CheckedEntrySet` below this does not work because it does not call `entryConsumer`.)

-------------

PR: https://git.openjdk.org/jdk/pull/11154