Hello,
I’ve started using Smack v4.0.0-rc1 via maven and I’m running into the following issue:
When I disconnect the XMPPTCPConnection i’ve been using, the “Smack Listener Processor (0)” thread keeps running. That means that my application cannot gracefully quit.
My test scenario:
Connect
Login
Disconnect
What can I do about this?
It’s a deamon thread that should not prevent your application from quiting gracefully. Could you elaborate that a bit more?
But you are right that the ExecutorService of the “Smack Listener Processor” is leaking. I think this can be prevented by using the dreaded finalize() method. SMACK-567
Proposed fix: https://github.com/Flowdalic/Smack/commit/7ed9128593d4a79ab690aa83cdc0adbf49a445 cd
I am probably missing something here, but why not just call the listenerExecutor.shutdownNow(); on disconnect()?
Because then you need to rather “complex” logic to re-created the listenerExecutor when we re-connect. Instead we simply keep it running for the whole lifetime of the XMPPConnection instance.
How long before I can expect this fix to be included in the version accessible via maven?
When it’s done. You can always grab the source and build/deploy a snapshot for yourself. You don’t have to wait for an offical deployment.
But 4.0.0-rc2 should happen in the near future.
Ok, I tried to import the source using Gradle (in Eclipse) but I ran into the following error. Does that sound familiar?
https://gist.github.com/dirkmoors/ed86c6bed9110a5e7129
Would you be able to push the latest git version to the Sonatype snapshot repo instead? (4.0.0-rc2-SNAPSHOT)
Thanks in advance
Just some thoughts:
If “The thread(s) of the executor have a reference to their ExecutorService which prevents the ExecutorService from being gc’ed” and the Connection has a reference to the ExecutorService, is the finalize method really called? My thinking is, that the Connection cannot be gc’ed as well in that case, because it’s indirectly referenced by the running thread.
Maybe solveable by having a disconnect() method for the user and kind of a disconnectedByException() method for the reconnection. The former shuts down the executor, the latter not.
You need the ‘git’ binary in your PATH when building Smack.
Hi, I’ve been able to import the Gradle project, however, I’m now running into the following exception:
SEVERE: Could not determine Smack version
java.lang.NullPointerException
at org.jivesoftware.smack.SmackConfiguration.(SmackConfiguration.java:100)
at org.jivesoftware.smack.ConnectionConfiguration.(ConnectionConfiguration.j ava:65)
Any idea what i’m missing?
If “The thread(s) of the executor have a reference to their ExecutorService which prevents the ExecutorService from being gc’ed” and the Connection has a reference to the ExecutorService, is the finalize method really called?
That is the important question. If there is nothing preventing the XMPPConnection instance from being gc’ed, then finalized will eventually get called.
My thinking is, that the Connection cannot be gc’ed as well in that case, because it’s indirectly referenced by the running thread.
I don’t see such a reference. Where does a Thread hold a reference to a XMPPConnection instance? How is it “indirectly” referenced? Could you elaborate that?
You need to run ‘gradle assemble’ at least once so that the version resource is created.
Hello, after going through your code in the XMPPConnection I was thinking of the following; would it be an option to add the needed ExecutorServices as optional constructor parameters, which -if not provided- default to the current implementation? That would allow for outsourcing the ExecutorService management.
Could you elaborate how “outsourcing” the executorService management is beneficial? What is the advantage? After all, as library user you should be happy if the library abstracts as much work as possible from you.
How do you determine when a executor is no longer needed?
My thinking was:
If the Executor cannot be GCed, the Connection cannot be GCed either because it holds a reference to the Executor.
But now that I rethink that, there’s a flaw in this logic… sorry. finalize should work then.
With outsourcing I mean something like this:
public class MainClass{
public void static main(String[] args){
ExecutorService testExecutorService = Executors.newFixedThreadPool(10);
XMPPConnection connection = new XMPPTCPConnection(config, testExecutorService, testExecutorService);
connection.connect();
…
…
connection.disconnect();
testExecutorService.shutdownNow();
}
}
…
public class XMPPConnection{
//Current constructor
protected XMPPConnection(ConnectionConfiguration configuration) {
this(configuration,
new ScheduledThreadPoolExecutor(2),
Executors.newSingleThreadExecutor(new ThreadFactory() {
public Thread newThread(Runnable runnable) {
Thread thread = new Thread(runnable,
“Smack Listener Processor (” + connectionCounterValue + “)”);
thread.setDaemon(true);
return thread;
}
});
}
//Proposed constructor
protected XMPPConnection(ConnectionConfiguration configuration, ExecutorService executorService , ExecutorService listenerExecutor) {
config = configuration;
this.executorService = executorService;
this.listenerExecutor = listenerExecutor;
}
}
This way I make my main process responsible for managing the threads if I want to, and regular coders could just stick with the default implemenation (Overloaded constructor).
This way, If I’m sure an executor is no longer needed (e.g. on clean application shutdown) I can manually shutdown the executor.
As I come to think about it, you cóuld always create the executors on connect, and shutdown the executors on disconnect…then you wouldn’t need to expose the executorservices. Or would that cause unwanted behaviour?
Thanks for having a second look. It’s always good if someone does this.
I consider the current solution cleaner. I also still don’t understand why you need to shutdown an executor that has only one daemon thread. Furthermore, with SMACK-567 fixed, you don’t have to care at all.
As it is, your proposed solution does just introduce unnecessary LOC (and therefore complexity), which I’d like to avoid.
The current problem is that the finalize method is never triggered. Even if I set all the references to the XMPPConnection object to null, the thread just keeps running. Only if I quit the application in an unclean fashion (e.g. System.exit(1)), the app stops. A clean solution would be to gracefully stop the thread(s) that are running.
Especially on Android, this would cause problems, since it would keep the app running in the background forever, even if I’ve disconnected.
Would you be okay with providing a accessors like this:
public class XMPPConnection{
protected ExecutorService getMainExecutorService(){
return this.executorService;
}
protected void setMainExecutorService(ExecutorService e){
this.executorService = e;
}
protected ExecutorService getListenerExecutorService(){
return this.listenerService;
}
protected void setListenerExecutorService(ExecutorService e){
this.listenerService = e;
}
}
That would allow me to subclass the XMPPConnection and implement that functionality myself.
I understand your concern on keeping the interface clean, but I’m only looking for a solution where disconnecting my xmppConnection would cause every related thread to gracefully stop. Again, I would like to use this in an Android App, and I’ve had a lot of issues in the past with “zombie threads” that keep running and make my app unable to close cleanly.