first off, thanks for a great library. However, I noticed a peculiarity in the way PacketListeners are cleaned up on disconnect. Consider the following case:
How to reproduce:
Create a new connection
Add a PacketListener instance
Connect and authenticate on this connection
Disconnect (using Connection.disconnect())
Reconnect using Connection.connect() / Connection.login()
Notice that the PacketListener created at (2) is no longer called
Details:
At (4), Connection.disconnect() calls PacketReader.cleanup(), which contains the following line:
connection.recvListeners.clear();
This means that the PacketListener instance created at (2) is now removed from the connection. Hence, when the connection is restored the PacketListener created at (2) will no longer be called.
Note that it’s odd that PacketReader.cleanup() should perform cleanup on the connection object. Encapsulation would dictate that a) recvListeners should be private to Connection, and b) that recvListeners should be managed by Connection exclusively.
The reason I’m using disconnect/connect is because I’m developing an Android application, where frequent IP changes due to changes in network interfaces (Wi-Fi/2G/3G) are commonplace. Sockets are not guaranteed to close on such transitions, so this should be handled by the application by listenening to events from ConnectivityManager/TelephonyManager.
Possible fix:
Simply move the recvListeners.clear to Connection.shutdown() ?
I don’'t think it is something that can be fixed in a point release as it changes the behaviour of the connection, which may break existing code. There may be some alternative though such as adding a new disconnect method that will not cause the same problem.
I think that simply creating a new disconnect() method is not the right approach. AFAIK many Managers rely on that behavior. This is a fundamental design problem in Smack. The state should be kept.
It seems like the cleanup() for the Packet Reader/Writers was introduced with cs7183 to prevent Listeners/Interceptors from sticking around im Memory. And since Listeners are often anonoymous inner classes of Managers, the whole Manager won’t be gc’ed. cs7183 may be introduced because of SMACK-278, which is now fixed.
In fact, the whole process of reusing a Connection instance after disconnection (be it by Exception or clean) is flawed. For example, the ServiceDiscoveryManager installs a ConnectionClosed Listener that will remove itself from the instances Map, but only if it was a clean disconnect. The PacketListeners will stick around and I am not sure but on a subsequent successful connection will be duplicated by calling ServiceDiscoveryManager.getInstanceFor(), because a new ServiceDiscoveryManager is created whose constructor calls init() resulting in new Packet Listeners. (See also SMACK-383).
PEPManager, on the other side, tries a finalize() approach. But in my understanding, finalize() will never be called, again because of PacketListeners as local inner classes that are added to the connection.
It would be great if some time in the future, maybe the next major release of Smack, this issues could be addressed. Connection disconnect() should keep it’s state and the Manager should be adjusted to be aware of that (i.e. not installing additional packet listeners/interceptors). I think that this would also make the patch of SMACK-383 obsolete (and many other fixes dirty workarounds ). This would be great for Android since the count of object instanciation would be reduced resulting in a lower overhead (and more fun )