On Thu, 17 Nov 2022 06:05:40 GMT, Jaikiran Pai <j...@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
