Iterator fixes

3 files changed, 48 insertions, 67 deletions

diff --git a/java/src/Freeze/ConnectionI.java b/java/src/Freeze/ConnectionI.java
index 9edd48b88a0..7237e7f4c69 100755
--- a/java/src/Freeze/ConnectionI.java
+++ b/java/src/Freeze/ConnectionI.java
@@ -85,9 +85,10 @@ class ConnectionI extends Ice.LocalObjectImpl implements Connection
 	

 	synchronized(this)

 	{

-	    while(!_mapList.isEmpty())

+	    java.util.Iterator p = _mapList.iterator();

+	    while(p.hasNext())

 	    {

-		((Map) _mapList.get(0)).close(finalizing);

+		((Map) p.next()).close(finalizing);

 	    }

 	}

 	

@@ -115,8 +116,8 @@ class ConnectionI extends Ice.LocalObjectImpl implements Connection
 	_trace = _communicator.getProperties().getPropertyAsInt("Freeze.Trace.Map");

 	Ice.Properties properties = _communicator.getProperties();

 

-	_deadlockWarning = properties.getPropertyAsInt("Freeze.Warn.Deadlocks") != 0;

-	_keepIterators = properties.getPropertyAsIntWithDefault("Freeze.Map.KeepIterators", 1) != 0;

+	_deadlockWarning = properties.getPropertyAsInt("Freeze.Warn.Deadlocks") > 0;

+	_closeInFinalizeWarning = properties.getPropertyAsIntWithDefault("Freeze.Warn.CloseInFinalize", 1) > 0;

     }

 

     ConnectionI(Ice.Communicator communicator, String envName, com.sleepycat.db.DbEnv dbEnv)

@@ -141,16 +142,19 @@ class ConnectionI extends Ice.LocalObjectImpl implements Connection
 	}

     }

 

-    synchronized void

+    synchronized java.util.Iterator

     registerMap(Map map)

     {

-	_mapList.add(map);

+	_mapList.addFirst(map);

+	java.util.Iterator p = _mapList.iterator();

+	p.next();

+	return p;

     }

 

     synchronized void

-    unregisterMap(Map map)

+    unregisterMap(java.util.Iterator p)

     {

-	_mapList.remove(map);

+	p.remove();

     }

 

     void

@@ -203,9 +207,9 @@ class ConnectionI extends Ice.LocalObjectImpl implements Connection
     }

 

     final boolean

-    keepIterators()

+    closeInFinalizeWarning()

     {

-	return _keepIterators;

+	return _closeInFinalizeWarning;

     }

 

     private Ice.Communicator _communicator;

@@ -213,8 +217,8 @@ class ConnectionI extends Ice.LocalObjectImpl implements Connection
     private com.sleepycat.db.DbEnv _dbEnv;

     private String _envName;

     private TransactionI _transaction;

-    private java.util.List _mapList = new java.util.LinkedList();

+    private LinkedList _mapList = new Freeze.LinkedList();

     private int _trace;

     private boolean _deadlockWarning;

-    private boolean _keepIterators;

+    private boolean _closeInFinalizeWarning;

 }

diff --git a/java/src/Freeze/Map.java b/java/src/Freeze/Map.java
index ec85e6e7542..6c06f009a0b 100644
--- a/java/src/Freeze/Map.java
+++ b/java/src/Freeze/Map.java
@@ -24,7 +24,7 @@ public abstract class Map extends java.util.AbstractMap
 	_trace = _connection.trace();
 	_db = SharedDb.get(_connection, dbName, createDb);
 	
-	_connection.registerMap(this);
+	_token = _connection.registerMap(this);
     }
 
     public void
@@ -413,32 +413,16 @@ public abstract class Map extends java.util.AbstractMap
     void
     closeAllIteratorsExcept(Object except, boolean finalizing)
     {
-	java.util.Iterator p = _iteratorList.iterator();
-	int count = 0;
-
-	if(_connection.keepIterators())
-	{
-	    while(p.hasNext())
-	    {
-		Object o = p.next();
-		if(o != except)
-		{
-		    ((EntryIterator) o).close(finalizing);
-		}
-	    }
-	}
-	else
+	synchronized(_iteratorList)
 	{
+	    java.util.Iterator p = _iteratorList.iterator();
+
 	    while(p.hasNext())
 	    {
-		Object o = ((java.lang.ref.WeakReference) p.next()).get();
-		if(o != except && o != null)
+		Object obj = p.next();
+		if(obj != except)
 		{
-		    ((EntryIterator) o).close(finalizing);
-		}
-		if(o == null || o != except)
-		{
-		    p.remove();
+		    ((EntryIterator) obj).close(finalizing);
 		}
 	    }
 	}
@@ -450,11 +434,8 @@ public abstract class Map extends java.util.AbstractMap
 	close(true);
     }
 
-    
     //
     // The synchronization is only needed when finalizing is true
-    // The extra runtime cost is not worth the added complexity
-    // of synchronizing only when finalizing is true.
     //
     void 
     close(boolean finalizing)
@@ -471,7 +452,8 @@ public abstract class Map extends java.util.AbstractMap
 		finally
 		{
 		    _db = null;
-		    _connection.unregisterMap(this);
+		    _connection.unregisterMap(_token);
+		    _token = null;
 		}
 	    }
 	}
@@ -725,13 +707,13 @@ public abstract class Map extends java.util.AbstractMap
 		throw ex;
 	    }
 
-	    if(_connection.keepIterators())
+	    synchronized(_iteratorList)
 	    {
-		_iteratorList.add(this);
-	    }
-	    else
-	    {
-		_iteratorList.add(new java.lang.ref.WeakReference(this));
+		_iteratorList.addFirst(this);
+		java.util.Iterator p = _iteratorList.iterator();
+		p.next();
+		_iteratorListToken = p;
+
 	    }
         }
 
@@ -905,11 +887,10 @@ public abstract class Map extends java.util.AbstractMap
 	// The synchronized is needed because this method can be called 
 	// concurrently by Connection, Map and Map.EntryIterator finalizers.
 	//
-
 	synchronized void
 	close(boolean finalizing)
 	{
-	    if(finalizing && (_cursor != null || _txn != null))
+	    if(finalizing && (_cursor != null || _txn != null) && _connection.closeInFinalizeWarning())
 	    {
 		_connection.communicator().getLogger().warning
 		    ("finalize() closing a live iterator on Map \"" + _db.dbName() + "\"; the application "
@@ -917,7 +898,16 @@ public abstract class Map extends java.util.AbstractMap
 		     + "Map.closeAllIterators(), Map.close(), Connection.close(), or (if also "
 		     + "leaking a transaction) Transaction.commit() or Transaction.rollback()");
 	    }
-	    
+	 
+	    if(_iteratorListToken != null)
+	    {
+		synchronized(_iteratorList)
+		{
+		    _iteratorListToken.remove();
+		    _iteratorListToken = null;
+		}
+	    }
+   
 	    if(_cursor != null)	
 	    {
 		com.sleepycat.db.Dbc cursor = _cursor;
@@ -970,22 +960,7 @@ public abstract class Map extends java.util.AbstractMap
 	protected void
         finalize()
         {
-	    //
-	    // finalizing denotes the finalization of the containing
-	    // Map and/or Connection.
-	    // When keepIterators is true, the only way this iterator
-	    // can be finalized is when the containing Map (and indirectly
-	    // Connection) is also finalizable, so we set finalizing 
-	    // to true.
-	    // When iterators are kept as weak-references (i.e. keepIterators
-	    // is false), we can't issue finalization warnings because
-	    // early finalization, for example before an explicit Map.close(),
-	    // is actually a good thing since it releases locks earlier, 
-	    // improving concurrency.
-	    //
-	    boolean finalizing = _connection.keepIterators();
-	  
-            close(finalizing);
+            close(true);
         }
 
 	void
@@ -1156,6 +1131,7 @@ public abstract class Map extends java.util.AbstractMap
         private com.sleepycat.db.Dbc _cursor;
         private Entry _current;
         private Entry _lastReturned;
+	private java.util.Iterator _iteratorListToken;
     }
 
     static class Entry implements java.util.Map.Entry 
@@ -1287,8 +1263,9 @@ public abstract class Map extends java.util.AbstractMap
     }
     
     private ConnectionI _connection;
+    private java.util.Iterator _token;
     private java.util.Set _entrySet;
-    private java.util.List _iteratorList = new java.util.LinkedList();
+    private LinkedList _iteratorList = new LinkedList();
     private SharedDb _db;
     private String _errorPrefix;
     private int _trace;
diff --git a/java/src/Ice/PropertiesI.java b/java/src/Ice/PropertiesI.java
index cda48b92bce..d050ca4fb7c 100644
--- a/java/src/Ice/PropertiesI.java
+++ b/java/src/Ice/PropertiesI.java
@@ -566,12 +566,12 @@ final class PropertiesI extends LocalObjectImpl implements Properties
     private static final String _freezeProps[] =
     {
 	"Warn.Deadlocks",
+	"Warn.CloseInFinalize",
 	"Trace.Map",
 	"Trace.Evictor",
 	"Trace.DbEnv",
 	"Evictor.*",
-	"DbEnv.*",
-	"Map.KeepIterators"
+	"DbEnv.*"
     };
 
     private static final class ValidProps