ICE-5630 - ACM timeout is broken

- Also refactored the code a bit in java so that the ACMMonitor doesn't
  inherit from runnable.

6 files changed, 42 insertions, 27 deletions

diff --git a/cpp/src/Ice/ConnectionI.cpp b/cpp/src/Ice/ConnectionI.cpp
index 37afd228ca7..b42b8558a7f 100644
--- a/cpp/src/Ice/ConnectionI.cpp
+++ b/cpp/src/Ice/ConnectionI.cpp
@@ -1197,10 +1197,6 @@ Ice::ConnectionI::setACM(const IceUtil::Optional<int>& timeout,
             _monitor->remove(this);
         }
         _monitor = _monitor->acm(timeout, close, heartbeat);
-        if(_state == StateActive)
-        {
-            _monitor->add(this);
-        }
 
         if(_monitor->getACM().timeout <= 0)
         {
@@ -1210,6 +1206,11 @@ Ice::ConnectionI::setACM(const IceUtil::Optional<int>& timeout,
         {
             _acmLastActivity = IceUtil::Time::now(IceUtil::Time::Monotonic);
         }
+
+        if(_state == StateActive)
+        {
+            _monitor->add(this);
+        }
     }
 }
 
@@ -2483,11 +2484,11 @@ Ice::ConnectionI::setState(State state)
     {
         if(state == StateActive)
         {
-            _monitor->add(this);
             if(_acmLastActivity != IceUtil::Time())
             {
                 _acmLastActivity = IceUtil::Time::now(IceUtil::Time::Monotonic);
             }
+            _monitor->add(this);
         }
         else if(_state == StateActive)
         {
diff --git a/cs/src/Ice/ConnectionI.cs b/cs/src/Ice/ConnectionI.cs
index adeebbe7e9a..84ece0190ef 100644
--- a/cs/src/Ice/ConnectionI.cs
+++ b/cs/src/Ice/ConnectionI.cs
@@ -901,10 +901,6 @@ namespace Ice
                         _monitor.remove(this);
                     }
                     _monitor = _monitor.acm(timeout, close, heartbeat);
-                    if(_state == StateActive)
-                    {
-                        _monitor.add(this);
-                    }
 
                     if(_monitor.getACM().timeout <= 0)
                     {
@@ -914,6 +910,12 @@ namespace Ice
                     {
                         _acmLastActivity = IceInternal.Time.currentMonotonicTimeMillis();
                     }
+
+                    if(_state == StateActive)
+                    {
+                        _monitor.add(this);
+                    }
+
                 }
             }
         }
@@ -2091,11 +2093,11 @@ namespace Ice
             {
                 if(state == StateActive)
                 {
-                    _monitor.add(this);
                     if(_acmLastActivity > 0)
                     {
                         _acmLastActivity = IceInternal.Time.currentMonotonicTimeMillis();
                     }
+                    _monitor.add(this);
                 }
                 else if(_state == StateActive)
                 {
diff --git a/java/src/Ice/ConnectionI.java b/java/src/Ice/ConnectionI.java
index 3e8c735a99d..c835e398735 100644
--- a/java/src/Ice/ConnectionI.java
+++ b/java/src/Ice/ConnectionI.java
@@ -893,11 +893,7 @@ public final class ConnectionI extends IceInternal.EventHandler implements Conne
                 _monitor.remove(this);
             }
             _monitor = _monitor.acm(timeout, close, heartbeat);
-            if(_state == StateActive)
-            {
-                _monitor.add(this);
-            }
-
+            
             if(_monitor.getACM().timeout <= 0)
             {
                 _acmLastActivity = -1; // Disable the recording of last activity.
@@ -906,6 +902,12 @@ public final class ConnectionI extends IceInternal.EventHandler implements Conne
             {
                 _acmLastActivity = IceInternal.Time.currentMonotonicTimeMillis();
             }
+
+            if(_state == StateActive)
+            {
+                _monitor.add(this);
+            }
+
         }
     }
 
@@ -2066,11 +2068,11 @@ public final class ConnectionI extends IceInternal.EventHandler implements Conne
         {
             if(state == StateActive)
             {
-                _monitor.add(this);
                 if(_acmLastActivity > 0)
                 {
                     _acmLastActivity = IceInternal.Time.currentMonotonicTimeMillis();
                 }
+                _monitor.add(this);
             }
             else if(_state == StateActive)
             {
diff --git a/java/src/IceInternal/ACMMonitor.java b/java/src/IceInternal/ACMMonitor.java
index 5d592e62b22..7b9fad1bc3d 100644
--- a/java/src/IceInternal/ACMMonitor.java
+++ b/java/src/IceInternal/ACMMonitor.java
@@ -9,7 +9,7 @@
 
 package IceInternal;
 
-public interface ACMMonitor extends Runnable
+public interface ACMMonitor
 {
     void add(Ice.ConnectionI con);
     void remove(Ice.ConnectionI con);
diff --git a/java/src/IceInternal/ConnectionACMMonitor.java b/java/src/IceInternal/ConnectionACMMonitor.java
index 38c57189dd9..c0a135123fa 100644
--- a/java/src/IceInternal/ConnectionACMMonitor.java
+++ b/java/src/IceInternal/ConnectionACMMonitor.java
@@ -45,8 +45,14 @@ class ConnectionACMMonitor implements ACMMonitor
         _connection = connection;
         if(_config.timeout > 0)
         {
-            _future = _timer.scheduleAtFixedRate(this, _config.timeout / 2, _config.timeout / 2,
-                java.util.concurrent.TimeUnit.MILLISECONDS);
+            _future = _timer.scheduleAtFixedRate(new Runnable() {
+                @Override
+                public void run()
+                {
+                    monitorConnection();
+                }
+            },
+            _config.timeout / 2, _config.timeout / 2, java.util.concurrent.TimeUnit.MILLISECONDS);
         }
     }
 
@@ -88,9 +94,8 @@ class ConnectionACMMonitor implements ACMMonitor
         return acm;
     }
 
-    @Override
-    public void
-    run()
+    private void
+    monitorConnection()
     {
         Ice.ConnectionI connection;
         synchronized(this)
diff --git a/java/src/IceInternal/FactoryACMMonitor.java b/java/src/IceInternal/FactoryACMMonitor.java
index 420513ff9dd..0f9b6d18f00 100644
--- a/java/src/IceInternal/FactoryACMMonitor.java
+++ b/java/src/IceInternal/FactoryACMMonitor.java
@@ -77,8 +77,14 @@ class FactoryACMMonitor implements ACMMonitor
             {
                 _connections.add(connection);
                 assert _future == null;
-                _future = _instance.timer().scheduleAtFixedRate(this, _config.timeout / 2, _config.timeout / 2,
-                    java.util.concurrent.TimeUnit.MILLISECONDS);
+                _future = _instance.timer().scheduleAtFixedRate(new Runnable() {
+                    @Override
+                    public void run()
+                    {
+                        monitorConnections();
+                    }
+                },
+                _config.timeout / 2, _config.timeout / 2, java.util.concurrent.TimeUnit.MILLISECONDS);
             }
             else
             {
@@ -155,9 +161,8 @@ class FactoryACMMonitor implements ACMMonitor
         return connections;
     }
 
-    @Override
-    public void
-    run()
+    private void
+    monitorConnections()
     {
         synchronized(this)
         {