Thread leak from httpClient in HttpProjectConfigManager

[GenadiIvanov]

Hello,

Recently we discovered a thread leak in the HttpProjectConfigManager.java which forgets to override the "close()" method and therefore to close the ClosableHttpClient (https://github.com/optimizely/java-sdk/blob/master/core-httpclient-impl/src/main/java/com/optimizely/ab/config/HttpProjectConfigManager.java#L64).

    private final OptimizelyHttpClient httpClient;
    private final URI uri;
    private final String datafileAccessToken;
    private String datafileLastModified;

    private HttpProjectConfigManager(long period,
                                     TimeUnit timeUnit,
                                     OptimizelyHttpClient httpClient,
                                     String url,
                                     String datafileAccessToken,
                                     long blockingTimeoutPeriod,
                                     TimeUnit blockingTimeoutUnit,
                                     NotificationCenter notificationCenter) {
        super(period, timeUnit, blockingTimeoutPeriod, blockingTimeoutUnit, notificationCenter);
        this.httpClient = httpClient;
        this.uri = URI.create(url);
        this.datafileAccessToken = datafileAccessToken;
    }

If the httpClient is built by the Optimizely code (https://github.com/optimizely/java-sdk/blob/master/core-httpclient-impl/src/main/java/com/optimizely/ab/config/HttpProjectConfigManager.java#L331) then the eviction configuration instructs the Apache httpClient to start a Connection evictor thread which won't be interrupted on shutdown.

if (httpClient == null) {
    httpClient = OptimizelyHttpClient.builder()
        .withEvictIdleConnections(evictConnectionIdleTimePeriod, evictConnectionIdleTimeUnit)
        .build();
}

Currently only the parent PollingProjectConfigManager.java calls "close()" to free up resources (https://github.com/optimizely/java-sdk/blob/master/core-api/src/main/java/com/optimizely/ab/config/PollingProjectConfigManager.java#L210).

public synchronized void close() {
    stop();
    scheduledExecutorService.shutdownNow();
    started = false;
}

Expected behaviour would be to override the "close()" method in HttpProjectConfigManager.java, call "httpClient.close()" and continue the flow to the parent class (PollingProjectConfigManager.java#L210) to close its resources.

[Tamara-Barum]

Thank you for the details. We have created an internal ticket and assigned to our Java engineer to start looking into this in our current Sprint. We will provide an update in the coming days.

[russell-loube-optimizely]

Hi @GenadiIvanov , we released a fix here: #530

I'll go ahead and close this ticket. Please let us know if you're still facing issues and we'll gladly re-open.

[GenadiIvanov]

Thank you for the fast reaction!

When we can expect the version with the fix to be released?

[russell-loube-optimizely]

Hi @GenadiIvanov , I apologize for missing your above reply. We will promote the current beta to GA once we meet our beta exit criteria for our Advanced Audience Targeting functionality. Unfortunately, I don't have a timeline quite yet. My apologies.