Making the connection nsq-read threads daemon

[TonioGela]

I've noticed using the library that there is a dangling non daemon thread that keeps the app open even after closing all the resources created via the library.

I spotted where the dangling thread gets created and I've managed to make it a daemon thread.

This example is a MRE that hungs with the current lib version. This PR fixes this behaviour and correctly closes the application.

import com.sproutsocial.nsq.*;

public class nsq {
  public static void main(String[] args) throws Exception {
    Publisher publisher = new Publisher("localhost:4150");
    Subscriber subscriber = new Subscriber("localhost:4161");
    publisher.publish("example_topic", "Hello nsq".getBytes());
    subscriber.subscribe("example_topic", "paperino", nsq::handleData);
    publisher.stop();
    subscriber.stop();
  }

  public static void handleData(byte[] data) {
    System.out.println("Received:" + new String(data));
  }
}

[robseed]

Client::stop is how you shut everything down cleanly.

Try replacing

publisher.stop();
subscriber.stop();

with

Client.getDefaultClient().stop()

[TonioGela]

That's more like a workaround, as it requires 30 seconds of waiting, while my PR does a different thing: it prevents any Pub/Sub instance from creating a background thread that's not running as a daemon one.

Also, more in general, how come that I'm required to call stop() on an internal object that is shared among all the Pub/Sub instances and the stop() method on those doesn't automatically close the resource I have a handle of?

[mfirry]

@robseed your suggestion is to always keep a hold on a client instance? So constructing Publisher and Subscriber passing the client or do somethinhg like subscriber.getClient().stop() or the equivalent for the publisher?

[robseed]

@mfirry There is no need to keep track of a client instance or pass it into the constructors, just call Client.getDefaultClient().stop() when you shut down.

[TonioGela]

What if someone doesn't want to use the default client? That said, the PR is not really about that, is about not leaving a dangling non daemon thread. What do you think about it?

[robseed]

The only reasons you might need more than one client are rare and specialized -- perhaps 2 independent teams at your company run 2 independent nsq clusters (say, one for metrics and another for sending email) and they are require different configurations (maybe one uses authentication and the other doesn't)

Even if this PR was merged, under real scenarios you'd still need to call Client::stop to shut down cleanly because there is an Executor running the message handlers and another Executor for delayed operations. Maybe you could make those use thread factories that create daemon threads -- but I'd still prefer one specific method to stop cleanly. One could argue Publisher::stop and Subscriber::stop should not be public to avoid this confusion.

[TonioGela]

but I'd still prefer one specific method to stop cleanly. One could argue Publisher::stop and Subscriber::stop should not be public to avoid this confusion.

This PR is about fixing a non daemon thread in the Connection class, that was clearly meant to be a daemon one. Isn't it worth merging then?

[robseed]

Sure, I guess it can't do any harm, but once again what you should do is call Client::stop