jsedding commented on code in PR #2457: URL: https://github.com/apache/jackrabbit-oak/pull/2457#discussion_r2287229883
########## oak-commons/src/main/java/org/apache/jackrabbit/oak/commons/function/Suppliers.java: ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.commons.function; + +import org.jetbrains.annotations.Nullable; + +import java.util.function.Supplier; + +/** + * Utility methods for {@link java.util.function.Supplier} handling. + */ +public class Suppliers { + + private Suppliers() { + } + + /** + * Transforms a {@link Supplier} based on a wrapper that evaluates + * the given {@code Supplier} at most one time. + * @param <T> return type + * @return Supplier based on the given Supplier + */ + public static <@Nullable T> Supplier<T> memoize(final Supplier<@Nullable T> computeOnce) { Review Comment: Nullability annotations don't go on type parameters. ```suggestion public static <T> Supplier<@Nullable T> memoize(final Supplier<@Nullable T> computeOnce) { ``` ########## oak-commons/src/test/java/org/apache/jackrabbit/oak/commons/function/SuppliersTest.java: ########## @@ -0,0 +1,111 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.commons.function; + +import org.junit.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.apache.jackrabbit.oak.commons.function.Suppliers.memoize; +import static org.junit.Assert.assertNull; + +public class SuppliersTest { + + @Test + public void computeOnce() { + AtomicInteger count = new AtomicInteger(0); + + Supplier<Integer> mem = Suppliers.memoize(count::incrementAndGet); + + assertEquals(0, count.get()); + int c = mem.get(); + assertEquals(1, c); + c = mem.get(); + assertEquals(1, c); + } + + @Test + public void nullSupplier() { + AtomicInteger count = new AtomicInteger(0); + + Supplier<Object> mem = Suppliers.memoize(new Supplier<Object>() { + @Override + public Integer get() { + count.incrementAndGet(); + return null; + } + }); + + assertEquals(0, count.get()); + Object o = mem.get(); + assertNull(o); + assertEquals(1, count.get()); + o = mem.get(); + assertEquals(1, count.get()); + + } + + @Test + public void concurrentSupplierAccess() throws InterruptedException { + List<Thread> threads = new ArrayList<>(); + int threadCount = 1000; + for (int k = 0; k < threadCount; k++) { + threads.add(new Thread(() -> { + synchronized (concurrencyTestMonitor) { + // the empty synchronized block is deliberate. + } + AtomicInteger result = memoizeTestSupplier.get(); + if (result == null || result.get() != 42) { + concurrencyTestFailed = true; + } + })); + } + Thread waitForAll = new Thread(() -> { + for (int k = 0; k < threadCount; k++) { + try { + threads.get(k).join(); + } catch (InterruptedException ignored) {} + } + }); + synchronized (concurrencyTestMonitor) { + for (int k = 0; k < threadCount; k++) { + threads.get(k).start(); + } + } + waitForAll.start(); + waitForAll.join(); Review Comment: This test seems overly complicated. I think it could be simplified a lot by using a `CountDownLatch` to let all threads wait before calling the memoizer. TBH, I have never seen this pattern with an empty synchronize block, and I don't understand what it does. The "waitForAll" functionality can happen in the "main" thread, no need to create a new thread and join it. Finally, it would be more elegant to move all the class fields (e.g. `concurrencyTestFailed`, `sourceSupplier`, etc) into the test method (this may require changing `concurrencyTestFailed` to `AtomicBoolean`). This makes the test method more self-contained and thus easier to read. It is also more maintainable, because it prevents test methods written in the future from re-using these fields, which might introduce flaky tests that depend on the ordering of test execution. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
