diff --git a/commandsv3/src/main/java/org/wpilib/command3/Scheduler.java b/commandsv3/src/main/java/org/wpilib/command3/Scheduler.java index 232c8c1a8e8..49cca4bca40 100644 --- a/commandsv3/src/main/java/org/wpilib/command3/Scheduler.java +++ b/commandsv3/src/main/java/org/wpilib/command3/Scheduler.java @@ -114,7 +114,8 @@ public final class Scheduler implements ProtobufSerializable { */ private final Collection m_activeBindings = new ArrayList<>(); - private final Collection m_boundTriggers = new ArrayList<>(); + /** Triggers with active command bindings that must not be allowed to be garbage collected. */ + private final Set m_boundTriggers = new HashSet<>(); /** The set of commands scheduled since the start of the previous run. */ private final SequencedSet m_queuedToRun = new LinkedHashSet<>(); @@ -834,10 +835,13 @@ private void cancelStaleBindings() { } private void unbindStaleTriggers() { + // Remove strong references to any triggers that have gone stale. This allows triggers to be + // garbage collected if they're not referenced outside the scope that created them. We don't + // clear any command bindings or unbind it from the event loop; otherwise, cached triggers + // would stop being updated until new command bindings are added. for (var iterator = m_boundTriggers.iterator(); iterator.hasNext(); ) { var trigger = iterator.next(); if (!trigger.isScopeActive()) { - trigger.unbind(); iterator.remove(); } } @@ -847,11 +851,20 @@ private void unbindStaleTriggers() { * Adds a bound trigger to this scheduler. The trigger will be unbound from the event loop when * its creation scope becomes inactive and may be eligible for garbage collection. */ - // package-private for Trigger to call when constructed + // package-private for Trigger void addBoundTrigger(Trigger trigger) { m_boundTriggers.add(trigger); } + /** + * Removes strong retention for a trigger, allowing it to potentially be garbage collected if no + * other references to it remain. + */ + // package-private for Trigger + void removeBoundTrigger(Trigger trigger) { + m_boundTriggers.remove(trigger); + } + private void promoteScheduledCommands() { // Clear any commands that conflict with the scheduled set for (var queuedState : m_queuedToRun) { diff --git a/commandsv3/src/main/java/org/wpilib/command3/Trigger.java b/commandsv3/src/main/java/org/wpilib/command3/Trigger.java index 1c6da4f9505..6efd009d0ff 100644 --- a/commandsv3/src/main/java/org/wpilib/command3/Trigger.java +++ b/commandsv3/src/main/java/org/wpilib/command3/Trigger.java @@ -48,6 +48,7 @@ public class Trigger implements BooleanSupplier { private final BooleanSupplier m_condition; private final EventLoop m_loop; private final Scheduler m_scheduler; + private final BindingScope m_lifetimeScope; /** The value of the signal before the most recent call to {@link #poll()}. May be null. */ private Signal m_previousSignal; @@ -57,8 +58,6 @@ public class Trigger implements BooleanSupplier { private final Map> m_bindings = new EnumMap<>(BindingType.class); private final Runnable m_eventLoopCallback = this::poll; - private boolean m_bound = true; - private final BindingScope m_creationScope; /** * Represents the state of a signal: high or low. Used instead of a boolean for nullity on the @@ -109,10 +108,10 @@ public Trigger(Scheduler scheduler, EventLoop loop, BooleanSupplier condition) { m_scheduler = requireNonNullParam(scheduler, "scheduler", "Trigger"); m_loop = requireNonNullParam(loop, "loop", "Trigger"); m_condition = requireNonNullParam(condition, "condition", "Trigger"); - m_creationScope = BindingScope.createNarrowestScope(m_scheduler); + m_lifetimeScope = BindingScope.createNarrowestScope(m_scheduler); m_scheduler.addBoundTrigger(this); - m_loop.bind(m_eventLoopCallback); + m_loop.bindWeak(m_eventLoopCallback); } /** @@ -512,37 +511,9 @@ Signal getPreviousSignal() { return m_previousSignal; } - /** Checks if the creation scope is currently active. */ - // package-private for the scheduler to access + // package-private for Scheduler boolean isScopeActive() { - return m_creationScope.active(); - } - - /** - * Unbinds this trigger from the event loop and clears all command bindings; any bound commands - * that are currently running will be canceled. The trigger may be garbage collected if no other - * references exist in user code. Binding a command to a trigger via {@link #onTrue(Command)} or - * similar will re-bind the trigger to the event loop. - * - *

Note: because triggers are only updated when they're bound to an event loop, calling {@code - * #unbind()} will result in {@link #getAsBoolean()} continuing to return the same value until the - * trigger is re-bound. - * - *

This method is automatically called by the associated {@link Scheduler} when the trigger's - * creation scope becomes inactive: a trigger created inside a command will be unbound when that - * command completes, and may be eligible for garbage collection; and a trigger created while an - * opmode is running will be unbound when that opmode ends (and also may be eligible for garbage - * collection). - */ - public void unbind() { - // Ensure all bound commands are canceled - m_bindings.forEach( - (_, bindings) -> { - bindings.forEach(binding -> m_scheduler.cancel(binding.command())); - }); - m_bindings.clear(); - m_loop.unbind(m_eventLoopCallback); // note: ConcurrentModificationException if called in poll() - m_bound = false; + return m_lifetimeScope.active(); } // package-private for testing @@ -552,13 +523,6 @@ void addBinding(BindingScope scope, BindingType bindingType, Command command) { m_bindings .computeIfAbsent(bindingType, _k -> new ArrayList<>()) .add(new Binding(scope, bindingType, command, new Throwable().getStackTrace())); - - if (!m_bound) { - // Ensure we're bound to the event loop. - // Otherwise, the command binding will never fire. - m_loop.bind(m_eventLoopCallback); - m_bound = true; - } } private void addBinding(BindingType bindingType, Command command) { diff --git a/commandsv3/src/test/java/org/wpilib/command3/CommandTestBase.java b/commandsv3/src/test/java/org/wpilib/command3/CommandTestBase.java index c5e4012c9b0..79b9a46c839 100644 --- a/commandsv3/src/test/java/org/wpilib/command3/CommandTestBase.java +++ b/commandsv3/src/test/java/org/wpilib/command3/CommandTestBase.java @@ -11,6 +11,8 @@ import java.util.function.Predicate; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.wpilib.math.util.MathShared; +import org.wpilib.math.util.MathSharedStore; import org.wpilib.system.RobotController; class CommandTestBase { @@ -43,6 +45,20 @@ String getOpModeName() { }); } + @BeforeEach + void initTime() { + MathSharedStore.setMathShared( + new MathShared() { + @Override + public void reportError(String error, StackTraceElement[] stackTrace) {} + + @Override + public double getTimestamp() { + return RobotController.getTime() / 1e6; + } + }); + } + @AfterEach void resetOpmodeFetcher() { m_opModeId = 0; diff --git a/commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java b/commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java index 6d64aee3ef9..19034bc2dcb 100644 --- a/commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java +++ b/commandsv3/src/test/java/org/wpilib/command3/TriggerTest.java @@ -10,9 +10,11 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.wpilib.units.Units.Seconds; +import java.lang.ref.WeakReference; import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.BooleanSupplier; import org.junit.jupiter.api.Test; import org.wpilib.system.RobotController; @@ -607,15 +609,17 @@ void complexComposition() { } @Test - void triggerUnbindsWhenCommandScopeInactive() { + void triggerDoesNotUnbindWhenCommandScopeInactive() { var triggerSignal = new AtomicBoolean(false); var commandRan = new AtomicBoolean(false); + var triggerRef = new AtomicReference(); var innerCommand = Command.noRequirements(_ -> commandRan.set(true)).named("Inner"); var outerCommand = Command.noRequirements( co -> { var trigger = new Trigger(m_scheduler, triggerSignal::get); + triggerRef.set(trigger); trigger.onTrue(innerCommand); co.park(); }) @@ -634,7 +638,59 @@ void triggerUnbindsWhenCommandScopeInactive() { m_scheduler.run(); assertFalse(m_scheduler.isRunning(outerCommand)); - // The trigger should have unbound itself during the last run() call. + // Trigger should still update even though command bindings were removed + assertTrue(triggerRef.get().getAsBoolean()); + triggerSignal.set(false); + assertTrue(triggerRef.get().getAsBoolean()); + + m_scheduler.run(); + assertFalse(triggerRef.get().getAsBoolean()); + } + + @Test + void oneLineWhileTrueBindingRetainsTrigger() { + var signal = new AtomicBoolean(false); + var command = Command.noRequirements(Coroutine::park).named("Command"); + + var triggerRef = new WeakReference<>(new Trigger(m_scheduler, signal::get).whileTrue(command)); + + assertFalse( + waitForCollection(triggerRef), + "Trigger with active command bindings should be strongly retained"); + + signal.set(true); + m_scheduler.run(); + assertTrue( + m_scheduler.isRunning(command), + "Retained trigger should continue scheduling command bindings"); + } + + @Test + void unscopedTriggerCanBeGarbageCollected() { + // Makes the trigger scoped to an opmode + m_opModeId = 1; + m_opModeName = "opmode"; + + var signal = new AtomicBoolean(false); + + var command = Command.noRequirements(Coroutine::park).named("Command"); + var triggerRef = new WeakReference<>(new Trigger(m_scheduler, signal::get).onTrue(command)); + + m_scheduler.run(); + assertFalse( + waitForCollection(triggerRef), + "Trigger should not be garbage collected while still in scope"); + + // Exit the opmode scope + m_opModeId = 0; + m_opModeName = ""; + + signal.set(true); + m_scheduler.run(); // internally removes a strong reference to the trigger + assertEquals(List.of(), m_events, "The trigger should not have fired"); + assertTrue( + waitForCollection(triggerRef), + "Trigger should be garbage collected after going out of scope"); } @Test @@ -839,4 +895,28 @@ private BooleanSupplier flickering(AtomicBoolean signal) { return val; }; } + + @SuppressWarnings("PMD.DoNotCallGarbageCollectionExplicitly") + private static boolean waitForCollection(WeakReference reference) { + for (int i = 0; i < 200; i++) { + if (reference.get() == null) { + return true; + } + + System.gc(); + byte[] pressure = new byte[1024 * 1024]; + pressure[0] = 1; + + try { + Thread.sleep(5); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + break; + } + + assertEquals(1, pressure[0]); + } + + return reference.get() == null; + } } diff --git a/styleguide/spotbugs-exclude.xml b/styleguide/spotbugs-exclude.xml index 93cf809432d..e7e5c75b921 100644 --- a/styleguide/spotbugs-exclude.xml +++ b/styleguide/spotbugs-exclude.xml @@ -37,6 +37,11 @@ + + + + + diff --git a/wpilibj/src/main/java/org/wpilib/event/EventLoop.java b/wpilibj/src/main/java/org/wpilib/event/EventLoop.java index a92ebb44881..da09096fcda 100644 --- a/wpilibj/src/main/java/org/wpilib/event/EventLoop.java +++ b/wpilibj/src/main/java/org/wpilib/event/EventLoop.java @@ -4,22 +4,27 @@ package org.wpilib.event; -import java.util.Collection; import java.util.ConcurrentModificationException; import java.util.LinkedHashSet; +import org.wpilib.util.container.WeakLinkedHashSet; /** * A declarative way to bind a set of actions to a loop and execute them when the loop is polled. */ public final class EventLoop { - private final Collection m_bindings = new LinkedHashSet<>(); + // Keep all bindings in one insertion-ordered collection for polling. + private final WeakLinkedHashSet m_bindings = new WeakLinkedHashSet<>(); + // Keep strong references for strongly bound actions to prevent garbage collection. + private final LinkedHashSet m_strongBindings = new LinkedHashSet<>(); private boolean m_running; /** Default constructor. */ public EventLoop() {} /** - * Bind a new action to run when the loop is polled. + * Bind a new action to run when the loop is polled. The event loop keeps a reference to the + * action, which will prevent it from being garbage collected. If memory leaks are a concern, + * consider {@link #bindWeak(Runnable)}. * * @param action the action to run. */ @@ -28,6 +33,30 @@ public void bind(Runnable action) { throw new ConcurrentModificationException("Cannot bind EventLoop while it is running"); } m_bindings.add(action); + m_strongBindings.add(action); + } + + /** + * Weakly binds a new action to run when the loop is polled. Unlike {@link #bind(Runnable)}, a + * weakly bound action will not be prevented from being garbage collected. + * + * @param action the action to run + */ + public void bindWeak(Runnable action) { + if (m_running) { + throw new ConcurrentModificationException("Cannot bind EventLoop while it is running"); + } + m_bindings.add(action); + } + + /** + * Checks if an action is bound to the event loop. + * + * @param action the action to check + * @return true if the action is bound, false if not + */ + public boolean isBound(Runnable action) { + return m_bindings.contains(action); } /** @@ -41,6 +70,7 @@ public void unbind(Runnable action) { throw new ConcurrentModificationException("Cannot unbind EventLoop while it is running"); } m_bindings.remove(action); + m_strongBindings.remove(action); } /** Poll all bindings. */ @@ -60,5 +90,6 @@ public void clear() { throw new ConcurrentModificationException("Cannot clear EventLoop while it is running"); } m_bindings.clear(); + m_strongBindings.clear(); } } diff --git a/wpilibj/src/test/java/org/wpilib/event/EventLoopTest.java b/wpilibj/src/test/java/org/wpilib/event/EventLoopTest.java index 6dd6618a344..d365c62bb6e 100644 --- a/wpilibj/src/test/java/org/wpilib/event/EventLoopTest.java +++ b/wpilibj/src/test/java/org/wpilib/event/EventLoopTest.java @@ -61,6 +61,21 @@ void testClear() { assertEquals(1, counter.get()); } + @Test + void testMixedWeakAndStrongBindingsRunInInsertionOrder() { + var order = new StringBuilder(); + var loop = new EventLoop(); + + loop.bindWeak(() -> order.append('a')); + loop.bind(() -> order.append('b')); + loop.bindWeak(() -> order.append('c')); + loop.bind(() -> order.append('d')); + + loop.poll(); + + assertEquals("abcd", order.toString()); + } + @Test void testConcurrentModification() { var loop = new EventLoop(); diff --git a/wpiutil/src/main/java/org/wpilib/util/container/WeakLinkedHashSet.java b/wpiutil/src/main/java/org/wpilib/util/container/WeakLinkedHashSet.java new file mode 100644 index 00000000000..f0baad7c26c --- /dev/null +++ b/wpiutil/src/main/java/org/wpilib/util/container/WeakLinkedHashSet.java @@ -0,0 +1,268 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.util.container; + +import static org.wpilib.util.ErrorMessages.requireNonNullParam; + +import java.lang.ref.WeakReference; +import java.util.AbstractSet; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.NoSuchElementException; +import java.util.SequencedSet; +import java.util.Spliterator; +import java.util.Spliterators; + +/** + * A variant of {@link java.util.LinkedHashSet} with weak references to entries. This container will + * not prevent its entries from being garbage collected, unlike a typical {@code Set}. Null elements + * are not permitted; calling {@link #add(Object) add(null)} will always throw a {@code + * NullPointerException}. + * + * @param The type of elements in the set. + * @see java.util.LinkedHashSet + * @see WeakReference + */ +public final class WeakLinkedHashSet extends AbstractSet implements SequencedSet { + private final LinkedHashMap, Object> map = new LinkedHashMap<>(); + private static final Object PRESENT = new Object(); + + /** + * Constructs a new, empty linked hash set with the default initial capacity (16) and load factor + * (0.75). + */ + public WeakLinkedHashSet() { + super(); + } + + /** + * Constructs a new linked hash set with the same elements as the specified collection. The linked + * hash set is created with an initial capacity sufficient to hold the elements in the specified + * collection and the default load factor (0.75). + * + * @param collection the collection whose elements are to be placed into this set + * @throws NullPointerException if the specified collection is null + */ + public WeakLinkedHashSet(Collection collection) { + requireNonNullParam(collection, "collection", "WeakLinkedHashSet"); + addAll(collection); + } + + @Override + public boolean add(E e) { + requireNonNullParam(e, "e", "WeakLinkedHashSet.add"); + + purgeCollected(); + return map.putLast(new WeakKey<>(e), PRESENT) == null; + } + + @Override + public void addFirst(E e) { + requireNonNullParam(e, "e", "WeakLinkedHashSet.addFirst"); + + purgeCollected(); + map.putFirst(new WeakKey<>(e), PRESENT); + } + + @Override + public void addLast(E e) { + requireNonNullParam(e, "e", "WeakLinkedHashSet.addLast"); + + purgeCollected(); + map.putLast(new WeakKey<>(e), PRESENT); + } + + @Override + public boolean remove(Object o) { + requireNonNullParam(o, "o", "WeakLinkedHashSet.remove"); + + purgeCollected(); + return map.remove(new WeakKey<>(o)) != null; + } + + @Override + public boolean contains(Object o) { + requireNonNullParam(o, "o", "WeakLinkedHashSet.contains"); + + purgeCollected(); + return map.containsKey(new WeakKey<>(o)); + } + + @Override + public int size() { + purgeCollected(); + return map.size(); + } + + // Removes entries where the element has been garbage collected + private void purgeCollected() { + map.keySet().removeIf(key -> key.get() == null); + } + + @Override + public Iterator iterator() { + purgeCollected(); + return new WeakKeyIterator(map.keySet().iterator()); + } + + @Override + public SequencedSet reversed() { + purgeCollected(); + + return new ReversedView<>(this, map.sequencedKeySet().reversed()); + } + + @Override + public Spliterator spliterator() { + return Spliterators.spliteratorUnknownSize( + iterator(), Spliterator.DISTINCT | Spliterator.ORDERED | Spliterator.NONNULL); + } + + private static final class WeakKey extends WeakReference { + private final int m_hashCode; + + WeakKey(T referent) { + super(referent); + m_hashCode = referent.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof WeakKey other)) { + return false; + } + + T ref = get(); + return ref != null && ref.equals(other.get()); + } + + @Override + public int hashCode() { + return m_hashCode; + } + } + + private class WeakKeyIterator implements Iterator { + private final ArrayList> keys = new ArrayList<>(); + private int nextIndex; + private E nextElement = null; + private E lastReturnedElement = null; + + WeakKeyIterator(Iterator> mapIterator) { + mapIterator.forEachRemaining(keys::add); + } + + @Override + public boolean hasNext() { + while (nextElement == null && nextIndex < keys.size()) { + WeakKey key = keys.get(nextIndex++); + nextElement = key.get(); + if (nextElement == null) { + map.remove(key); + } + } + return nextElement != null; + } + + @Override + public E next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } + E element = nextElement; + lastReturnedElement = element; + nextElement = null; + return element; + } + + @Override + public void remove() { + if (lastReturnedElement == null) { + throw new IllegalStateException(); + } + + map.remove(new WeakKey<>(lastReturnedElement)); + lastReturnedElement = null; + } + } + + private static class ReversedView extends AbstractSet implements SequencedSet { + private final WeakLinkedHashSet originalSet; + private final SequencedSet> reversedKeys; + + ReversedView(WeakLinkedHashSet originalSet, SequencedSet> reversedKeys) { + this.originalSet = originalSet; + this.reversedKeys = reversedKeys; + } + + @Override + public int size() { + return originalSet.size(); + } + + @Override + public Iterator iterator() { + return originalSet.new WeakKeyIterator(reversedKeys.iterator()); + } + + @Override + public SequencedSet reversed() { + return originalSet; // Reversing a reversed view returns the original set + } + + @Override + public boolean add(E e) { + return originalSet.add(e); + } + + @Override + public boolean remove(Object o) { + return originalSet.remove(o); + } + + @Override + public boolean contains(Object o) { + return originalSet.contains(o); + } + + @Override + public void addFirst(E e) { + originalSet.addLast(e); + } + + @Override + public void addLast(E e) { + originalSet.addFirst(e); + } + + @Override + public E getFirst() { + return originalSet.getLast(); + } + + @Override + public E getLast() { + return originalSet.getFirst(); + } + + @Override + public E removeFirst() { + return originalSet.removeLast(); + } + + @Override + public E removeLast() { + return originalSet.removeFirst(); + } + + @Override + public Spliterator spliterator() { + return Spliterators.spliteratorUnknownSize( + iterator(), Spliterator.DISTINCT | Spliterator.ORDERED | Spliterator.NONNULL); + } + } +} diff --git a/wpiutil/src/test/java/org/wpilib/util/container/WeakLinkedHashSetTest.java b/wpiutil/src/test/java/org/wpilib/util/container/WeakLinkedHashSetTest.java new file mode 100644 index 00000000000..250698c7b0d --- /dev/null +++ b/wpiutil/src/test/java/org/wpilib/util/container/WeakLinkedHashSetTest.java @@ -0,0 +1,238 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package org.wpilib.util.container; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.lang.ref.WeakReference; +import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.SequencedSet; +import java.util.Set; +import org.junit.jupiter.api.Test; + +@SuppressWarnings({"PMD.UnusedAssignment", "PMD.DoNotCallGarbageCollectionExplicitly"}) +class WeakLinkedHashSetTest { + private record TestValue(int id) {} + + @Test + void behavesLikeLinkedHashSetForBasicSetOperations() { + final Set expected = new LinkedHashSet<>(); + final Set actual = new WeakLinkedHashSet<>(); + + final var a = new TestValue(1); + final var b = new TestValue(2); + final var c = new TestValue(3); + final var d = new TestValue(4); + + assertEquals(expected.add(a), actual.add(a)); + assertEquals(expected.add(b), actual.add(b)); + assertEquals(expected.add(c), actual.add(c)); + assertEquals(expected.add(b), actual.add(b)); + + assertEquals(expected.contains(a), actual.contains(a)); + assertEquals(expected.contains(d), actual.contains(d)); + + assertEquals(expected.remove(b), actual.remove(b)); + assertEquals(expected.remove(b), actual.remove(b)); + + assertEquals(expected.add(d), actual.add(d)); + + assertEquals(expected.size(), actual.size()); + assertEquals(List.copyOf(expected), List.copyOf(actual)); + } + + @Test + void reversedViewIterationMatchesLinkedHashSet() { + final SequencedSet expected = new LinkedHashSet<>(); + final SequencedSet actual = new WeakLinkedHashSet<>(); + + final var a = new TestValue(1); + final var b = new TestValue(2); + final var c = new TestValue(3); + + expected.add(a); + expected.add(b); + expected.add(c); + + actual.add(a); + actual.add(b); + actual.add(c); + + assertEquals(List.copyOf(expected.reversed()), List.copyOf(actual.reversed())); + } + + @Test + void positionalInsertionsMatchLinkedHashSet() { + final SequencedSet expected = new LinkedHashSet<>(); + final SequencedSet actual = new WeakLinkedHashSet<>(); + + final var a = new TestValue(1); + final var b = new TestValue(2); + final var c = new TestValue(3); + final var d = new TestValue(4); + + expected.add(a); + expected.add(b); + actual.add(a); + actual.add(b); + + expected.addFirst(c); + expected.addLast(d); + expected.addFirst(a); + + actual.addFirst(c); + actual.addLast(d); + actual.addFirst(a); + + assertEquals(List.copyOf(expected), List.copyOf(actual)); + assertEquals(List.copyOf(expected.reversed()), List.copyOf(actual.reversed())); + } + + @Test + void reversedViewAddMatchesLinkedHashSetAndDuplicateReturnValue() { + final SequencedSet expected = new LinkedHashSet<>(); + final SequencedSet actual = new WeakLinkedHashSet<>(); + + final var a = new TestValue(1); + final var b = new TestValue(2); + final var c = new TestValue(3); + + expected.add(a); + expected.add(b); + actual.add(a); + actual.add(b); + + final SequencedSet expectedReversed = expected.reversed(); + final SequencedSet actualReversed = actual.reversed(); + + assertEquals(expectedReversed.add(c), actualReversed.add(c)); + assertEquals(expectedReversed.add(c), actualReversed.add(c)); + + assertEquals(List.copyOf(expected), List.copyOf(actual)); + assertEquals(List.copyOf(expectedReversed), List.copyOf(actualReversed)); + } + + @Test + void nullArgumentsThrowInMainView() { + final var set = new WeakLinkedHashSet(); + + assertThrows(NullPointerException.class, () -> set.add(null)); + assertThrows(NullPointerException.class, () -> set.addFirst(null)); + assertThrows(NullPointerException.class, () -> set.addLast(null)); + assertThrows(NullPointerException.class, () -> set.contains(null)); + assertThrows(NullPointerException.class, () -> set.remove(null)); + } + + @Test + void nullArgumentsThrowInReversedView() { + final SequencedSet set = new WeakLinkedHashSet<>(); + final SequencedSet reversed = set.reversed(); + + assertThrows(NullPointerException.class, () -> reversed.add(null)); + assertThrows(NullPointerException.class, () -> reversed.addFirst(null)); + assertThrows(NullPointerException.class, () -> reversed.addLast(null)); + assertThrows(NullPointerException.class, () -> reversed.contains(null)); + assertThrows(NullPointerException.class, () -> reversed.remove(null)); + } + + @Test + void iteratorRemoveAfterHasNextRemovesPreviouslyReturnedElement() { + final var set = new WeakLinkedHashSet(); + final var a = new TestValue(1); + final var b = new TestValue(2); + final var c = new TestValue(3); + set.add(a); + set.add(b); + set.add(c); + + final Iterator iterator = set.iterator(); + assertEquals(a, iterator.next()); + assertTrue(iterator.hasNext()); + + iterator.remove(); + + assertEquals(List.of(b, c), List.copyOf(set)); + } + + @Test + void iteratorRemoveBeforeNextThrows() { + final var set = new WeakLinkedHashSet(); + set.add(new TestValue(1)); + + final Iterator iterator = set.iterator(); + assertTrue(iterator.hasNext()); + assertThrows(IllegalStateException.class, iterator::remove); + } + + @Test + void garbageCollectedEntriesArePurged() { + final var set = new WeakLinkedHashSet(); + + var value = new TestValue(10); + final var ref = new WeakReference<>(value); + + set.add(value); + value = null; // clear the only reference to this object + + assertTrue(waitForCollection(ref)); + assertNull(ref.get()); + + assertEquals(0, set.size()); + assertFalse(set.iterator().hasNext()); + } + + @Test + void collectedEntriesDoNotRemoveLiveEntries() { + final var set = new WeakLinkedHashSet(); + + final var kept = new TestValue(1); + var dropped = new TestValue(2); + final var droppedRef = new WeakReference<>(dropped); + + set.add(kept); + set.add(dropped); + dropped = null; // clear the only reference to this object + + assertTrue(waitForCollection(droppedRef)); + assertNull(droppedRef.get()); + + assertEquals(1, set.size()); + assertTrue(set.contains(new TestValue(1))); + assertFalse(set.contains(new TestValue(2))); + assertEquals(List.of(new TestValue(1)), List.copyOf(set)); + } + + private static boolean waitForCollection(WeakReference reference) { + // Allocate in a loop to increase garbage collection pressure and force the GC to clean up + // our WeakReferences + for (int i = 0; i < 200; i++) { + if (reference.get() == null) { + return true; + } + + System.gc(); + byte[] pressure = new byte[1024 * 1024]; + pressure[0] = 1; + + try { + Thread.sleep(5); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + break; + } + + // Ensure JIT doesn't decide to elide the allocation + assertEquals(1, pressure[0]); + } + + return reference.get() == null; + } +}