SMACK-388

Use ScheduledExecutorService.
Set ping received when pinging another entity.
Refactored PacketListener and ConnectionListeners as anonymous inner-classes

git-svn-id: http://svn.igniterealtime.org/svn/repos/smack/trunk@13531 b35dd754-fafc-0310-a699-88a17e54d16e

build/resources/META-INF/smack-config.xml

@@ -31,7 +31,7 @@
     <!-- Port of the local Socks5 proxy -->
     <packetCollectorSize>10000</packetCollectorSize>
     
-    <!-- Default interval in which the Ping Manager sends ping request to the server (30 minutes) -->
+    <!-- Default interval (seconds) in which the Ping Manager sends ping request to the server (30 minutes) -->
-    <defaultPingInterval>1800000</defaultPingInterval>
+    <defaultPingInterval>1800</defaultPingInterval>
 
 </smack>

source/org/jivesoftware/smack/SmackConfiguration.java

@@ -58,7 +58,10 @@ public final class SmackConfiguration {
     private static int localSocks5ProxyPort = 7777;
     private static int packetCollectorSize = 5000;
 
-    private static int defaultPingInterval = 1800000; // 30 min (30*60*1000)
+    /**
+     * defaultPingInterval (in seconds)
+     */
+    private static int defaultPingInterval = 1800; // 30 min (30*60)
 
     private SmackConfiguration() {
     }
@@ -308,10 +311,20 @@ public final class SmackConfiguration {
         SmackConfiguration.localSocks5ProxyPort = localSocks5ProxyPort;
     }
 
+    /**
+     * Returns the default ping interval (seconds)
+     * 
+     * @return
+     */
     public static int getDefaultPingInterval() {
         return defaultPingInterval;
     }
 
+    /**
+     * Sets the default ping interval (seconds). Set it to '-1' to disable the periodic ping
+     *
+     * @param defaultPingInterval
+     */
     public static void setDefaultPingInterval(int defaultPingInterval) {
         SmackConfiguration.defaultPingInterval = defaultPingInterval;
     }

source/org/jivesoftware/smackx/ping/PingManager.java

@@ -21,10 +21,14 @@ import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 import java.util.WeakHashMap;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 
-import org.jivesoftware.smack.AbstractConnectionListener;
 import org.jivesoftware.smack.Connection;
 import org.jivesoftware.smack.ConnectionCreationListener;
+import org.jivesoftware.smack.ConnectionListener;
 import org.jivesoftware.smack.PacketCollector;
 import org.jivesoftware.smack.PacketListener;
 import org.jivesoftware.smack.SmackConfiguration;
@@ -55,6 +59,7 @@ public class PingManager {
     public static final String NAMESPACE = "urn:xmpp:ping";
     public static final String ELEMENT = "ping";
 
+
     private static Map<Connection, PingManager> instances =
             Collections.synchronizedMap(new WeakHashMap<Connection, PingManager>());
 
@@ -66,29 +71,79 @@ public class PingManager {
         });
     }
 
+    private ScheduledExecutorService periodicPingExecutorService;
     private Connection connection;
-    private Thread serverPingThread;
-    private ServerPingTask serverPingTask;
     private int pingInterval = SmackConfiguration.getDefaultPingInterval();
     private Set<PingFailedListener> pingFailedListeners = Collections
             .synchronizedSet(new HashSet<PingFailedListener>());
+    private ScheduledFuture<?> periodicPingTask;
+    protected volatile long lastSuccessfulPingByTask = -1;
+
 
     // Ping Flood protection
     private long pingMinDelta = 100;
     private long lastPingStamp = 0; // timestamp of the last received ping
 
-    // Last server pong timestamp if a ping request manually
+    // Timestamp of the last pong received, either from the server or another entity
-    private long lastServerPingStamp = -1;
+    // Note, no need to synchronize this value, it will only increase over time
+    private long lastSuccessfulManualPing = -1;
 
     private PingManager(Connection connection) {
         ServiceDiscoveryManager sdm = ServiceDiscoveryManager.getInstanceFor(connection);
         sdm.addFeature(NAMESPACE);
         this.connection = connection;
+        init();
+    }
+
+    private void init() {
+        periodicPingExecutorService = new ScheduledThreadPoolExecutor(1);
         PacketFilter pingPacketFilter = new PacketTypeFilter(Ping.class);
-        connection.addPacketListener(new PingPacketListener(), pingPacketFilter);
+        connection.addPacketListener(new PacketListener() {
-        connection.addConnectionListener(new PingConnectionListener());
+            /**
+             * Sends a Pong for every Ping
+             */
+            public void processPacket(Packet packet) {
+                if (pingMinDelta > 0) {
+                    // Ping flood protection enabled
+                    long currentMillies = System.currentTimeMillis();
+                    long delta = currentMillies - lastPingStamp;
+                    lastPingStamp = currentMillies;
+                    if (delta < pingMinDelta) {
+                        return;
+                    }
+                }
+                Pong pong = new Pong((Ping)packet);
+                connection.sendPacket(pong);
+            }
+        }
+        , pingPacketFilter);
+        connection.addConnectionListener(new ConnectionListener() {
+
+            @Override
+            public void connectionClosed() {
+                maybeStopPingServerTask();
+            }
+
+            @Override
+            public void connectionClosedOnError(Exception arg0) {
+                maybeStopPingServerTask();
+            }
+
+            @Override
+            public void reconnectionSuccessful() {
+                maybeSchedulePingServerTask();
+            }
+
+            @Override
+            public void reconnectingIn(int seconds) {
+            }
+
+            @Override
+            public void reconnectionFailed(Exception e) {
+            }
+        });
         instances.put(connection, this);
-        maybeStartPingServerTask();
+        maybeSchedulePingServerTask();
     }
 
     public static PingManager getInstanceFor(Connection connection) {
@@ -103,9 +158,6 @@ public class PingManager {
 
     public void setPingIntervall(int pingIntervall) {
         this.pingInterval = pingIntervall;
-        if (serverPingTask != null) {
-            serverPingTask.setPingInterval(pingIntervall);
-        }
     }
 
     public int getPingIntervall() {
@@ -190,10 +242,10 @@ public class PingManager {
     public boolean pingEntity(String jid, long pingTimeout) {
         IQ result = ping(jid, pingTimeout);
 
-        if (result == null 
+        if (result == null || result.getType() == IQ.Type.ERROR) {
-                || result.getType() == IQ.Type.ERROR) {
             return false;
         }
+        pongReceived();
         return true;
     }
     
@@ -218,7 +270,8 @@ public class PingManager {
             }
             return false;
         }
-        lastServerPingStamp = System.currentTimeMillis();
+        // Maybe not really a pong, but an answer is an answer
+        pongReceived();
         return true;
     }
     
@@ -257,80 +310,34 @@ public class PingManager {
      * @return
      */
     public long getLastSuccessfulPing() {
-        long taskLastSuccessfulPing = -1;
+        return Math.max(lastSuccessfulPingByTask, lastSuccessfulManualPing);
-        if (serverPingTask != null) {
-            taskLastSuccessfulPing = serverPingTask.getLastSucessfulPing();
-        }
-        return Math.max(taskLastSuccessfulPing, lastServerPingStamp);
     }
     
     protected Set<PingFailedListener> getPingFailedListeners() {
         return pingFailedListeners;
     }
 
-    private class PingConnectionListener extends AbstractConnectionListener {
+    /**
-
+     * Cancels any existing periodic ping task if there is one and schedules a new ping task if pingInterval is greater
-        @Override
+     * then zero.
-        public void connectionClosed() {
+     * 
+     */
+    protected synchronized void maybeSchedulePingServerTask() {
         maybeStopPingServerTask();
-        }
-
-        @Override
-        public void connectionClosedOnError(Exception arg0) {
-            maybeStopPingServerTask();
-        }
-
-        @Override
-        public void reconnectionSuccessful() {
-            maybeStartPingServerTask();
-        }
-
-    }
-    
-    private void maybeStartPingServerTask() {
-        if (serverPingTask != null) {
-            serverPingTask.setDone();
-            serverPingThread.interrupt();
-            serverPingTask = null;
-            serverPingThread = null;
-        }
-        
         if (pingInterval > 0) {
-            serverPingTask = new ServerPingTask(connection, pingInterval);
+            periodicPingTask = periodicPingExecutorService.schedule(new ServerPingTask(connection), pingInterval,
-            serverPingThread = new Thread(serverPingTask);
+                    TimeUnit.SECONDS);
-            serverPingThread.setDaemon(true);
-            serverPingThread.setName("Smack Ping Server Task (" + connection.getServiceName() + ")");
-            serverPingThread.start();
         }
     }
 
     private void maybeStopPingServerTask() {
-        if (serverPingThread != null) {
+        if (periodicPingTask != null) {
-            serverPingTask.setDone();
+            periodicPingTask.cancel(true);
-            serverPingThread.interrupt();
+            periodicPingTask = null;
         }
     }
 
-    private class PingPacketListener implements PacketListener {
+    private void pongReceived() {
-
+        lastSuccessfulManualPing = System.currentTimeMillis();
-        public PingPacketListener() {
-        }
-        
-        /**
-         * Sends a Pong for every Ping
-         */
-        public void processPacket(Packet packet) {
-            if (pingMinDelta > 0) {
-                // Ping flood protection enabled
-                long currentMillies = System.currentTimeMillis();
-                long delta = currentMillies - lastPingStamp;
-                lastPingStamp = currentMillies;
-                if (delta < pingMinDelta) {
-                    return;
-                }
-            }
-            Pong pong = new Pong((Ping)packet);
-            connection.sendPacket(pong);
-        }
     }
 }

source/org/jivesoftware/smackx/ping/ServerPingTask.java

@@ -1,5 +1,5 @@
 /**
- * Copyright 2012 Florian Schmaus
+ * Copyright 2012-2013 Florian Schmaus
  *
  * All rights reserved. Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,62 +27,39 @@ class ServerPingTask implements Runnable {
     // for objects and we have a new thread here that should hold a strong
     // reference to connection so that it can be GCed.
     private WeakReference<Connection> weakConnection;
-    private int pingInterval;
-    private volatile long lastSuccessfulPing = -1;
 
     private int delta = 1000; // 1 seconds
     private int tries = 3; // 3 tries
 
-    protected ServerPingTask(Connection connection, int pingIntervall) {
+    protected ServerPingTask(Connection connection) {
         this.weakConnection = new WeakReference<Connection>(connection);
-        this.pingInterval = pingIntervall;
-    }
-    
-    protected void setDone() {
-        this.pingInterval = -1;
-    }
-    
-    protected void setPingInterval(int pingIntervall) {
-        this.pingInterval = pingIntervall;
-    }
-    
-    protected int getIntInterval() {
-        return pingInterval;
-    }
-    
-    protected long getLastSucessfulPing() {
-        return lastSuccessfulPing;
     }
 
     public void run() {
-        sleep(60000);
-        
-        outerLoop:
-        while(pingInterval > 0) {
         Connection connection = weakConnection.get();
         if (connection == null) {
             // connection has been collected by GC
             // which means we can stop the thread by breaking the loop
-                break;
+            return;
         }
         if (connection.isAuthenticated()) {
             PingManager pingManager = PingManager.getInstanceFor(connection);
             boolean res = false;
 
-                for(int i = 0; i < tries; i++) {
+            for (int i = 0; i < tries; i++) {
                 if (i != 0) {
                     try {
                         Thread.sleep(delta);
                     } catch (InterruptedException e) {
                         // We received an interrupt
                         // This only happens if we should stop pinging
-                            break outerLoop;
+                        return;
                     }
                 }
                 res = pingManager.pingMyServer();
                 // stop when we receive a pong back
                 if (res) {
-                        lastSuccessfulPing = System.currentTimeMillis();
+                    pingManager.lastSuccessfulPingByTask = System.currentTimeMillis();
                     break;
                 }
             }
@@ -91,30 +68,10 @@ class ServerPingTask implements Runnable {
                 for (PingFailedListener l : pingFailedListeners) {
                     l.pingFailed();
                 }
+            } else {
+                // Ping was successful, wind-up the periodic task again
+                pingManager.maybeSchedulePingServerTask();
             }
         }
-            sleep();
-        }
-    }
-    
-    /*
-     * If pingInterval > 0 sleeps a minimum of pingInterval
-     */
-    private void sleep(int extraSleepTime) {
-        int totalSleep = pingInterval + extraSleepTime;
-        if (totalSleep > 0) {
-            try {
-                Thread.sleep(totalSleep);
-            } catch (InterruptedException e) {
-                /* Ignore */
-            }
-        }
-    }
-    
-    /**
-     * Sleeps the amount of pingInterval
-     */
-    private void sleep() {
-        sleep(0);
     }
 }