Fixed IceLocatorDiscovery race condition, fixes #555

2 files changed, 57 insertions, 11 deletions

diff --git a/java/src/IceLocatorDiscovery/src/main/java/com/zeroc/IceLocatorDiscovery/PluginI.java b/java/src/IceLocatorDiscovery/src/main/java/com/zeroc/IceLocatorDiscovery/PluginI.java
index 18e0043493e..07c709c221f 100644
--- a/java/src/IceLocatorDiscovery/src/main/java/com/zeroc/IceLocatorDiscovery/PluginI.java
+++ b/java/src/IceLocatorDiscovery/src/main/java/com/zeroc/IceLocatorDiscovery/PluginI.java
@@ -138,8 +138,20 @@ class PluginI implements Plugin
         {
             _lookup = lookup;
             _timeout = properties.getPropertyAsIntWithDefault(name + ".Timeout", 300);
+            if(_timeout < 0)
+            {
+                _timeout = 300;
+            }
             _retryCount = properties.getPropertyAsIntWithDefault(name + ".RetryCount", 3);
+            if(_retryCount < 0)
+            {
+                _retryCount = 0;
+            }
             _retryDelay = properties.getPropertyAsIntWithDefault(name + ".RetryDelay", 2000);
+            if(_retryDelay < 0)
+            {
+                _retryDelay = 0;
+            }
             _timer = com.zeroc.IceInternal.Util.getInstance(lookup.ice_getCommunicator()).timer();
             _traceLevel = properties.getPropertyAsInt(name + ".Trace.Lookup");
             _instanceName = instanceName;
@@ -147,6 +159,7 @@ class PluginI implements Plugin
             _locator = lookup.ice_getCommunicator().getDefaultLocator();
             _voidLocator = voidLocator;
             _pendingRetryCount = 0;
+            _pending = false;
             _failureCount = 0;
             _warnOnce = true;
 
@@ -232,7 +245,7 @@ class PluginI implements Plugin
                 {
                     synchronized(this)
                     {
-                        while(!_locators.containsKey(instanceName) && _pendingRetryCount > 0)
+                        while(!_locators.containsKey(instanceName) && _pending)
                         {
                             wait(waitTime);
                         }
@@ -290,12 +303,12 @@ class PluginI implements Plugin
                 return;
             }
 
-            if(_pendingRetryCount > 0) // No need to retry, we found a locator
+            if(_pending) // No need to continue, we found a locator
             {
                 _future.cancel(false);
                 _future = null;
-
                 _pendingRetryCount = 0;
+                _pending = false;
             }
 
             if(_traceLevel > 0)
@@ -389,10 +402,11 @@ class PluginI implements Plugin
                     _pendingRequests.add(request);
                 }
 
-                if(_pendingRetryCount == 0) // No request in progress
+                if(!_pending) // No request in progress
                 {
-                    _failureCount = 0;
+                    _pending = true;
                     _pendingRetryCount = _retryCount;
+                    _failureCount = 0;
                     try
                     {
                         if(_traceLevel > 1)
@@ -436,6 +450,7 @@ class PluginI implements Plugin
                             req.invoke(_voidLocator);
                         }
                         _pendingRequests.clear();
+                        _pending = false;
                         _pendingRetryCount = 0;
                     }
                 }
@@ -444,15 +459,15 @@ class PluginI implements Plugin
 
         synchronized void exception(Throwable ex)
         {
-            if(++_failureCount == _lookups.size() && _pendingRetryCount > 0)
+            if(++_failureCount == _lookups.size() && _pending)
             {
                 //
                 // All the lookup calls failed, cancel the timer and propagate the error to the requests.
                 //
                 _future.cancel(false);
                 _future = null;
-
                 _pendingRetryCount = 0;
+                _pending = false;
 
                 if(_warnOnce)
                 {
@@ -499,8 +514,15 @@ class PluginI implements Plugin
             {
                 synchronized(LocatorI.this)
                 {
-                    if(--_pendingRetryCount > 0)
+                    if(!_pending)
                     {
+                        assert(_pendingRequests.isEmpty());
+                        return; // Request failed
+                    }
+
+                    if(_pendingRetryCount > 0)
+                    {
+                        --_pendingRetryCount;
                         try
                         {
                             if(_traceLevel > 1)
@@ -535,6 +557,9 @@ class PluginI implements Plugin
                         _pendingRetryCount = 0;
                     }
 
+                    assert(_pendingRetryCount == 0);
+                    _pending = false;
+
                     if(_traceLevel > 0)
                     {
                         StringBuilder s = new StringBuilder("locator lookup timed out:\nlookup = ");
@@ -566,12 +591,12 @@ class PluginI implements Plugin
 
         private final LookupPrx _lookup;
         private final Map<LookupPrx, LookupReplyPrx> _lookups = new java.util.HashMap<>();
-        private final int _timeout;
+        private int _timeout;
         private java.util.concurrent.Future<?> _future;
         private final java.util.concurrent.ScheduledExecutorService _timer;
         private final int _traceLevel;
-        private final int _retryCount;
-        private final int _retryDelay;
+        private int _retryCount;
+        private int _retryDelay;
 
         private String _instanceName;
         private boolean _warned;
@@ -579,6 +604,7 @@ class PluginI implements Plugin
         private com.zeroc.Ice.LocatorPrx _voidLocator;
         private Map<String, com.zeroc.Ice.LocatorPrx> _locators = new HashMap<>();
 
+        private boolean _pending;
         private int _pendingRetryCount;
         private int _failureCount;
         private boolean _warnOnce;
diff --git a/java/test/src/main/java/test/IceGrid/simple/AllTests.java b/java/test/src/main/java/test/IceGrid/simple/AllTests.java
index 9c53a754803..1132be3b311 100644
--- a/java/test/src/main/java/test/IceGrid/simple/AllTests.java
+++ b/java/test/src/main/java/test/IceGrid/simple/AllTests.java
@@ -173,6 +173,26 @@ public class AllTests
 
             initData.properties = communicator.getProperties()._clone();
             initData.properties.setProperty("Ice.Default.Locator", "");
+            initData.properties.setProperty("IceLocatorDiscovery.RetryCount", "0");
+            initData.properties.setProperty("Ice.Plugin.IceLocatorDiscovery",
+                                            "com.zeroc.IceLocatorDiscovery.PluginFactory");
+            initData.properties.setProperty("IceLocatorDiscovery.Lookup",
+                                             "udp -h " + multicast + " --interface unknown");
+            comm = com.zeroc.Ice.Util.initialize(initData);
+            test(comm.getDefaultLocator() != null);
+            try
+            {
+                comm.stringToProxy("test @ TestAdapter").ice_ping();
+                test(false);
+            }
+            catch(com.zeroc.Ice.NoEndpointException ex)
+            {
+            }
+            comm.destroy();
+
+            initData.properties = communicator.getProperties()._clone();
+            initData.properties.setProperty("Ice.Default.Locator", "");
+            initData.properties.setProperty("IceLocatorDiscovery.RetryCount", "1");
             initData.properties.setProperty("Ice.Plugin.IceLocatorDiscovery",
                                             "com.zeroc.IceLocatorDiscovery.PluginFactory");
             {