From 007d3e3702b22b1593eafa42856f867b9c92eb37 Mon Sep 17 00:00:00 2001 From: Stephen Colebourne Date: Sat, 27 Sep 2003 10:07:14 +0000 Subject: [PATCH] Change CursorableLinkedList to use weak references to avoid memory leaks from Simon Kitching git-svn-id: https://svn.apache.org/repos/asf/jakarta/commons/proper/collections/trunk@131194 13f79535-47bb-0310-9956-ffa450edef68 --- .../collections/CursorableLinkedList.java | 91 +++++++++++++++---- .../collections/TestCursorableLinkedList.java | 84 ++++++++++++++++- 2 files changed, 152 insertions(+), 23 deletions(-) diff --git a/src/java/org/apache/commons/collections/CursorableLinkedList.java b/src/java/org/apache/commons/collections/CursorableLinkedList.java index 035cca4e1..967cea2b0 100644 --- a/src/java/org/apache/commons/collections/CursorableLinkedList.java +++ b/src/java/org/apache/commons/collections/CursorableLinkedList.java @@ -1,5 +1,5 @@ /* - * $Header: /home/jerenkrantz/tmp/commons/commons-convert/cvs/home/cvs/jakarta-commons//collections/src/java/org/apache/commons/collections/CursorableLinkedList.java,v 1.17 2003/09/20 14:03:57 scolebourne Exp $ + * $Header: /home/jerenkrantz/tmp/commons/commons-convert/cvs/home/cvs/jakarta-commons//collections/src/java/org/apache/commons/collections/CursorableLinkedList.java,v 1.18 2003/09/27 10:07:14 scolebourne Exp $ * ==================================================================== * * The Apache Software License, Version 1.1 @@ -69,6 +69,7 @@ import java.util.Iterator; import java.util.List; import java.util.ListIterator; import java.util.NoSuchElementException; +import java.lang.ref.WeakReference; /** * A doubly-linked list implementation of the {@link List} interface, @@ -84,14 +85,13 @@ import java.util.NoSuchElementException; * * @see java.util.LinkedList * @since Commons Collections 1.0 - * @version $Revision: 1.17 $ $Date: 2003/09/20 14:03:57 $ + * @version $Revision: 1.18 $ $Date: 2003/09/27 10:07:14 $ * * @author Rodney Waldhoff * @author Janek Bogucki + * @author Simon Kitching */ public class CursorableLinkedList implements List, Serializable { - // TODO: use weak references to cursors in case they aren't closed directly - /** Ensure serialization compatability */ private static final long serialVersionUID = 8836393098519411393L; @@ -304,11 +304,9 @@ public class CursorableLinkedList implements List, Serializable { * {@link ListIterator#nextIndex} and {@link ListIterator#previousIndex} * methods (they throw {@link UnsupportedOperationException} when invoked. *

- * Clients must close the cursor when they are done using it. - * The returned {@link ListIterator} will be an instance of - * {@link CursorableLinkedList.Cursor}. To close the cursor, - * cast the {@link ListIterator} to {@link CursorableLinkedList.Cursor} - * and invoke the {@link CursorableLinkedList.Cursor#close} method. + * Historical Note: In previous versions of this class, the object + * returned from this method was required to be explicitly closed. This + * is no longer necessary. * * @see #cursor(int) * @see #listIterator @@ -839,7 +837,16 @@ public class CursorableLinkedList implements List, Serializable { * of changes to this list. */ protected void registerCursor(Cursor cur) { - _cursors.add(cur); + // We take this opportunity to clean the _cursors list + // of WeakReference objects to garbage-collected cursors. + for (Iterator it = _cursors.iterator(); it.hasNext(); ) { + WeakReference ref = (WeakReference) it.next(); + if (ref.get() == null) { + it.remove(); + } + } + + _cursors.add( new WeakReference(cur) ); } /** @@ -847,7 +854,21 @@ public class CursorableLinkedList implements List, Serializable { * the set of cursors to be notified of changes to this list. */ protected void unregisterCursor(Cursor cur) { - _cursors.remove(cur); + for (Iterator it = _cursors.iterator(); it.hasNext(); ) { + WeakReference ref = (WeakReference) it.next(); + Cursor cursor = (Cursor) ref.get(); + if (cursor == null) { + // some other unrelated cursor object has been + // garbage-collected; let's take the opportunity to + // clean up the cursors list anyway.. + it.remove(); + + } else if (cursor == cur) { + ref.clear(); + it.remove(); + break; + } + } } /** @@ -856,8 +877,14 @@ public class CursorableLinkedList implements List, Serializable { */ protected void invalidateCursors() { Iterator it = _cursors.iterator(); - while(it.hasNext()) { - ((Cursor)it.next()).invalidate(); + while (it.hasNext()) { + WeakReference ref = (WeakReference) it.next(); + Cursor cursor = (Cursor) ref.get(); + if (cursor != null) { + // cursor is null if object has been garbage-collected + cursor.invalidate(); + ref.clear(); + } it.remove(); } } @@ -869,8 +896,14 @@ public class CursorableLinkedList implements List, Serializable { */ protected void broadcastListableChanged(Listable elt) { Iterator it = _cursors.iterator(); - while(it.hasNext()) { - ((Cursor)it.next()).listableChanged(elt); + while (it.hasNext()) { + WeakReference ref = (WeakReference) it.next(); + Cursor cursor = (Cursor) ref.get(); + if (cursor == null) { + it.remove(); // clean up list + } else { + cursor.listableChanged(elt); + } } } @@ -880,8 +913,14 @@ public class CursorableLinkedList implements List, Serializable { */ protected void broadcastListableRemoved(Listable elt) { Iterator it = _cursors.iterator(); - while(it.hasNext()) { - ((Cursor)it.next()).listableRemoved(elt); + while (it.hasNext()) { + WeakReference ref = (WeakReference) it.next(); + Cursor cursor = (Cursor) ref.get(); + if (cursor == null) { + it.remove(); // clean up list + } else { + cursor.listableRemoved(elt); + } } } @@ -891,8 +930,14 @@ public class CursorableLinkedList implements List, Serializable { */ protected void broadcastListableInserted(Listable elt) { Iterator it = _cursors.iterator(); - while(it.hasNext()) { - ((Cursor)it.next()).listableInserted(elt); + while (it.hasNext()) { + WeakReference ref = (WeakReference) it.next(); + Cursor cursor = (Cursor) ref.get(); + if (cursor == null) { + it.remove(); // clean up list + } else { + cursor.listableInserted(elt); + } } } @@ -1171,6 +1216,14 @@ public class CursorableLinkedList implements List, Serializable { _valid = false; } + /** + * Mark this cursor as no longer being needed. Any resources + * associated with this cursor are immediately released. + * In previous versions of this class, it was mandatory to close + * all cursor objects to avoid memory leaks. It is no longer + * necessary to call this close method; an instance of this class + * can now be treated exactly like a normal iterator. + */ public void close() { if(_valid) { _valid = false; diff --git a/src/test/org/apache/commons/collections/TestCursorableLinkedList.java b/src/test/org/apache/commons/collections/TestCursorableLinkedList.java index b4448f235..fab58b1ea 100644 --- a/src/test/org/apache/commons/collections/TestCursorableLinkedList.java +++ b/src/test/org/apache/commons/collections/TestCursorableLinkedList.java @@ -1,7 +1,7 @@ /* - * $Header: /home/jerenkrantz/tmp/commons/commons-convert/cvs/home/cvs/jakarta-commons//collections/src/test/org/apache/commons/collections/TestCursorableLinkedList.java,v 1.10 2003/09/20 14:03:57 scolebourne Exp $ - * $Revision: 1.10 $ - * $Date: 2003/09/20 14:03:57 $ + * $Header: /home/jerenkrantz/tmp/commons/commons-convert/cvs/home/cvs/jakarta-commons//collections/src/test/org/apache/commons/collections/TestCursorableLinkedList.java,v 1.11 2003/09/27 10:07:14 scolebourne Exp $ + * $Revision: 1.11 $ + * $Date: 2003/09/27 10:07:14 $ * * ==================================================================== * @@ -70,8 +70,11 @@ import java.util.ListIterator; import junit.framework.Test; /** + * Test class. + * * @author Rodney Waldhoff - * @version $Id: TestCursorableLinkedList.java,v 1.10 2003/09/20 14:03:57 scolebourne Exp $ + * @author Simon Kitching + * @version $Id: TestCursorableLinkedList.java,v 1.11 2003/09/27 10:07:14 scolebourne Exp $ */ public class TestCursorableLinkedList extends TestList { public TestCursorableLinkedList(String testName) { @@ -334,6 +337,79 @@ public class TestCursorableLinkedList extends TestList { it.close(); } + public void testCursorConcurrentModification() { + // this test verifies that cursors remain valid when the list + // is modified via other means. + list.add("1"); + list.add("2"); + list.add("3"); + list.add("5"); + list.add("7"); + list.add("9"); + + CursorableLinkedList.Cursor c1 = list.cursor(); + CursorableLinkedList.Cursor c2 = list.cursor(); + ListIterator li = list.listIterator(); + + // test cursors remain valid when list modified by std ListIterator + // test cursors skip elements removed via ListIterator + assertEquals("1",li.next()); + assertEquals("2",li.next()); + li.remove(); + assertEquals("3",li.next()); + assertEquals("1",c1.next()); + assertEquals("3",c1.next()); + assertEquals("1",c2.next()); + + // test cursor c1 can remove elements from previously modified list + // test cursor c2 skips elements removed via different cursor + c1.remove(); + assertEquals("5",c2.next()); + c2.add("6"); + assertEquals("5",c1.next()); + assertEquals("6",c1.next()); + assertEquals("7",c1.next()); + + // test cursors remain valid when list mod via CursorableLinkedList + // test cursor remains valid when elements inserted into list before + // the current position of the cursor. + list.add(0, "0"); + + // test cursor remains valid when element inserted immediately after + // current element of a cursor, and the element is seen on the + // next call to the next method of that cursor. + list.add(5, "8"); + + assertEquals("8",c1.next()); + assertEquals("9",c1.next()); + c1.add("10"); + assertEquals("7",c2.next()); + assertEquals("8",c2.next()); + assertEquals("9",c2.next()); + assertEquals("10",c2.next()); + + boolean nosuch = false; + try { + c2.next(); + } + catch (java.util.NoSuchElementException nse) { + nosuch = true; // expected + } + assertTrue(nosuch); + + boolean listIteratorInvalid = false; + try { + li.next(); + } + catch(java.util.ConcurrentModificationException cme) { + listIteratorInvalid = true; // expected + } + assertTrue(listIteratorInvalid); + + c1.close(); // not necessary + c2.close(); // not necessary + } + public void testEqualsAndHashCode() { assertTrue(list.equals(list)); assertEquals(list.hashCode(),list.hashCode());