diff --git a/server/src/main/java/org/eclipse/openvsx/storage/AwsStorageService.java b/server/src/main/java/org/eclipse/openvsx/storage/AwsStorageService.java index 8a9884d56..0ba4db400 100644 --- a/server/src/main/java/org/eclipse/openvsx/storage/AwsStorageService.java +++ b/server/src/main/java/org/eclipse/openvsx/storage/AwsStorageService.java @@ -7,9 +7,10 @@ * * SPDX-License-Identifier: EPL-2.0 ********************************************************************************/ - package org.eclipse.openvsx.storage; +import jakarta.annotation.Nullable; +import jakarta.annotation.PreDestroy; import org.apache.commons.lang3.StringUtils; import org.eclipse.openvsx.cache.FilesCacheKeyGenerator; import org.eclipse.openvsx.entities.FileResource; @@ -76,45 +77,75 @@ public class AwsStorageService implements IStorageService { @Value("${ovsx.storage.aws.path-style-access:false}") boolean pathStyleAccess; - private S3Client s3Client; + private volatile S3Client s3Client; + private volatile S3Presigner s3Presigner; public AwsStorageService(FileCacheDurationConfig fileCacheDurationConfig, FilesCacheKeyGenerator filesCacheKeyGenerator) { this.fileCacheDurationConfig = fileCacheDurationConfig; this.filesCacheKeyGenerator = filesCacheKeyGenerator; } - public S3Client getS3Client() { - if (s3Client == null) { - var s3ClientBuilder = S3Client.builder() - .defaultsMode(DefaultsMode.STANDARD) - .forcePathStyle(pathStyleAccess) - .region(Region.of(region)) - .credentialsProvider(getCredentialsProvider()); - - if(StringUtils.isNotEmpty(serviceEndpoint)) { - var endpointParams = S3EndpointParams.builder() - .endpoint(serviceEndpoint) - .region(Region.of(region)) - .build(); + @PreDestroy + public void close() { + if (s3Presigner != null) { + try { + s3Presigner.close(); + } catch (RuntimeException _) {} + } - var endpoint = S3EndpointProvider - .defaultProvider() - .resolveEndpoint(endpointParams).join(); + if (s3Client != null) { + try { + s3Client.close(); + } catch (RuntimeException _) {} + } + } - s3ClientBuilder = s3ClientBuilder.endpointOverride(endpoint.url()); + public S3Client getS3Client() { + // lazily generates the s3 client using double-checked locking + if (s3Client == null) { + synchronized (this) { + if (s3Client == null) { + var s3ClientBuilder = S3Client.builder() + .defaultsMode(DefaultsMode.STANDARD) + .forcePathStyle(pathStyleAccess) + .region(Region.of(region)) + .credentialsProvider(getCredentialsProvider()); + + var serviceEndpoint = getServiceEndpoint(); + if (serviceEndpoint != null) { + s3ClientBuilder = s3ClientBuilder.endpointOverride(serviceEndpoint); + } + + s3Client = s3ClientBuilder.build(); + } } - - s3Client = s3ClientBuilder.build(); } return s3Client; } private S3Presigner getS3Presigner() { - var builder = S3Presigner.builder() - .region(Region.of(region)) - .credentialsProvider(getCredentialsProvider()); + // lazily generates the s3 presigner using double-checked locking + if (s3Presigner == null) { + synchronized (this) { + if (s3Presigner == null) { + var builder = S3Presigner.builder() + .region(Region.of(region)) + .credentialsProvider(getCredentialsProvider()); + + var serviceEndpoint = getServiceEndpoint(); + if (serviceEndpoint != null) { + builder = builder.endpointOverride(serviceEndpoint); + } + + s3Presigner = builder.build(); + } + } + } + return s3Presigner; + } - if(StringUtils.isNotEmpty(serviceEndpoint)) { + private @Nullable URI getServiceEndpoint() { + if (StringUtils.isNotEmpty(serviceEndpoint)) { var endpointParams = S3EndpointParams.builder() .endpoint(serviceEndpoint) .region(Region.of(region)) @@ -124,10 +155,10 @@ private S3Presigner getS3Presigner() { .defaultProvider() .resolveEndpoint(endpointParams).join(); - builder = builder.endpointOverride(endpoint.url()); + return endpoint.url(); } - return builder.build(); + return null; } private AwsCredentialsProvider getCredentialsProvider() { @@ -141,7 +172,6 @@ private AwsCredentialsProvider getCredentialsProvider() { return DefaultCredentialsProvider.create(); } - private boolean hasStaticCredentials() { return !StringUtils.isEmpty(accessKeyId) && !StringUtils.isEmpty(secretAccessKey); } @@ -149,6 +179,7 @@ private boolean hasStaticCredentials() { private boolean hasSessionToken() { return !StringUtils.isEmpty(sessionToken); } + @Override public boolean isEnabled() { // Require region and bucket to be configured @@ -240,10 +271,8 @@ private URI getLocation(String objectKey) { .getObjectRequest(objectRequest) .build(); - try (var presigner = getS3Presigner()) { - var presignedRequest = presigner.presignGetObject(presignRequest); - return presignedRequest.httpRequest().getUri(); - } + var presignedRequest = getS3Presigner().presignGetObject(presignRequest); + return presignedRequest.httpRequest().getUri(); } @Override diff --git a/server/src/test/java/org/eclipse/openvsx/storage/AwsStorageServiceTest.java b/server/src/test/java/org/eclipse/openvsx/storage/AwsStorageServiceTest.java index 966663550..d79e61916 100644 --- a/server/src/test/java/org/eclipse/openvsx/storage/AwsStorageServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/storage/AwsStorageServiceTest.java @@ -253,4 +253,25 @@ void testAuthenticationMethodDetection() { assertFalse((Boolean) ReflectionTestUtils.invokeMethod(storageService, "hasStaticCredentials")); assertFalse((Boolean) ReflectionTestUtils.invokeMethod(storageService, "hasSessionToken")); } + + @Test + void testPresignerIsCachedAsSingleton() { + // Only region is needed to construct the presigner builder + ReflectionTestUtils.setField(storageService, "region", "us-east-1"); + ReflectionTestUtils.setField(storageService, "serviceEndpoint", ""); + + var presigner1 = ReflectionTestUtils.invokeMethod(storageService, "getS3Presigner"); + var presigner2 = ReflectionTestUtils.invokeMethod(storageService, "getS3Presigner"); + + assertNotNull(presigner1); + assertSame(presigner1, presigner2, + "S3Presigner must be cached as a singleton; recreating it on each call " + + "destroys the HTTP connection pool needed for IRSA credential refresh"); + } + + @Test + void testPresignerFieldInitiallyNull() { + assertNull(ReflectionTestUtils.getField(storageService, "s3Presigner"), + "S3Presigner should be lazily initialized"); + } }