Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore ability to make OkHttp's Dispatcher use daemon threads #4797

Closed
rdesgroppes opened this issue Jan 27, 2023 · 2 comments · Fixed by #4919
Closed

Restore ability to make OkHttp's Dispatcher use daemon threads #4797

rdesgroppes opened this issue Jan 27, 2023 · 2 comments · Fixed by #4919
Assignees
Milestone

Comments

@rdesgroppes
Copy link

rdesgroppes commented Jan 27, 2023

Is your enhancement related to a problem? Please describe

Up to version 6.3.1, it was possible to pass a custom ExecutorService to OkHttp client's Dispatcher, thanks to OkHttpClientFactory:

  /**
   * Subclasses may use this to apply additional configuration after the Config has been applied
   * This method is only called for clients constructed using the Config.
   *
   * @param builder
   */
  protected void additionalConfig(OkHttpClient.Builder builder) {
    // no default implementation
  }

... which used to allow, for instance, to make sure the thread pool spawns daemon threads:

            @Override
            protected void additionalConfig(final Builder builder) {
                builder.dispatcher(new Dispatcher(Executors.newCachedThreadPool(
                    new ThreadFactoryBuilder().setDaemon(true).setNameFormat("k8s-client-%d").build())));
            }

Since version 6.4.0, due to "Test/simultaneous HttpClient connections" (#4687), the default Dispatcher creation happens after additionalConfig, thus disregarding any user-provided ExecutorService.

Describe the solution you'd like

To get the best of both worlds, i.e. both the default Dispatcher configuration from #4687 and a daemon thread pool:

  1. either always set a cached thread pool spawning daemon threads when configuring the default Dispatcher (assuming this forms a consensus, of course),
  2. or allow to pass a custom ExecutorService at some stage that would be later used when configuring the default Dispatcher, for instance leveraging existing KubernetesClientBuilder.withTaskExecutor[Supplier] (probably a good option),
  3. or mutate the builder's Dispatcher in place (obtained through builder.dispatcher()),
  4. or invoke additionalConfig after having configured the default Dispatcher. In this case users would have to replicate any default configuration by themselves (through [get|set]IdleCallback, [get|set]MaxRequests and [get|set]MaxRequestsPerHost as of now).

Describe alternatives you've considered

Something as arguable as:

    private static KubernetesClient replaceOkHttpThreadFactory(final KubernetesClient client) {
        final var httpClient = (OkHttpClientImpl) client.getHttpClient();
        final var pool = (ThreadPoolExecutor) httpClient.getOkHttpClient().dispatcher().executorService();
        pool.setThreadFactory(new ThreadFactoryBuilder().setDaemon(true).setNameFormat("k8s-client-%d").build());
        return client;
    }

Additional context

No response

@shawkins
Copy link
Contributor

@manusa this is related to the change that moved the dispatcher configuration from the factory to the okhttp client builder. I'm fine with 1, or we can make the Dispatcher a first class member of the factory with a method like createDispatcher so that this behavior can easily be overloaded.

@rdesgroppes rdesgroppes changed the title Restore ability to configure OkHttp's ExecutorService (Dispatcher) Restore ability to make OkHttp's Dispatcher use daemon threads Feb 7, 2023
@shawkins shawkins added this to the 6.5.0 milestone Feb 20, 2023
@manusa
Copy link
Member

manusa commented Feb 27, 2023

or we can make the Dispatcher a first class member of the factory with a method like createDispatcher so that this behavior can easily be overloaded.

I'll provide a fix based on this since there might be users that want further customization regardless of whatever we agree to default the Dispatcher to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants