set threads to daemon as per #759

[cplee]

No description provided.

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #100 SUCCESS
This pull request looks good

[cloudbees-pull-request-builder]

NetflixOSS » Hystrix » Hystrix-pull-requests #101 SUCCESS
This pull request looks good

[mattrjacobs]

Thanks for the contribution, @cplee . Just to be clear about what this does, this should only change the behavior at JVM shutdown, correct? In current releases, Hystrix threads need to be explicitly killed and your change would allow the JVM to do the killing. Did I summarize that properly?

[cplee]

@mattrjacobs Yes, the only change in behavior should be at JVM shutdown. Per the javadoc, "The Java Virtual Machine exits when the only threads running are all daemon threads."

I suppose a concern may be that on JVM shutdown, these threads would be stopped abruptly. Maybe this behavior should be configurable?

[mattrjacobs]

I only have experience in an environment where we never attempt graceful shutdown, so we're used to threads stopping abruptly. I guess the concern here would be things like I/O writes being in an incomplete state and now getting killed without being allowed to come to a natural end.

I also took a closer look and, in practice, we are using a custom concurrency plugin at Netflix in production to set all threads to be daemon threads already - we just never changed this to be the default in open-source Hystrix.

/cc @benjchristensen Any history on this that you can recall?

[benjchristensen]

I believe there were other things that prevented a clean shutdown which is why the Hystrix.reset() API was added but I can't remember.

It won't hurt anything to have the threads be daemons.

[mattrjacobs]

@benjchristensen Thanks for the confirmation.

@cplee Merging now.