Fixed bug 4249 - waitForHold waits outside the OA sync

5 files changed, 143 insertions, 102 deletions

diff --git a/java/src/Ice/ObjectAdapterI.java b/java/src/Ice/ObjectAdapterI.java
index 674e690d27b..7478f6ffb5d 100644
--- a/java/src/Ice/ObjectAdapterI.java
+++ b/java/src/Ice/ObjectAdapterI.java
@@ -38,17 +38,21 @@ public final class ObjectAdapterI implements ObjectAdapter
             checkForDeactivation();
 
             //
+            // If some threads are waiting on waitForHold(), we set this
+            // flag to ensure the threads will start again the wait for
+            // all the incoming connection factories.
+            //
+            _waitForHoldRetry = _waitForHold > 0;
+
+            //
             // If the one off initializations of the adapter are already
             // done, we just need to activate the incoming connection
             // factories and we're done.
             //
             if(_activateOneOffDone)
             {
-                final int sz = _incomingConnectionFactories.size();
-                for(int i = 0; i < sz; ++i)
+                for(IceInternal.IncomingConnectionFactory factory : _incomingConnectionFactories)
                 {
-                    IceInternal.IncomingConnectionFactory factory =
-                        (IceInternal.IncomingConnectionFactory)_incomingConnectionFactories.get(i);
                     factory.activate();
                 }
                 return;
@@ -111,11 +115,8 @@ public final class ObjectAdapterI implements ObjectAdapter
 
             _activateOneOffDone = true;
 
-            final int sz = _incomingConnectionFactories.size();
-            for(int i = 0; i < sz; ++i)
+            for(IceInternal.IncomingConnectionFactory factory : _incomingConnectionFactories)
             {
-                IceInternal.IncomingConnectionFactory factory =
-                    (IceInternal.IncomingConnectionFactory)_incomingConnectionFactories.get(i);
                 factory.activate();
             }
         }
@@ -126,26 +127,68 @@ public final class ObjectAdapterI implements ObjectAdapter
     {
         checkForDeactivation();
 
-        final int sz = _incomingConnectionFactories.size();
-        for(int i = 0; i < sz; ++i)
+        for(IceInternal.IncomingConnectionFactory factory : _incomingConnectionFactories)
         {
-            IceInternal.IncomingConnectionFactory factory =
-                (IceInternal.IncomingConnectionFactory)_incomingConnectionFactories.get(i);
             factory.hold();
         }
     }
 
-    public synchronized void
+    public void
     waitForHold()
     {
-        checkForDeactivation();
-
-        final int sz = _incomingConnectionFactories.size();
-        for(int i = 0; i < sz; ++i)
+        while(true)
         {
-            IceInternal.IncomingConnectionFactory factory =
-                (IceInternal.IncomingConnectionFactory)_incomingConnectionFactories.get(i);
-            factory.waitUntilHolding();
+            java.util.List<IceInternal.IncomingConnectionFactory> incomingConnectionFactories;
+            synchronized(this)
+            {
+                checkForDeactivation();
+            
+                incomingConnectionFactories =
+                    new java.util.ArrayList<IceInternal.IncomingConnectionFactory>(_incomingConnectionFactories);
+
+                ++_waitForHold;
+            }
+
+            for(IceInternal.IncomingConnectionFactory factory : incomingConnectionFactories)
+            {
+                factory.waitUntilHolding();
+            }
+
+            synchronized(this)
+            {
+                if(--_waitForHold == 0)
+                {
+                    notifyAll();
+                }
+                
+                //
+                // If we don't need to retry, we're done. Otherwise, we wait until 
+                // all the waiters finish waiting on the connections and we try 
+                // again waiting on all the conncetions. This is necessary in the 
+                // case activate() is called by another thread while waitForHold()
+                // waits on the some connection, if we didn't retry, waitForHold() 
+                // could return only after waiting on a subset of the connections.
+                //
+                if(!_waitForHoldRetry)
+                {
+                    return;
+                }
+                else
+                {
+                    while(_waitForHold > 0)
+                    {
+                        checkForDeactivation();
+                        try
+                        {
+                            wait();
+                        }
+                        catch(java.lang.InterruptedException ex)
+                        {
+                        }
+                    }
+                    _waitForHoldRetry = false;
+                }
+            }
         }
     }
 
@@ -222,10 +265,8 @@ public final class ObjectAdapterI implements ObjectAdapter
         // Connection::destroy() might block when sending a CloseConnection
         // message.
         //
-        final int sz = incomingConnectionFactories.size();
-        for(int i = 0; i < sz; ++i)
+        for(IceInternal.IncomingConnectionFactory factory : incomingConnectionFactories)
         {
-            IceInternal.IncomingConnectionFactory factory = incomingConnectionFactories.get(i);
             factory.destroy();
         }
 
@@ -348,14 +389,7 @@ public final class ObjectAdapterI implements ObjectAdapter
             _destroyed = true;
             notifyAll();
 
-            //
-            // We're done, now we can throw away all incoming connection
-            // factories.
-            //
-            // For compatibility with C#, we set _incomingConnectionFactories
-            // to null so that the finalizer does not invoke methods on objects.
-            //
-            _incomingConnectionFactories = null;
+            _incomingConnectionFactories.clear();
 
             //
             // Remove object references (some of them cyclic).
@@ -604,9 +638,8 @@ public final class ObjectAdapterI implements ObjectAdapter
     getEndpoints()
     {
         java.util.List<Endpoint> endpoints = new java.util.ArrayList<Endpoint>();
-        for(int i = 0; i < _incomingConnectionFactories.size(); ++i)
+        for(IceInternal.IncomingConnectionFactory factory : _incomingConnectionFactories)
         {
-            IceInternal.IncomingConnectionFactory factory = _incomingConnectionFactories.get(i);
             endpoints.add(factory.endpoint());
         }
         return endpoints.toArray(new Endpoint[0]);
@@ -782,6 +815,8 @@ public final class ObjectAdapterI implements ObjectAdapter
         _name = name;
         _directCount = 0;
         _waitForActivate = false;
+        _waitForHold = 0;
+        _waitForHoldRetry = false;
         _destroying = false;
         _destroyed = false;
         _noConfig = noConfig;
@@ -934,9 +969,8 @@ public final class ObjectAdapterI implements ObjectAdapter
                 //
                 java.util.List<IceInternal.EndpointI> endpoints =
                     parseEndpoints(properties.getProperty(_name + ".Endpoints"), true);
-                for(int i = 0; i < endpoints.size(); ++i)
+                for(IceInternal.EndpointI endp : endpoints)
                 {
-                    IceInternal.EndpointI endp = endpoints.get(i);
                     IceInternal.IncomingConnectionFactory factory =
                         new IceInternal.IncomingConnectionFactory(instance, endp, this, _name);
                     _incomingConnectionFactories.add(factory);
@@ -991,7 +1025,7 @@ public final class ObjectAdapterI implements ObjectAdapter
         {
             IceUtilInternal.Assert.FinalizerAssert(_threadPool == null);
             //IceUtilInternal.Assert.FinalizerAssert(_servantManager == null); // Not cleared, it needs to be immutable.
-            IceUtilInternal.Assert.FinalizerAssert(_incomingConnectionFactories == null);
+            //IceUtilInternal.Assert.FinalizerAssert(_incomingConnectionFactories.isEmpty());
             IceUtilInternal.Assert.FinalizerAssert(_directCount == 0);
             IceUtilInternal.Assert.FinalizerAssert(!_waitForActivate);
         }
@@ -1177,9 +1211,8 @@ public final class ObjectAdapterI implements ObjectAdapter
             // If the PublishedEndpoints property isn't set, we compute the published enpdoints
             // from the OA endpoints.
             //
-            for(int i = 0; i < _incomingConnectionFactories.size(); ++i)
+            for(IceInternal.IncomingConnectionFactory factory : _incomingConnectionFactories)
             {
-                IceInternal.IncomingConnectionFactory factory = _incomingConnectionFactories.get(i);
                 endpoints.add(factory.endpoint());
             }
 
@@ -1500,6 +1533,8 @@ public final class ObjectAdapterI implements ObjectAdapter
     private IceInternal.LocatorInfo _locatorInfo;
     private int _directCount;
     private boolean _waitForActivate;
+    private int _waitForHold;
+    private boolean _waitForHoldRetry;
     private boolean _destroying;
     private boolean _destroyed;
     private boolean _noConfig;
diff --git a/java/src/IceInternal/ObjectAdapterFactory.java b/java/src/IceInternal/ObjectAdapterFactory.java
index 85980cf68f5..381a317e6e5 100644
--- a/java/src/IceInternal/ObjectAdapterFactory.java
+++ b/java/src/IceInternal/ObjectAdapterFactory.java
@@ -29,7 +29,7 @@ public final class ObjectAdapterFactory
             _instance = null;
             _communicator = null;
 
-            adapters = _adapters;
+            adapters = new java.util.LinkedList<Ice.ObjectAdapterI>(_adapters);
         
             notifyAll();
         }
@@ -38,12 +38,9 @@ public final class ObjectAdapterFactory
         // Deactivate outside the thread synchronization, to avoid
         // deadlocks.
         //
-        if(adapters != null)
+        for(Ice.ObjectAdapterI adapter : adapters)
         {
-            for(Ice.ObjectAdapterI adapter : adapters)
-            {
-                adapter.deactivate();
-            }
+            adapter.deactivate();
         }
     }
 
@@ -66,43 +63,16 @@ public final class ObjectAdapterFactory
                 {
                 }
             }
-            
-            //
-            // If some other thread is currently shutting down, we wait
-            // until this thread is finished.
-            //
-            while(_waitForShutdown)
-            {
-                try
-                {
-                    wait();
-                }
-                catch(InterruptedException ex)
-                {
-                }
-            }
-            _waitForShutdown = true;
-            adapters = _adapters;
+
+            adapters = new java.util.LinkedList<Ice.ObjectAdapterI>(_adapters);
         }
         
         //
         // Now we wait for deactivation of each object adapter.
         //
-        if(adapters != null)
-        {
-            for(Ice.ObjectAdapterI adapter : adapters)
-            {
-                adapter.waitForDeactivate();
-            }
-        }
-
-        synchronized(this)
+        for(Ice.ObjectAdapterI adapter : adapters)
         {
-            //
-            // Signal that waiting is complete.
-            //
-            _waitForShutdown = false;
-            notifyAll();
+            adapter.waitForDeactivate();
         }
     }
 
@@ -123,25 +93,17 @@ public final class ObjectAdapterFactory
         java.util.List<Ice.ObjectAdapterI> adapters;
         synchronized(this)
         {
-            adapters = _adapters;
+            adapters = new java.util.LinkedList<Ice.ObjectAdapterI>(_adapters);
+        }
 
-            //
-            // For consistency with C#, we set _adapters to null
-            // so that our finalizer does not try to invoke any
-            // methods on member objects.
-            //
-            _adapters = null;
+        for(Ice.ObjectAdapterI adapter : adapters)
+        {
+            adapter.destroy();
         }
 
-        //
-        // Now we destroy each object adapter.
-        //
-        if(adapters != null)
+        synchronized(this)
         {
-            for(Ice.ObjectAdapterI adapter : adapters)
-            {
-                adapter.destroy();
-            }
+            _adapters.clear();
         }
     }
     
@@ -183,7 +145,7 @@ public final class ObjectAdapterFactory
                 return null;
             }
 
-            adapters = _adapters;
+            adapters = new java.util.LinkedList<Ice.ObjectAdapterI>(_adapters);
         }
 
         for(Ice.ObjectAdapterI adapter : adapters)
@@ -222,12 +184,7 @@ public final class ObjectAdapterFactory
         java.util.List<Ice.ObjectAdapterI> adapters;
         synchronized(this)
         {
-            if(_adapters == null)
-            {
-                return;
-            }
-
-            adapters = _adapters;
+            adapters = new java.util.LinkedList<Ice.ObjectAdapterI>(_adapters);
         }
 
         for(Ice.ObjectAdapterI adapter : adapters)
@@ -243,7 +200,6 @@ public final class ObjectAdapterFactory
     {
         _instance = instance;
         _communicator = communicator;
-        _waitForShutdown = false;
     }
 
     protected synchronized void
@@ -252,8 +208,7 @@ public final class ObjectAdapterFactory
     {
         IceUtilInternal.Assert.FinalizerAssert(_instance == null);
         IceUtilInternal.Assert.FinalizerAssert(_communicator == null);
-        IceUtilInternal.Assert.FinalizerAssert(_adapters == null);
-        IceUtilInternal.Assert.FinalizerAssert(!_waitForShutdown);
+        //IceUtilInternal.Assert.FinalizerAssert(_adapters.isEmpty())
 
         super.finalize();
     }
@@ -262,5 +217,4 @@ public final class ObjectAdapterFactory
     private Ice.Communicator _communicator;
     private java.util.Set<String> _adapterNamesInUse = new java.util.HashSet<String>();
     private java.util.List<Ice.ObjectAdapterI> _adapters = new java.util.LinkedList<Ice.ObjectAdapterI>();
-    private boolean _waitForShutdown;
 }
diff --git a/java/test/Ice/hold/AllTests.java b/java/test/Ice/hold/AllTests.java
index 4999c70cd63..388d179b401 100644
--- a/java/test/Ice/hold/AllTests.java
+++ b/java/test/Ice/hold/AllTests.java
@@ -216,6 +216,26 @@ public class AllTests
         }
         out.println("ok");
 
+        out.print("testing waitForHold... ");
+        out.flush();
+        {
+            hold.waitForHold();
+            hold.waitForHold();
+            for(int i = 0; i < 1000; ++i)
+            {
+                holdOneway.ice_ping();
+                if((i % 20) == 0)
+                {
+                    hold.putOnHold(0);
+                }
+            }
+            hold.putOnHold(-1);
+            hold.ice_ping();
+            hold.putOnHold(-1);
+            hold.ice_ping();            
+        }
+        out.println("ok");
+        
         out.print("changing state to hold and shutting down server... ");
         out.flush();
         hold.shutdown();
diff --git a/java/test/Ice/hold/HoldI.java b/java/test/Ice/hold/HoldI.java
index 06236d11bb9..e004281aa00 100644
--- a/java/test/Ice/hold/HoldI.java
+++ b/java/test/Ice/hold/HoldI.java
@@ -1,3 +1,4 @@
+
 // **********************************************************************
 //
 // Copyright (c) 2003-2009 ZeroC, Inc. All rights reserved.
@@ -32,7 +33,11 @@ public final class HoldI extends _HoldDisp
     public void
     putOnHold(int milliSeconds, Ice.Current current)
     {
-        if(milliSeconds <= 0)
+        if(milliSeconds < 0)
+        {
+            _adapter.hold();
+        }
+        else if(milliSeconds == 0)
         {
             _adapter.hold();
             _adapter.activate();
@@ -56,6 +61,32 @@ public final class HoldI extends _HoldDisp
         }
     }
 
+    public void
+    waitForHold(final Ice.Current current)
+    {
+        _timer.schedule(new java.util.TimerTask() 
+                        {
+                            public void 
+                            run()
+                            {
+                                try
+                                {
+                                    current.adapter.waitForHold();
+                                    current.adapter.activate();
+                                }
+                                catch(Ice.ObjectAdapterDeactivatedException ex)
+                                {
+                                    //
+                                    // This shouldn't occur. The test ensures all the waitForHold timers are 
+                                    // finished before shutting down the communicator.
+                                    //
+                                    test(false);
+                                }
+                            }
+                        }, 0);
+    }
+
+
     public int
     set(int value, int delay, Ice.Current current)
     {
diff --git a/java/test/Ice/hold/Test.ice b/java/test/Ice/hold/Test.ice
index da92d1d1103..c05fe4e94ee 100644
--- a/java/test/Ice/hold/Test.ice
+++ b/java/test/Ice/hold/Test.ice
@@ -17,6 +17,7 @@ module Test
 interface Hold
 {
     void putOnHold(int seconds);
+    void waitForHold();
     ["ami"] int set(int value, int delay);
     void setOneway(int value, int expected);
     void shutdown();