On Tue, 15 Nov 2022 02:10:01 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. LGTM ------------- Marked as reviewed by sundar (Reviewer). PR: https://git.openjdk.org/jdk/pull/11154
