ThreadPool: make sure no leaking threads are left behind in case of initialization failure

[javanna]

Our ThreadPool constructor creates a couple of threads (scheduler and timer) which might not get shut down if the initialization of a node fails. A guice error might occur for example, which causes the InternalNode constructor to throw an exception. In this case the two threads are left behind, which is not a big problem when running es standalone as the error will be intercepted and the jvm will be stopped as a whole. It can become more of a problem though when running es in embedded mode, as we'll end up with lingering threads.

The steps to reproduce are a bit exotic: create a tribe node, and make sure one of its inner client nodes initialization fails. Whether the threads are left behind or not depends on when the failure happens, whether it happens before or after the ThreadPool has been initialized.

    @Test
    public void testThreadPoolLeakingThreadsWithTribeNode() {
        Settings settings = ImmutableSettings.builder()
                .put("node.name", "thread_pool_leaking_threads_tribe_node")
                .put("tribe.t1.cluster.name", "non_existing_cluster")
                 //trigger initialization failure of one of the tribes (doesn't require starting the node)
                .put("tribe.t1.plugin.mandatory", "non_existing").build();

        try {
            NodeBuilder.nodeBuilder().settings(settings).build();
        } catch(Throwable t) {
            //all good
            assertThat(t.getMessage(), containsString("mandatory plugins [non_existing]"));
        }
    }

I think creating threads on the constructor is quite bad already, even worse since we haven't started the node yet. In general I would love to have more lightweight constructors, especially since we use guice. I think we should delay the threads creation to the actual start phase of the node, but this requires quite some changes as we currently schedule executions (threadPool.schedule) from different objects constructors, which need the thread pool scheduler to be already initialized.

[jpountz]

+1 on not starting threads in the constructor. So we would need to have a start() method and start threads in this start method, is it the idea you had in mind @javanna ?

I'm making it an adoptme.

[javanna]

So we would need to have a start() method and start threads in this start method

Yes this is what I had in mind and I gave it a quick try but found various other constructors that call threadPool.schedule() hence they rely on the scheduler thread to be already initialized. All those schedule calls would need to be delayed as well, not sure if this would be it for this change though :)

[jpountz]

Then maybe the ThreadPool creation should be moved outside of the hands of guice? For instance we could create it ourselves, then tell guice to use the provided instance for its injection, and finally close it if the guice injection failed?

[javanna]

sounds good to me!