Skip to content

Commit da2e018

Browse files
SNOW-3428785: add shutdown to executorService in s3 transfers (#2602)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent aca4e0b commit da2e018

3 files changed

Lines changed: 216 additions & 17 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
- Added IPv6 support for cloud metadata services so Workload Identity Federation and platform detection work on IPv6-only instances (snowflakedb/snowflake-jdbc#2586):
99
- GCP WIF attestation now uses hostname `metadata.google.internal` instead of the IPv4 link-local address.
1010
- EC2 instance detection probes the IPv4 and IPv6 IMDS endpoints (`[fd00:ec2::254]`) in parallel so detection succeeds on IPv6-only instances without doubling the detection budget on dual-stack hosts.
11-
- Added `enableCopyResultSet` connection property (default `false`): when `true`, `Statement.execute()` exposes the COPY INTO per-file metadata result set via `getResultSet()` instead of consuming it internally (snowflakedb/snowflake-jdbc#SNOW-3388627)
11+
- Added `enableCopyResultSet` connection property (default `false`): when `true`, `Statement.execute()` exposes the COPY INTO per-file metadata result set via `getResultSet()` instead of consuming it internally (snowflakedb/snowflake-jdbc#2592)
1212
- Migrated CI test images from CentOS 7 (EOL) to Rocky Linux 8
1313
- Fixed NPE "The URI scheme of endpointOverride must not be null" happening during file transfer (e.g. PUT) in some use-cases (snowflakedb/snowflake-jdbc#2572)
1414
- Fixed connections.toml auto-configuration behaviour (snowflakedb/snowflake-jdbc#2591):
@@ -17,6 +17,7 @@
1717
- Fixed protocol field in connections.toml being ignored, causing connections to always use HTTPS (snowflakedb/snowflake-jdbc#2585)
1818
- Fixed SecurityException on credential cache file ownership check in containers where JVM returns '?' for user.name (snowflakedb/snowflake-jdbc#2600).
1919
- Fixed credential cache delete operations ignoring clientStoreTemporaryCredential=false setting (snowflakedb/snowflake-jdbc#2600).
20+
- Fixed S3 transfer thread pool leak during repeated PUT/GET operations causing possible OOM (snowflakedb/snowflake-jdbc#2602)
2021

2122
- v4.1.0
2223
- Added warning about using plain HTTP OAuth endpoints (snowflakedb/snowflake-jdbc#2556).

src/main/java/net/snowflake/client/internal/jdbc/cloud/storage/SnowflakeS3Client.java

Lines changed: 46 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.util.concurrent.CompletableFuture;
2626
import java.util.concurrent.CompletionException;
2727
import java.util.concurrent.ThreadPoolExecutor;
28+
import java.util.concurrent.TimeUnit;
2829
import net.snowflake.client.api.exception.ErrorCode;
2930
import net.snowflake.client.api.exception.SnowflakeSQLException;
3031
import net.snowflake.client.api.http.HttpHeadersCustomizer;
@@ -82,6 +83,8 @@ public class SnowflakeS3Client implements SnowflakeStorageClient {
8283
private static final String S3_STREAMING_INGEST_CLIENT_NAME = "ingestclientname";
8384
private static final String S3_STREAMING_INGEST_CLIENT_KEY = "ingestclientkey";
8485

86+
private static final int EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS = 5;
87+
8588
// expired AWS token error code
8689
protected static final String EXPIRED_AWS_TOKEN_ERROR_CODE = "ExpiredToken";
8790

@@ -366,21 +369,19 @@ public void download(
366369
String localFilePath = localLocation + localFileSep + destFileName;
367370
logger.debug(
368371
"Starting download of file from S3 stage path: {} to {}", stageFilePath, localFilePath);
369-
S3TransferManager tx = null;
370372
int retryCount = 0;
371373
do {
374+
ThreadPoolExecutor executorService = null;
375+
S3TransferManager tx = null;
372376
try {
373377
File localFile = new File(localFilePath);
374378

375379
logger.debug("Creating executor service for transfer manager with {} threads", parallelism);
376380

381+
executorService =
382+
createDefaultExecutorService("s3-transfer-manager-downloader-", parallelism);
377383
// download files from s3
378-
tx =
379-
S3TransferManager.builder()
380-
.s3Client(amazonClient)
381-
.executor(
382-
createDefaultExecutorService("s3-transfer-manager-downloader-", parallelism))
383-
.build();
384+
tx = S3TransferManager.builder().s3Client(amazonClient).executor(executorService).build();
384385

385386
FileDownload fileDownload =
386387
tx.downloadFile(
@@ -456,9 +457,7 @@ public void download(
456457
ex, ++retryCount, StorageHelper.DOWNLOAD, session, command, this, queryId);
457458

458459
} finally {
459-
if (tx != null) {
460-
tx.close();
461-
}
460+
closeTransferManagerShutdownExecutor("download", tx, executorService);
462461
}
463462
} while (retryCount <= getMaxRetries());
464463

@@ -628,17 +627,18 @@ public void upload(
628627
Stopwatch stopwatch = new Stopwatch();
629628
stopwatch.start();
630629
do {
630+
tx = null;
631+
ThreadPoolExecutor executorService = null;
631632
try {
632633
PutObjectRequest.Builder putRequestBuilder =
633634
((S3ObjectMetadata) meta)
634635
.getS3PutObjectRequest().toBuilder()
635636
.bucket(remoteStorageLocation)
636637
.key(destFileName);
637638

638-
logger.debug(
639-
"Creating executor service for transfer" + "manager with {} threads", parallelism);
639+
logger.debug("Creating executor service for transfer manager with {} threads", parallelism);
640640

641-
ThreadPoolExecutor executorService =
641+
executorService =
642642
createDefaultExecutorService("s3-transfer-manager-uploader-", parallelism);
643643
// upload files to s3
644644
tx = S3TransferManager.builder().s3Client(amazonClient).executor(executorService).build();
@@ -722,9 +722,7 @@ public void upload(
722722
toClose,
723723
queryId);
724724
} finally {
725-
if (tx != null) {
726-
tx.close();
727-
}
725+
closeTransferManagerShutdownExecutor("upload", tx, executorService);
728726
}
729727
} while (retryCount <= getMaxRetries());
730728

@@ -983,4 +981,36 @@ public int getSocketTimeout() {
983981
return socketTimeout;
984982
}
985983
}
984+
985+
private static void closeTransferManagerShutdownExecutor(
986+
String name, S3TransferManager tx, ThreadPoolExecutor executor) {
987+
try {
988+
if (tx != null) {
989+
tx.close();
990+
}
991+
} catch (Exception e) {
992+
logger.warn("Failed to close S3 {} transfer manager", name, e);
993+
} finally {
994+
if (executor != null) {
995+
try {
996+
executor.shutdown();
997+
if (!executor.awaitTermination(EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
998+
logger.warn(
999+
"S3 {} executor did not terminate within {} seconds, forcing shutdown",
1000+
name,
1001+
EXECUTOR_SHUTDOWN_TIMEOUT_SECONDS);
1002+
executor.shutdownNow();
1003+
}
1004+
} catch (InterruptedException e) {
1005+
// The only checked exception from awaitTermination so need to reset the interrupt flag
1006+
logger.warn("S3 {} executor shutdown interrupted, forcing shutdown", name);
1007+
executor.shutdownNow();
1008+
Thread.currentThread().interrupt();
1009+
} catch (Exception e) {
1010+
logger.warn("Failed to shut down S3 {} executor, forcing shutdown", name, e);
1011+
executor.shutdownNow();
1012+
}
1013+
}
1014+
}
1015+
}
9861016
}

src/test/java/net/snowflake/client/internal/jdbc/StatementIT.java

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@
1212
import static org.junit.jupiter.api.Assertions.assertTrue;
1313

1414
import java.io.File;
15+
import java.io.FileOutputStream;
16+
import java.nio.charset.StandardCharsets;
17+
import java.nio.file.Files;
18+
import java.nio.file.Path;
1519
import java.sql.BatchUpdateException;
1620
import java.sql.Connection;
1721
import java.sql.ResultSet;
@@ -609,4 +613,168 @@ public void testQueryIdIsNullOnFreshStatement() throws SQLException {
609613
assertNull(stmt.unwrap(SnowflakeStatement.class).getQueryID());
610614
}
611615
}
616+
617+
@Test
618+
public void testRepeatedPutDoesNotLeakThreads() throws Exception {
619+
int iterations = Integer.getInteger("snowflake.test.putThreadLeak.iterations", 10);
620+
int putParallelism = 4;
621+
assertTrue(iterations > 0, "Iterations must be positive");
622+
623+
File testFile = new File(tmpFolder, "thread_leak_test_1mb.csv");
624+
createCsvFile(testFile, 1024 * 1024);
625+
String stageName = "TEMP_THREAD_LEAK_STAGE_" + System.nanoTime();
626+
627+
try (Statement statement = connection.createStatement()) {
628+
statement.execute("CREATE OR REPLACE TEMPORARY STAGE " + stageName);
629+
630+
// Warm up: first PUT initializes Netty event loops and other one-time resources
631+
String warmupPut =
632+
"PUT file://"
633+
+ testFile.getCanonicalPath()
634+
+ " @"
635+
+ stageName
636+
+ " PARALLEL="
637+
+ putParallelism
638+
+ " AUTO_COMPRESS=FALSE";
639+
try (ResultSet rs = statement.executeQuery(warmupPut)) {
640+
while (rs.next()) {}
641+
}
642+
waitForTransferThreadCount(putParallelism);
643+
644+
long baselineTransferThreads = countTransferManagerThreads();
645+
646+
// Use a unique file name per iteration to avoid GCS per-object rate limits
647+
for (int i = 0; i < iterations; i++) {
648+
Path iterFile = tmpFolder.toPath().resolve("thread_leak_put_" + i + ".csv");
649+
Files.createLink(iterFile, testFile.toPath());
650+
String putCommand =
651+
"PUT file://"
652+
+ iterFile.toFile().getCanonicalPath()
653+
+ " @"
654+
+ stageName
655+
+ " PARALLEL="
656+
+ putParallelism
657+
+ " AUTO_COMPRESS=FALSE";
658+
try (ResultSet rs = statement.executeQuery(putCommand)) {
659+
while (rs.next()) {}
660+
}
661+
}
662+
663+
waitForTransferThreadCount(baselineTransferThreads);
664+
long finalTransferThreads = countTransferManagerThreads();
665+
666+
// Without the fix, each PUT leaks `putParallelism` threads, so after
667+
// N iterations we'd see N * putParallelism extra threads (e.g. 10 * 4 = 40).
668+
// With the fix, all executor threads are shut down after each PUT,
669+
// so the count must not grow at all.
670+
long threadGrowth = finalTransferThreads - baselineTransferThreads;
671+
assertTrue(
672+
threadGrowth <= 0,
673+
String.format(
674+
"Transfer manager threads grew by %d after %d PUTs with PARALLEL=%d "
675+
+ "(baseline=%d, final=%d, would be %d without fix)."
676+
+ " This indicates a thread pool leak in S3 upload.",
677+
threadGrowth,
678+
iterations,
679+
putParallelism,
680+
baselineTransferThreads,
681+
finalTransferThreads,
682+
(long) iterations * putParallelism));
683+
}
684+
}
685+
686+
@Test
687+
public void testRepeatedGetDoesNotLeakThreads() throws Exception {
688+
int iterations = Integer.getInteger("snowflake.test.getThreadLeak.iterations", 10);
689+
int getParallelism = 4;
690+
assertTrue(iterations > 0, "Iterations must be positive");
691+
692+
File testFile = new File(tmpFolder, "thread_leak_test_1mb.csv");
693+
createCsvFile(testFile, 1024 * 1024);
694+
String stageName = "TEMP_THREAD_LEAK_GET_STAGE_" + System.nanoTime();
695+
File destFolder = new File(tmpFolder, "downloads");
696+
destFolder.mkdirs();
697+
698+
try (Statement statement = connection.createStatement()) {
699+
statement.execute("CREATE OR REPLACE TEMPORARY STAGE " + stageName);
700+
701+
String putCommand =
702+
"PUT file://"
703+
+ testFile.getCanonicalPath()
704+
+ " @"
705+
+ stageName
706+
+ " AUTO_COMPRESS=FALSE OVERWRITE=TRUE";
707+
try (ResultSet rs = statement.executeQuery(putCommand)) {
708+
while (rs.next()) {}
709+
}
710+
711+
String getCommand =
712+
"GET @"
713+
+ stageName
714+
+ " 'file://"
715+
+ destFolder.getCanonicalPath().replace("\\", "/")
716+
+ "' PARALLEL="
717+
+ getParallelism;
718+
719+
// Warm up: first GET initializes Netty event loops and other one-time resources
720+
try (ResultSet rs = statement.executeQuery(getCommand)) {
721+
while (rs.next()) {}
722+
}
723+
waitForTransferThreadCount(getParallelism);
724+
725+
long baselineTransferThreads = countTransferManagerThreads();
726+
727+
for (int i = 0; i < iterations; i++) {
728+
for (File f : destFolder.listFiles()) {
729+
f.delete();
730+
}
731+
try (ResultSet rs = statement.executeQuery(getCommand)) {
732+
while (rs.next()) {}
733+
}
734+
}
735+
736+
waitForTransferThreadCount(baselineTransferThreads);
737+
long finalTransferThreads = countTransferManagerThreads();
738+
739+
long threadGrowth = finalTransferThreads - baselineTransferThreads;
740+
assertTrue(
741+
threadGrowth <= 0,
742+
String.format(
743+
"Transfer manager threads grew by %d after %d GETs with PARALLEL=%d "
744+
+ "(baseline=%d, final=%d, would be %d without fix)."
745+
+ " This indicates a thread pool leak in S3 download.",
746+
threadGrowth,
747+
iterations,
748+
getParallelism,
749+
baselineTransferThreads,
750+
finalTransferThreads,
751+
(long) iterations * getParallelism));
752+
}
753+
}
754+
755+
private static void waitForTransferThreadCount(long atMost) {
756+
Awaitility.await()
757+
.atMost(Duration.ofSeconds(7))
758+
.pollInterval(Duration.ofSeconds(1))
759+
.until(() -> countTransferManagerThreads() <= atMost);
760+
}
761+
762+
private static long countTransferManagerThreads() {
763+
return Thread.getAllStackTraces().keySet().stream()
764+
.filter(t -> t.getName().startsWith("s3-transfer-manager-"))
765+
.count();
766+
}
767+
768+
private static void createCsvFile(File targetFile, long targetBytes) throws Exception {
769+
byte[] line =
770+
"1,abcdefghijklmnopqrstuvwxyz,2026-01-01T00:00:00Z\n".getBytes(StandardCharsets.US_ASCII);
771+
try (FileOutputStream out = new FileOutputStream(targetFile)) {
772+
long written = 0;
773+
while (written < targetBytes) {
774+
int chunk = (int) Math.min(line.length, targetBytes - written);
775+
out.write(line, 0, chunk);
776+
written += chunk;
777+
}
778+
}
779+
}
612780
}

0 commit comments

Comments
 (0)