Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ public Thread newThread(Runnable r) {
return thread;
}
};
return new ThreadPoolExecutor(1, poolSize, 1L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), threadFactory);
ThreadPoolExecutor executor = new ThreadPoolExecutor(
1, poolSize, 1L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>(), threadFactory);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot reducing the timeout here seems not really useful, a threadpool is all about reusing threads and making them to timeout immediately makes it quite useless.

Instead one should take care that the poolsize is limited if we really get 1500 instances something feels fishy. A corepool size equal to the CPU count seems a good upper bound.

Apart from that one might want to adjust the ParallelScatterZipCreator to allow reclaim some of its used memory.

// Allow core threads to time out quickly to prevent ThreadLocal memory leaks
// from Apache Commons Compress ParallelScatterZipCreator
executor.allowCoreThreadTimeOut(true);
return executor;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.atomic.AtomicInteger;

/**
Expand All @@ -32,7 +33,11 @@ public class ConcurrentJarCreatorExecutorServiceFactory {
static ExecutorService createExecutorService(int poolSize) {
int poolCount = POOL_COUNTER.incrementAndGet();
AtomicInteger threadCounter = new AtomicInteger();
return Executors.newThreadPerTaskExecutor(
Thread.ofVirtual().name("plx-arch-" + poolCount + "-" + threadCounter.incrementAndGet()).factory());
ThreadFactory threadFactory = r -> {
return Thread.ofVirtual()
.name("plx-arch-" + poolCount + "-" + threadCounter.incrementAndGet())
.unstarted(r);
};
return Executors.newThreadPerTaskExecutor(threadFactory);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot should we maybe better limit this instead? From the reported 1500 instances it looks like virtual threads really don't play well with thread locals in general and it is more a problem of blowing up quickly.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package org.codehaus.plexus.archiver.zip;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;

import org.apache.commons.compress.archivers.zip.ZipArchiveEntry;
import org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import static org.junit.jupiter.api.Assertions.assertTrue;

/**
* Test to verify that ThreadLocal memory leaks are prevented when creating
* multiple jar files sequentially, which was the root cause of the OOM issue
* reported in https://github.com/codehaus-plexus/plexus-archiver/issues/xxx
*/
class ThreadLocalLeakTest {

@TempDir
File tempDir;

/**
* This test creates multiple jar files to simulate the scenario where
* ThreadLocal values would accumulate in thread pools. The fix ensures
* that threads are terminated quickly after completing tasks, which
* cleans up ThreadLocal values.
*/
@Test
void testMultipleJarCreationsDoNotLeakMemory() throws Exception {
// Create a source file to add to jars
File sourceFile = new File(tempDir, "test.txt");
Files.write(sourceFile.toPath(), "test content".getBytes());

// Create multiple jars sequentially
for (int i = 0; i < 10; i++) {
createJar(new File(tempDir, "test-" + i + ".jar"), sourceFile);
}

// If we got here without OOM, the test passed
assertTrue(true, "Multiple jar creations completed successfully without OOM");
}

private void createJar(File outputFile, File sourceFile) throws Exception {
ConcurrentJarCreator zipCreator =
new ConcurrentJarCreator(Runtime.getRuntime().availableProcessors());

ZipArchiveEntry entry = new ZipArchiveEntry(sourceFile.getName());
entry.setMethod(ZipArchiveEntry.DEFLATED);
entry.setSize(sourceFile.length());
entry.setTime(sourceFile.lastModified());

zipCreator.addArchiveEntry(
entry,
() -> {
try {
return Files.newInputStream(sourceFile.toPath());
} catch (IOException e) {
throw new RuntimeException(e);
}
},
true);

try (ZipArchiveOutputStream zos = new ZipArchiveOutputStream(outputFile)) {
zos.setEncoding("UTF-8");
zipCreator.writeTo(zos);
}
}
}
Loading