Thread count increased after usage of ScheduledDataLoaderRegistry

[Ruch12]

Describe the bug
We've merged the latest version after validating the fix was deployed - #135 yet the thread count has started to increase significantly. The threads aren't getting closed after the completion of the task unlike DataLoaderRegistry.

We've initialized ScheduledDataLoaderRegistry in the following way in the ServiceContextImpl -

    public ScheduledDataLoaderRegistry getScheduledDataLoaderRegistry() {

        if (null == scheduledDataLoaderRegistry) {
            DispatchPredicate dispatchPredicate = DispatchPredicate.dispatchIfLongerThan(Duration.ofMillis(50));
            scheduledDataLoaderRegistry = ScheduledDataLoaderRegistry.newScheduledRegistry()
                    .registerAll(dataLoaderRegistry)
                    .dispatchPredicate(dispatchPredicate)
                    .build();
        }

        return scheduledDataLoaderRegistry;
    }

The issue is that the threads are not getting closed after the execution of the scheduledDataLoaderRegistry and when the response is ready and sent back.

In this case, shouldn't the logic be handled in a way when there is nothing to be dispatched and all data loaders have completed their work, we should close the thread?

[bbakerman]

The ScheduledDataLoaderRegistry works in two ways regarding threads or more specifically the ScheduledExecutorService that houses the threads.

You can specify your own executor when you build the ScheduledDataLoaderRegistry as you have shown above or you can use a default ScheduledExecutorService.

If you use the default you must call close on the ScheduledDataLoaderRegistry at the end of the request.

    public void close() {
        closed = true;
        if (defaultExecutorUsed) {
            scheduledExecutorService.shutdown();
        }
    }

In many cases its better to provide your own "shared" ScheduledExecutorService since it wont be created per request.

In this case, shouldn't the logic be handled in a way when there is nothing to be dispatched and all data loaders have completed their work, we should close the thread?

No not really since we cant know when you are finished work... well not truly finished. This is responsibility the using code must take on. You need to call close() on it

[Ruch12]

Yes, which is what I tried it yesterday of closing the scheduledExecutorService but yet was leading to increase in thread count. I wanted to use the default executor only -

public ScheduledDataLoaderRegistry getScheduledDataLoaderRegistry() {

        if (null == scheduledDataLoaderRegistry) {
            DispatchPredicate dispatchPredicate = DispatchPredicate.dispatchIfLongerThan(Duration.ofMillis(50));
            scheduledDataLoaderRegistry = ScheduledDataLoaderRegistry.newScheduledRegistry()
                    .registerAll(dataLoaderRegistry)
                    .dispatchPredicate(dispatchPredicate)
                    .build();
        }

        return scheduledDataLoaderRegistry;
    }

    /**
     * Experimental change which if doesn't close the threads, will be reverted.
     */
    public void areAllDispatchesComplete(ScheduledDataLoaderRegistry scheduledDataLoaderRegistry) {
        boolean allComplete = true;

        for (DataLoader<?, ?> dataLoader : scheduledDataLoaderRegistry.getDataLoaders()) {
            CompletableFuture<? extends List<?>> dispatchFuture = dataLoader.dispatch();

            if (dispatchFuture.isDone()) {
                try {
                    List<?> futureResults = dispatchFuture.get();

                    for (Object future : futureResults) {
                        if (future instanceof CompletableFuture<?> individualFuture) {
                            if (!individualFuture.isDone()) {
                                allComplete = false;
                                break;
                            }
                        }
                    }
                } catch (InterruptedException | ExecutionException e) {
                    allComplete = false;
                }
            } else {
                allComplete = false;
                break;
            }
        }

        if (allComplete) {
            scheduledDataLoaderRegistry.close();
        }
    }

And now, I'm invoking from my dataloaders function - areAllDispatchesComplete before returning the promise. The thread count is still increasing