aws-sdk-java icon indicating copy to clipboard operation
aws-sdk-java copied to clipboard

Expired connections are not released

Open bitsal opened this issue 4 years ago • 7 comments

I use

AmazonS3ClientBuilder.standard()
            .withClientConfiguration(clientConfiguration())
            .build();

to build AmazonS3 where

private ClientConfiguration clientConfiguration() {
        ConnectionPool connectionPool = awsProperties.getS3().getConnectionPool();

        return PredefinedClientConfigurations.defaultConfig()
            .withMaxConnections(connectionPool.getMaxConnections())
            .withConnectionTTL(connectionPool.getConnectionTtl());
    }

Describe the bug

And connection TTL makes connections (what have memory leak) expired, but no way to turn on the code that releases expired connections from connection pool.

  1. com.amazonaws.http.AmazonHttpClient -> no not @SdkTestInternalApi constructor to pass ConnectionManagerAwareHttpClient OR replace default httpClientFactory
  2. com.amazonaws.http.apache.client.impl.ApacheHttpClientFactory.create() -> no way to customize HttpClientBuilder and set evictExpiredConnections to TRUE

Expected Behavior

I should be able to turn on expired connection eviction.

Current Behavior

Expired connections live forever and when all connections from the pool are leased the pool throws timeout exception upon connection creation.

Steps to Reproduce

  1. Create a connection pool with 10 connections max
  2. Open 10 connections and do not close them
  3. Wait "connectionTTL" time
  4. Try to open 11th connection

Possible Solution

  1. Allow to configure http client factory for AmazonHttpClient.
  2. Allow to configure ApacheHttpClientFactory.create()

Context

It is a high priority for us to prevent production application downtimes in case of developer mistakes in managing S3 objects (close it all the time job code is done even there are errors). We gonna fix memory leaks and will run code

if (evictExpiredConnections || evictIdleConnections) {
                final IdleConnectionEvictor connectionEvictor = new IdleConnectionEvictor(cm,
                        maxIdleTime > 0 ? maxIdleTime : 10, maxIdleTimeUnit != null ? maxIdleTimeUnit : TimeUnit.SECONDS);
                closeablesCopy.add(new Closeable() {

                    @Override
                    public void close() throws IOException {
                        connectionEvictor.shutdown();
                    }

                });
                connectionEvictor.start();
            }

ourselves that is implemented inside org.apache.http.impl.client.HttpClientBuilder.

Your Environment

  • AWS Java SDK version used: 1.11.768
  • JDK version used: 1.8
  • Operating System and version: Linux

bitsal avatar Apr 15 '20 11:04 bitsal

No way to create/start IdleConnectionEvictor ourselves due to no way to have access to connection manager what is created inside. :(

bitsal avatar Apr 16 '20 16:04 bitsal

@bitsal I believe we are also facing a similar issue, In your "Steps to Reproduce" Point #2. when you say do not close connection - what does that mean? you don't return the connection in the pool that you started using it? or after operation completely keep the connection open in the pool to be able to re-use?

Nirmal-zymr avatar Oct 12 '20 04:10 Nirmal-zymr

In case there is a bug and connection/stream is not closed and marked as busy forever. I have fixed the root cause but I would like to prevent similar issues in the future by enabling idle connection evictor.

you don't return the connection in the pool that you started using it

my case

bitsal avatar Oct 12 '20 07:10 bitsal

@bitsal Technically once you checkout connection from the pool, its not fall under the category of idle connection to be evicted, i,e idle connection evictor. The solution to this is to use ClientExcecutionTimeout but that is not working for me due to https://github.com/aws/aws-sdk-java/issues/1776 (S3 and SQS seems using the same pool)

We did create an intermediate layer in application though, which tracks leased connection and forcefully close after timeout if not closed by application to prevent leakage.

Nirmal-zymr avatar Oct 12 '20 09:10 Nirmal-zymr

@Nirmal-zymr I've checked it once again in the code and actually IdleConnectionEvictor has one more thing connectionManager.closeExpiredConnections();. It is what we need I suppose, not idle connections agree.

bitsal avatar Oct 12 '20 22:10 bitsal

@bitsal I'm sorry for the long silence in this thread. To clarify, after your last comment, do you still need us to investigate or did you find the connections were being closed as expected?

debora-ito avatar May 19 '21 23:05 debora-ito

@debora-ito I didn't check new versions/releases, but based on @Nirmal-zymr comment it is still an issue with expired connections.

bitsal avatar May 20 '21 08:05 bitsal