From 09425609af005e102475621e9a6f4ca0cb70d3f0 Mon Sep 17 00:00:00 2001
From: Florian Schmaus <flo@geekplace.eu>
Date: Wed, 2 Jul 2014 00:17:04 +0200
Subject: [PATCH] Refactor FileTransfer(Manager|Negotiator)

to use WeakHashMaps and extend Manager.

SMACK-579
---
 documentation/extensions/filetransfer.html    | 14 ++-
 .../java/org/jivesoftware/smack/Manager.java  |  3 +
 .../filetransfer/FaultTolerantNegotiator.java |  3 -
 .../filetransfer/FileTransferManager.java     | 76 +++++++---------
 .../filetransfer/FileTransferNegotiator.java  | 90 ++++++-------------
 .../filetransfer/IBBTransferNegotiator.java   |  3 -
 .../Socks5TransferNegotiator.java             |  5 --
 .../smackx/filetransfer/StreamNegotiator.java |  5 --
 8 files changed, 66 insertions(+), 133 deletions(-)

diff --git a/documentation/extensions/filetransfer.html b/documentation/extensions/filetransfer.html
index f71fcc5df..ce84bd79d 100644
--- a/documentation/extensions/filetransfer.html
+++ b/documentation/extensions/filetransfer.html
@@ -33,9 +33,8 @@ to enable the user to easily send a file.
 
 <b>Usage</b><p>
 
-In order to send a file you must first construct an instance of the <b><i>FileTransferManager</i></b>
-class. This class has one constructor with one parameter which is your XMPPConnection.
-In order to instantiate the manager you should call <i>new FileTransferManager(connection)</i>
+In order to send a file you must first construct an instance of the <b><i>FileTransferManager</i></b> class.
+In order to instantiate the manager you should call <i>FileTransferManager.getInstanceFor(connection)</i>
 
 <p>Once you have your <b><i>FileTransferManager</i></b> you will need to create an outgoing
 file transfer to send a file. The method to use on the <b><i>FileTransferManager</i></b>
@@ -62,7 +61,7 @@ In this example we can see how to send a file: <br>
 <blockquote>
 <pre>
       <font color="#3f7f5f">// Create the file transfer manager</font>
-      FileTransferManager manager = new FileTransferManager(connection);
+      FileTransferManager manager = FileTransferManager.getInstanceFor(connection);
 		
       <font color="#3f7f5f">// Create the outgoing file transfer</font>
       OutgoingFileTransfer transfer = manager.createOutgoingFileTransfer(<font color="#0000FF">"romeo@montague.net"</font>);
@@ -85,9 +84,8 @@ manager.</p>
 
 <b>Usage</b><p>
 
-In order to recieve a file you must first construct an instance of the <b><i>FileTransferManager</i></b>
-class. This class has one constructor with one parameter which is your XMPPConnection.
-In order to instantiate the manager you should call <i>new FileTransferManager(connection)</i>
+In order to recieve a file you must first construct an instance of the <b><i>FileTransferManager</i></b> class.
+In order to instantiate the manager you should call <i>FileTransferManager.getInstanceFor(connection)</i>
 
 <p>Once you have your <b><i>FileTransferManager</i></b> you will need to register a listener
 with it. The FileTransferListner interface has one method, <b>fileTransferRequest(request)</b>.
@@ -114,7 +112,7 @@ consult the Javadoc for more information.
 In this example we can see how to approve or reject a file transfer request: <br>
 <blockquote>
 <pre>      <font color="#3f7f5f">// Create the file transfer manager</font>
-      final FileTransferManager manager = new FileTransferManager(connection);
+      FileTransferManager manager = FileTransferManager.getInstanceFor(connection);
 
       <font color="#3f7f5f">// Create the listener</font>
       manager.addFileTransferListener(new FileTransferListener() {
diff --git a/smack-core/src/main/java/org/jivesoftware/smack/Manager.java b/smack-core/src/main/java/org/jivesoftware/smack/Manager.java
index 0b6d1315d..a999d36a9 100644
--- a/smack-core/src/main/java/org/jivesoftware/smack/Manager.java
+++ b/smack-core/src/main/java/org/jivesoftware/smack/Manager.java
@@ -23,6 +23,9 @@ public abstract class Manager {
     final WeakReference<XMPPConnection> weakConnection;
 
     public Manager(XMPPConnection connection) {
+        if (connection == null) {
+            throw new IllegalArgumentException("XMPPConnection must not be null");
+        }
         weakConnection = new WeakReference<XMPPConnection>(connection);
     }
 
diff --git a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FaultTolerantNegotiator.java b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FaultTolerantNegotiator.java
index 1274afc57..41e701092 100644
--- a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FaultTolerantNegotiator.java
+++ b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FaultTolerantNegotiator.java
@@ -161,9 +161,6 @@ public class FaultTolerantNegotiator extends StreamNegotiator {
         return namespaces;
     }
 
-    public void cleanup() {
-    }
-
     private class NegotiatorService implements Callable<InputStream> {
 
         private PacketCollector collector;
diff --git a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FileTransferManager.java b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FileTransferManager.java
index 10ea4e93a..755215728 100644
--- a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FileTransferManager.java
+++ b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FileTransferManager.java
@@ -16,6 +16,7 @@
  */
 package org.jivesoftware.smackx.filetransfer;
 
+import org.jivesoftware.smack.Manager;
 import org.jivesoftware.smack.PacketListener;
 import org.jivesoftware.smack.SmackException.NotConnectedException;
 import org.jivesoftware.smack.XMPPConnection;
@@ -28,8 +29,10 @@ import org.jivesoftware.smack.packet.XMPPError;
 import org.jivesoftware.smackx.si.packet.StreamInitiation;
 import org.jxmpp.util.XmppStringUtils;
 
-import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 /**
  * The file transfer manager class handles the sending and recieving of files.
@@ -43,13 +46,22 @@ import java.util.List;
  * @author Alexander Wenckus
  * 
  */
-public class FileTransferManager {
+public class FileTransferManager extends Manager {
+
+    private static final Map<XMPPConnection, FileTransferManager> INSTANCES = new WeakHashMap<XMPPConnection, FileTransferManager>();
+
+    public static synchronized FileTransferManager getInstanceFor(XMPPConnection connection) {
+        FileTransferManager fileTransferManager = INSTANCES.get(connection);
+        if (fileTransferManager == null) {
+            fileTransferManager = new FileTransferManager(connection);
+            INSTANCES.put(connection, fileTransferManager);
+        }
+        return fileTransferManager;
+    }
 
 	private final FileTransferNegotiator fileTransferNegotiator;
 
-	private List<FileTransferListener> listeners;
-
-	private XMPPConnection connection;
+	private final List<FileTransferListener> listeners = new CopyOnWriteArrayList<FileTransferListener>();
 
 	/**
 	 * Creates a file transfer manager to initiate and receive file transfers.
@@ -57,10 +69,19 @@ public class FileTransferManager {
 	 * @param connection
 	 *            The XMPPConnection that the file transfers will use.
 	 */
-	public FileTransferManager(XMPPConnection connection) {
-		this.connection = connection;
+	private FileTransferManager(XMPPConnection connection) {
+		super(connection);
 		this.fileTransferNegotiator = FileTransferNegotiator
 				.getInstanceFor(connection);
+        connection.addPacketListener(new PacketListener() {
+            public void processPacket(Packet packet) {
+                StreamInitiation si = (StreamInitiation) packet;
+                FileTransferRequest request = new FileTransferRequest(FileTransferManager.this, si);
+                for (FileTransferListener listener : listeners) {
+                    listener.fileTransferRequest(request);
+                }
+            }
+        }, new AndFilter(new PacketTypeFilter(StreamInitiation.class), IQTypeFilter.SET));
 	}
 
 	/**
@@ -73,35 +94,7 @@ public class FileTransferManager {
 	 * @see FileTransferListener
 	 */
 	public void addFileTransferListener(final FileTransferListener li) {
-		if (listeners == null) {
-			initListeners();
-		}
-		synchronized (this.listeners) {
-			listeners.add(li);
-		}
-	}
-
-	private void initListeners() {
-		listeners = new ArrayList<FileTransferListener>();
-
-		connection.addPacketListener(new PacketListener() {
-			public void processPacket(Packet packet) {
-				fireNewRequest((StreamInitiation) packet);
-			}
-		}, new AndFilter(new PacketTypeFilter(StreamInitiation.class),
-				IQTypeFilter.SET));
-	}
-
-	protected void fireNewRequest(StreamInitiation initiation) {
-		FileTransferListener[] listeners = null;
-		synchronized (this.listeners) {
-			listeners = new FileTransferListener[this.listeners.size()];
-			this.listeners.toArray(listeners);
-		}
-		FileTransferRequest request = new FileTransferRequest(this, initiation);
-		for (int i = 0; i < listeners.length; i++) {
-			listeners[i].fileTransferRequest(request);
-		}
+		listeners.add(li);
 	}
 
 	/**
@@ -112,12 +105,7 @@ public class FileTransferManager {
 	 * @see FileTransferListener
 	 */
 	public void removeFileTransferListener(final FileTransferListener li) {
-		if (listeners == null) {
-			return;
-		}
-		synchronized (this.listeners) {
-			listeners.remove(li);
-		}
+		listeners.remove(li);
 	}
 
 	/**
@@ -140,7 +128,7 @@ public class FileTransferManager {
             throw new IllegalArgumentException("The provided user id was not a full JID (i.e. with resource part)");
         }
 
-		return new OutgoingFileTransfer(connection.getUser(), userID,
+		return new OutgoingFileTransfer(connection().getUser(), userID,
 				fileTransferNegotiator.getNextStreamID(),
 				fileTransferNegotiator);
 	}
@@ -175,6 +163,6 @@ public class FileTransferManager {
 				initiation.getPacketID(), initiation.getFrom(), initiation
 						.getTo(), IQ.Type.error);
 		rejection.setError(new XMPPError(XMPPError.Condition.no_acceptable));
-		connection.sendPacket(rejection);
+		connection().sendPacket(rejection);
 	}
 }
diff --git a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FileTransferNegotiator.java b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FileTransferNegotiator.java
index fb698391a..604271bb2 100644
--- a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FileTransferNegotiator.java
+++ b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/FileTransferNegotiator.java
@@ -24,9 +24,9 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Random;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.WeakHashMap;
 
-import org.jivesoftware.smack.AbstractConnectionListener;
+import org.jivesoftware.smack.Manager;
 import org.jivesoftware.smack.SmackException.NotConnectedException;
 import org.jivesoftware.smack.XMPPConnection;
 import org.jivesoftware.smack.PacketCollector;
@@ -50,16 +50,13 @@ import org.jivesoftware.smackx.xdata.packet.DataForm;
  * @author Alexander Wenckus
  * @see <a href="http://xmpp.org/extensions/xep-0096.html">XEP-0096: SI File Transfer</a>
  */
-public class FileTransferNegotiator {
+public class FileTransferNegotiator extends Manager {
 
-    // Static
+    public static final String SI_NAMESPACE = "http://jabber.org/protocol/si";
+    public static final String SI_PROFILE_FILE_TRANSFER_NAMESPACE = "http://jabber.org/protocol/si/profile/file-transfer";
+    private static final String[] NAMESPACE = { SI_NAMESPACE, SI_PROFILE_FILE_TRANSFER_NAMESPACE };
 
-    private static final String[] NAMESPACE = {
-            "http://jabber.org/protocol/si/profile/file-transfer",
-            "http://jabber.org/protocol/si"};
-
-    private static final Map<XMPPConnection, FileTransferNegotiator> transferObject =
-            new ConcurrentHashMap<XMPPConnection, FileTransferNegotiator>();
+    private static final Map<XMPPConnection, FileTransferNegotiator> INSTANCES = new WeakHashMap<XMPPConnection, FileTransferNegotiator>();
 
     private static final String STREAM_INIT_PREFIX = "jsi_";
 
@@ -80,27 +77,16 @@ public class FileTransferNegotiator {
      * service is automatically enabled.
      *
      * @param connection The connection for which the transfer manager is desired
-     * @return The IMFileTransferManager
+     * @return The FileTransferNegotiator
      */
-    public static FileTransferNegotiator getInstanceFor(
+    public static synchronized FileTransferNegotiator getInstanceFor(
             final XMPPConnection connection) {
-        if (connection == null) {
-            throw new IllegalArgumentException("XMPPConnection cannot be null");
-        }
-        if (!connection.isConnected()) {
-            return null;
-        }
-
-        if (transferObject.containsKey(connection)) {
-            return transferObject.get(connection);
-        }
-        else {
-            FileTransferNegotiator transfer = new FileTransferNegotiator(
-                    connection);
-            setServiceEnabled(connection, true);
-            transferObject.put(connection, transfer);
-            return transfer;
+        FileTransferNegotiator fileTransferNegotiator = INSTANCES.get(connection);
+        if (fileTransferNegotiator == null) {
+            fileTransferNegotiator = new FileTransferNegotiator(connection);
+            INSTANCES.put(connection, fileTransferNegotiator);
         }
+        return fileTransferNegotiator;
     }
 
     /**
@@ -110,7 +96,7 @@ public class FileTransferNegotiator {
      * @param connection The connection on which to enable or disable the services.
      * @param isEnabled  True to enable, false to disable.
      */
-    public static void setServiceEnabled(final XMPPConnection connection,
+    private static void setServiceEnabled(final XMPPConnection connection,
             final boolean isEnabled) {
         ServiceDiscoveryManager manager = ServiceDiscoveryManager
                 .getInstanceFor(connection);
@@ -124,14 +110,11 @@ public class FileTransferNegotiator {
 
         for (String namespace : namespaces) {
             if (isEnabled) {
-                if (!manager.includesFeature(namespace)) {
-                    manager.addFeature(namespace);
-                }
+                manager.addFeature(namespace);
             } else {
                 manager.removeFeature(namespace);
             }
         }
-        
     }
 
     /**
@@ -200,38 +183,16 @@ public class FileTransferNegotiator {
 
     // non-static
 
-    private final XMPPConnection connection;
-
     private final StreamNegotiator byteStreamTransferManager;
 
     private final StreamNegotiator inbandTransferManager;
 
     private FileTransferNegotiator(final XMPPConnection connection) {
-        configureConnection(connection);
-
-        this.connection = connection;
+        super(connection);
         byteStreamTransferManager = new Socks5TransferNegotiator(connection);
         inbandTransferManager = new IBBTransferNegotiator(connection);
-    }
 
-    private void configureConnection(final XMPPConnection connection) {
-        connection.addConnectionListener(new AbstractConnectionListener() {
-            @Override
-            public void connectionClosed() {
-                cleanup(connection);
-            }
-
-            @Override
-            public void connectionClosedOnError(Exception e) {
-                cleanup(connection);
-            }
-        });
-    }
-
-    private void cleanup(final XMPPConnection connection) {
-        if (transferObject.remove(connection) != null) {
-            inbandTransferManager.cleanup();
-        }
+        setServiceEnabled(connection, true);
     }
 
     /**
@@ -255,7 +216,7 @@ public class FileTransferNegotiator {
             IQ iqPacket = createIQ(si.getPacketID(), si.getFrom(), si.getTo(),
                     IQ.Type.error);
             iqPacket.setError(error);
-            connection.sendPacket(iqPacket);
+            connection().sendPacket(iqPacket);
             throw new XMPPErrorException(errorMessage, error);
         }
 
@@ -269,7 +230,7 @@ public class FileTransferNegotiator {
             IQ iqPacket = createIQ(si.getPacketID(), si.getFrom(), si.getTo(),
                     IQ.Type.error);
             iqPacket.setError(e.getXMPPError());
-            connection.sendPacket(iqPacket);
+            connection().sendPacket(iqPacket);
             throw e;
         }
 
@@ -308,9 +269,8 @@ public class FileTransferNegotiator {
             throw new XMPPErrorException(error);
         }
 
-       //if (isByteStream && isIBB && field.getType().equals(FormField.TYPE_LIST_MULTI)) {
         if (isByteStream && isIBB) { 
-            return new FaultTolerantNegotiator(connection,
+            return new FaultTolerantNegotiator(connection(),
                     byteStreamTransferManager,
                     inbandTransferManager);
         }
@@ -333,7 +293,7 @@ public class FileTransferNegotiator {
         IQ iqPacket = createIQ(si.getPacketID(), si.getFrom(), si.getTo(),
                 IQ.Type.error);
         iqPacket.setError(error);
-        connection.sendPacket(iqPacket);
+        connection().sendPacket(iqPacket);
     }
 
     /**
@@ -394,11 +354,11 @@ public class FileTransferNegotiator {
 
         si.setFeatureNegotiationForm(createDefaultInitiationForm());
 
-        si.setFrom(connection.getUser());
+        si.setFrom(connection().getUser());
         si.setTo(userID);
         si.setType(IQ.Type.set);
 
-        PacketCollector collector = connection.createPacketCollectorAndSend(si);
+        PacketCollector collector = connection().createPacketCollectorAndSend(si);
         Packet siResponse = collector.nextResult(responseTimeout);
         collector.cancel();
 
@@ -439,7 +399,7 @@ public class FileTransferNegotiator {
         }
 
         if (isByteStream && isIBB) {
-            return new FaultTolerantNegotiator(connection,
+            return new FaultTolerantNegotiator(connection(),
                     byteStreamTransferManager, inbandTransferManager);
         }
         else if (isByteStream) {
diff --git a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/IBBTransferNegotiator.java b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/IBBTransferNegotiator.java
index 5dcaebf3c..22fde9fb4 100644
--- a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/IBBTransferNegotiator.java
+++ b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/IBBTransferNegotiator.java
@@ -106,9 +106,6 @@ public class IBBTransferNegotiator extends StreamNegotiator {
         return session.getInputStream();
     }
 
-    public void cleanup() {
-    }
-
     /**
      * This PacketFilter accepts an incoming In-Band Bytestream open request
      * with a specified session ID.
diff --git a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/Socks5TransferNegotiator.java b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/Socks5TransferNegotiator.java
index bc4bb7440..c57923db6 100644
--- a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/Socks5TransferNegotiator.java
+++ b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/Socks5TransferNegotiator.java
@@ -122,11 +122,6 @@ public class Socks5TransferNegotiator extends StreamNegotiator {
         }
     }
 
-    @Override
-    public void cleanup() {
-        /* do nothing */
-    }
-
     /**
      * This PacketFilter accepts an incoming SOCKS5 Bytestream request with a specified session ID.
      */
diff --git a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/StreamNegotiator.java b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/StreamNegotiator.java
index aade2dbbb..0848f2988 100644
--- a/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/StreamNegotiator.java
+++ b/smack-extensions/src/main/java/org/jivesoftware/smackx/filetransfer/StreamNegotiator.java
@@ -157,9 +157,4 @@ public abstract class StreamNegotiator {
      */
     public abstract String[] getNamespaces();
 
-    /**
-     * Cleanup any and all resources associated with this negotiator.
-     */
-    public abstract void cleanup();
-
 }