Copilot commented on code in PR #694:
URL:
https://github.com/apache/commons-collections/pull/694#discussion_r3525190376
##########
src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java:
##########
@@ -255,4 +258,185 @@ private void validateExpiration(final Map<String, String>
map, final long timeou
assertNull(map.get("a"));
}
+ @Test
+ void testCollectionsSynchronizedMapExpiration() throws
InterruptedException {
+ final Map<String, String> map = Collections.synchronizedMap(new
PassiveExpiringMap<>(50L));
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ // Cache the views in SynchronizedMap before they expire
+ final Collection<Map.Entry<String, String>> entrySet = map.entrySet();
+ final Collection<String> keySet = map.keySet();
+ final Collection<String> values = map.values();
+ Thread.sleep(100L);
+ // entrySet iterator triggers expiration
Review Comment:
Comment says the entrySet *iterator* triggers expiration, but the code is
asserting via isEmpty() on the cached view. This is misleading for a regression
test meant to document the trigger mechanism.
##########
src/changes/changes.xml:
##########
@@ -24,6 +24,7 @@
<body>
<release version="4.6.0" date="YYYY-MM-DD" description="This is a feature
and maintenance release. Java 8 or later is required.">
<!-- FIX -->
+ <action type="fix" dev="ggregory" due-to="Gary Gregory"
issue="COLLECTIONS-776">Wrapping PassiveExpiringMap in a SynchronizedMap breaks
expiration.</action>
Review Comment:
Release note mentions "SynchronizedMap", which is an internal JDK
implementation detail. Using the public API name "Collections.synchronizedMap"
is clearer for users reading the changelog.
##########
src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java:
##########
@@ -255,4 +258,185 @@ private void validateExpiration(final Map<String, String>
map, final long timeou
assertNull(map.get("a"));
}
+ @Test
+ void testCollectionsSynchronizedMapExpiration() throws
InterruptedException {
+ final Map<String, String> map = Collections.synchronizedMap(new
PassiveExpiringMap<>(50L));
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ // Cache the views in SynchronizedMap before they expire
+ final Collection<Map.Entry<String, String>> entrySet = map.entrySet();
+ final Collection<String> keySet = map.keySet();
+ final Collection<String> values = map.values();
+ Thread.sleep(100L);
+ // entrySet iterator triggers expiration
+ synchronized (map) {
+ assertTrue(entrySet.isEmpty());
+ assertTrue(keySet.isEmpty());
+ assertTrue(values.isEmpty());
+ }
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ Thread.sleep(100L);
+ // keySet iterator triggers expiration
Review Comment:
Comment says the keySet *iterator* triggers expiration, but the code below
asserts via isEmpty() on the cached view. Update the comment to reflect what
the test actually does.
##########
src/test/java/org/apache/commons/collections4/map/PassiveExpiringMapTest.java:
##########
@@ -255,4 +258,185 @@ private void validateExpiration(final Map<String, String>
map, final long timeou
assertNull(map.get("a"));
}
+ @Test
+ void testCollectionsSynchronizedMapExpiration() throws
InterruptedException {
+ final Map<String, String> map = Collections.synchronizedMap(new
PassiveExpiringMap<>(50L));
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ // Cache the views in SynchronizedMap before they expire
+ final Collection<Map.Entry<String, String>> entrySet = map.entrySet();
+ final Collection<String> keySet = map.keySet();
+ final Collection<String> values = map.values();
+ Thread.sleep(100L);
+ // entrySet iterator triggers expiration
+ synchronized (map) {
+ assertTrue(entrySet.isEmpty());
+ assertTrue(keySet.isEmpty());
+ assertTrue(values.isEmpty());
+ }
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ Thread.sleep(100L);
+ // keySet iterator triggers expiration
+ synchronized (map) {
+ assertTrue(entrySet.isEmpty());
+ assertTrue(keySet.isEmpty());
+ assertTrue(values.isEmpty());
+ }
+ map.put("a", "b");
+ map.put("c", "d");
+ assertEquals(2, map.size());
+ Thread.sleep(100L);
+ // values iterator triggers expiration
Review Comment:
Comment says the values *iterator* triggers expiration, but the assertions
use isEmpty() on the cached values view. Adjust wording to match the actual
trigger under test.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
