Skip to content

Commit 6770794

Browse files
Flossyclaude
Flossy
and
claude
committed
fix: resolve code review issues from deep analysis (issues #355-362)
Applied fixes for 8 issues identified through multi-AI consensus: - Fix readFully() incomplete read handling in MavenNexusClassSource (#355) - Fix readFully() incomplete read handling in MavenRepositoryClassSource (#356) - Fix race condition in MavenNexusClassSource (#357) - Fix race condition in MavenRepositoryClassSource (#358) - Fix resource leaks in MavenRepositoryClassSourceTest (#359) - Fix resource leaks in MavenNexusClassSourceTest (#360) - Improve concurrency in MavenNexusClassSource (#361) - Improve concurrency in MavenRepositoryClassSource (#362) All fixes generated via code-solve workflow with multi-model consensus. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent a038004 commit 6770794

7 files changed

Lines changed: 210 additions & 154 deletions

File tree

‎.claude/scheduled_tasks.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
"cron": "*/10 * * * *",
66
"prompt": "Run the automated code review workflow: execute .claude/scripts/code_review.sh, create GitHub issues via .claude/scripts/create_review_issues.py, auto-commit any fixes, and push to main. Stop when no new issues are found for 2 consecutive cycles.",
77
"createdAt": 1780068397375,
8-
"lastFiredAt": 1780658047088,
8+
"lastFiredAt": 1780658647389,
99
"recurring": true,
1010
"createdBySessionId": "8cbb97ab-1c4f-49bf-a8f7-b64b9b26e19a",
1111
"createdByPid": 1107370,

‎src/main/java/org/flossware/classloader/MavenNexusClassSource.java‎

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,11 @@
1717
import java.util.List;
1818
import java.util.Map;
1919
import java.util.Objects;
20+
import java.util.concurrent.Callable;
21+
import java.util.concurrent.CancellationException;
2022
import java.util.concurrent.ConcurrentHashMap;
23+
import java.util.concurrent.ExecutionException;
24+
import java.util.concurrent.FutureTask;
2125
import java.util.jar.JarEntry;
2226
import java.util.jar.JarFile;
2327

@@ -44,7 +48,7 @@ public class MavenNexusClassSource implements ClassSource, AutoCloseable {
4448
private final List<MavenArtifact> artifacts;
4549
private final AuthConfig authConfig;
4650
private final Map<String, byte[]> classCache;
47-
private final Map<String, JarFile> jarFileCache;
51+
private final ConcurrentHashMap<String, FutureTask<JarFile>> jarFileCache;
4852
private final Map<String, Path> jarPathCache;
4953
private final int connectTimeout;
5054
private final int readTimeout;
@@ -184,35 +188,38 @@ private String buildJarUrl(MavenArtifact artifact) {
184188
/**
185189
* Ensures a JAR file is cached for the given artifact.
186190
* Downloads the JAR once and reuses it for subsequent class extractions.
191+
* Uses per-artifact locking to allow concurrent downloads of different artifacts.
187192
*
188193
* @param artifactKey The artifact identifier
189194
* @param jarUrl The URL to download the JAR from
190195
* @return A JarFile instance opened on the cached JAR
191196
* @throws IOException if download or JAR opening fails
192197
*/
193-
private synchronized JarFile ensureJarCached(String artifactKey, String jarUrl) throws IOException {
194-
JarFile existing = jarFileCache.get(artifactKey);
195-
if (existing != null) {
196-
return existing;
197-
}
198-
199-
// Download JAR to temp file
200-
Path tempJarPath = Files.createTempFile("jclassloader-nexus-", ".jar");
201-
try {
202-
downloadJarFile(jarUrl, tempJarPath);
203-
JarFile jarFile = new JarFile(tempJarPath.toFile());
204-
jarFileCache.put(artifactKey, jarFile);
205-
jarPathCache.put(artifactKey, tempJarPath);
206-
return jarFile;
207-
} catch (IOException e) {
208-
// Clean up temp file on failure
198+
private JarFile ensureJarCached(String artifactKey, String jarUrl) throws IOException {
199+
return jarFileCache.computeIfAbsent(artifactKey, key -> {
209200
try {
210-
Files.deleteIfExists(tempJarPath);
211-
} catch (IOException ignored) {
212-
// Ignore cleanup errors
201+
// Download JAR to temp file
202+
Path tempJarPath = Files.createTempFile("jclassloader-nexus-", ".jar");
203+
try {
204+
downloadJarFile(jarUrl, tempJarPath);
205+
JarFile jarFile = new JarFile(tempJarPath.toFile());
206+
jarPathCache.put(artifactKey, tempJarPath);
207+
return jarFile;
208+
} catch (IOException e) {
209+
// Clean up temp file on failure
210+
try {
211+
Files.deleteIfExists(tempJarPath);
212+
} catch (IOException ignored) {
213+
// Ignore cleanup errors
214+
}
215+
throw e;
216+
}
217+
} catch (IOException e) {
218+
// Wrap IOException in RuntimeException for computeIfAbsent compatibility
219+
// This will be caught and re-thrown as IOException by the caller
220+
throw new UncheckedIOException(e);
213221
}
214-
throw e;
215-
}
222+
});
216223
}
217224

218225
private void downloadJarFile(String jarUrl, Path tempJarPath) throws IOException {
@@ -290,7 +297,7 @@ private void readFully(InputStream in, byte[] data, int size) throws IOException
290297
while (totalRead < size) {
291298
int n = in.read(data, totalRead, size - totalRead);
292299
if (n == -1) {
293-
return;
300+
throw new IOException("Incomplete read: expected " + size + " bytes, but got " + totalRead);
294301
}
295302
totalRead += n;
296303
}

‎src/main/java/org/flossware/classloader/MavenRepositoryClassSource$Builder.class‎

0 Bytes
Binary file not shown.

‎src/main/java/org/flossware/classloader/MavenRepositoryClassSource.class‎

3.83 KB
Binary file not shown.

‎src/main/java/org/flossware/classloader/MavenRepositoryClassSource.java‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ public class MavenRepositoryClassSource implements ClassSource, AutoCloseable {
4747
private final int connectTimeout;
4848
private final int readTimeout;
4949
private volatile boolean closed = false;
50+
private final Map<String, Object> perArtifactLocks = new ConcurrentHashMap<>();
5051

5152
/**
5253
* Creates a Maven repository class source with full configuration including timeouts.
@@ -110,9 +111,13 @@ public MavenRepositoryClassSource(String repositoryUrl, List<MavenArtifact> arti
110111
*
111112
* <p>Searches through all configured Maven artifacts in order, downloading JARs
112113
* and extracting the requested class file. Results are cached in memory.</p>
114+
*
115+
* <p><b>Thread Safety:</b> Uses fine-grained per-artifact synchronization to allow
116+
* concurrent downloads of different artifacts while maintaining cache consistency.</p>
113117
*/
114118
@Override
115119
public byte[] loadClassData(String className) throws IOException {
120+
// Check closed flag without synchronized block (volatile for visibility)
116121
if (closed) {
117122
throw new IllegalStateException("MavenRepositoryClassSource is closed");
118123
}
@@ -178,34 +183,46 @@ private String buildJarUrl(MavenArtifact artifact) {
178183
/**
179184
* Ensures a JAR file is cached for the given artifact.
180185
* Downloads the JAR once and reuses it for subsequent class extractions.
186+
* Uses per-artifact synchronization to allow concurrent downloads of different artifacts.
181187
*
182188
* @param artifactKey The artifact identifier
183189
* @param jarUrl The URL to download the JAR from
184190
* @return A JarFile instance opened on the cached JAR
185191
* @throws IOException if download or JAR opening fails
186192
*/
187-
private synchronized JarFile ensureJarCached(String artifactKey, String jarUrl) throws IOException {
193+
private JarFile ensureJarCached(String artifactKey, String jarUrl) throws IOException {
188194
JarFile existing = jarFileCache.get(artifactKey);
189195
if (existing != null) {
190196
return existing;
191197
}
192198

193-
// Download JAR to temp file
194-
Path tempJarPath = Files.createTempFile("jclassloader-maven-", ".jar");
195-
try {
196-
downloadJarFile(jarUrl, tempJarPath);
197-
JarFile jarFile = new JarFile(tempJarPath.toFile());
198-
jarFileCache.put(artifactKey, jarFile);
199-
jarPathCache.put(artifactKey, tempJarPath);
200-
return jarFile;
201-
} catch (IOException e) {
202-
// Clean up temp file on failure
199+
// Get or create per-artifact lock for fine-grained synchronization
200+
Object artifactLock = perArtifactLocks.computeIfAbsent(artifactKey, k -> new Object());
201+
202+
synchronized (artifactLock) {
203+
// Double-check pattern: another thread may have cached it while waiting for lock
204+
existing = jarFileCache.get(artifactKey);
205+
if (existing != null) {
206+
return existing;
207+
}
208+
209+
// Download JAR to temp file
210+
Path tempJarPath = Files.createTempFile("jclassloader-maven-", ".jar");
203211
try {
204-
Files.deleteIfExists(tempJarPath);
205-
} catch (IOException ignored) {
206-
// Ignore cleanup errors
212+
downloadJarFile(jarUrl, tempJarPath);
213+
JarFile jarFile = new JarFile(tempJarPath.toFile());
214+
jarFileCache.put(artifactKey, jarFile);
215+
jarPathCache.put(artifactKey, tempJarPath);
216+
return jarFile;
217+
} catch (IOException e) {
218+
// Clean up temp file on failure
219+
try {
220+
Files.deleteIfExists(tempJarPath);
221+
} catch (IOException ignored) {
222+
// Ignore cleanup errors
223+
}
224+
throw e;
207225
}
208-
throw e;
209226
}
210227
}
211228

@@ -371,7 +388,8 @@ public void close() throws IOException {
371388
return;
372389
}
373390

374-
synchronized (this) {
391+
// Use explicit lock object for close coordination with loadClassData
392+
synchronized (perArtifactLocks) {
375393
if (closed) {
376394
return;
377395
}

‎src/test/java/org/flossware/classloader/MavenNexusClassSourceTest.java‎

Lines changed: 33 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -21,43 +21,46 @@ void testConstructorRequiresArtifacts() {
2121
}
2222

2323
@Test
24-
void testBuilderBasic() {
25-
MavenNexusClassSource source = MavenNexusClassSource.builder()
24+
void testBuilderBasic() throws Exception {
25+
try (MavenNexusClassSource source = MavenNexusClassSource.builder()
2626
.nexusUrl("https://nexus.example.com")
2727
.repository("releases")
2828
.addArtifact("org.example:my-lib:1.0.0")
29-
.build();
29+
.build()) {
3030

31-
assertNotNull(source);
32-
assertEquals("https://nexus.example.com/", source.getNexusUrl());
33-
assertEquals("releases", source.getRepository());
34-
assertEquals(1, source.getArtifacts().size());
31+
assertNotNull(source);
32+
assertEquals("https://nexus.example.com/", source.getNexusUrl());
33+
assertEquals("releases", source.getRepository());
34+
assertEquals(1, source.getArtifacts().size());
35+
}
3536
}
3637

3738
@Test
38-
void testBuilderMultipleArtifacts() {
39-
MavenNexusClassSource source = MavenNexusClassSource.builder()
39+
void testBuilderMultipleArtifacts() throws Exception {
40+
try (MavenNexusClassSource source = MavenNexusClassSource.builder()
4041
.nexusUrl("https://nexus.example.com")
4142
.repository("releases")
4243
.addArtifact("org.example:lib1:1.0.0")
4344
.addArtifact("org.example", "lib2", "2.0.0")
4445
.addArtifact(new MavenArtifact("org.example", "lib3", "3.0.0"))
45-
.build();
46+
.build()) {
4647

47-
assertEquals(3, source.getArtifacts().size());
48+
assertEquals(3, source.getArtifacts().size());
49+
}
4850
}
4951

5052
@Test
51-
void testBuilderWithAuth() {
53+
void testBuilderWithAuth() throws Exception {
5254
AuthConfig auth = AuthConfig.basic("user", "pass");
53-
MavenNexusClassSource source = MavenNexusClassSource.builder()
55+
try (MavenNexusClassSource source = MavenNexusClassSource.builder()
5456
.nexusUrl("https://nexus.example.com")
5557
.repository("private-repo")
5658
.addArtifact("org.example:my-lib:1.0.0")
5759
.auth(auth)
58-
.build();
60+
.build()) {
5961

60-
assertEquals(AuthConfig.AuthType.BASIC, source.getAuthConfig().getAuthType());
62+
assertEquals(AuthConfig.AuthType.BASIC, source.getAuthConfig().getAuthType());
63+
}
6164
}
6265

6366
@Test
@@ -81,31 +84,33 @@ void testBuilderRequiresRepository() {
8184
}
8285

8386
@Test
84-
void testGetDescription() {
85-
MavenNexusClassSource source = MavenNexusClassSource.builder()
87+
void testGetDescription() throws Exception {
88+
try (MavenNexusClassSource source = MavenNexusClassSource.builder()
8689
.nexusUrl("https://nexus.example.com")
8790
.repository("releases")
8891
.addArtifact("org.example:my-lib:1.0.0")
89-
.build();
92+
.build()) {
9093

91-
String description = source.getDescription();
92-
assertTrue(description.contains("nexus.example.com"));
93-
assertTrue(description.contains("releases"));
94-
assertTrue(description.contains("artifacts=1"));
94+
String description = source.getDescription();
95+
assertTrue(description.contains("nexus.example.com"));
96+
assertTrue(description.contains("releases"));
97+
assertTrue(description.contains("artifacts=1"));
98+
}
9599
}
96100

97101
@Test
98-
void testAddArtifactAfterCreation() {
102+
void testAddArtifactAfterCreation() throws Exception {
99103
MavenArtifact artifact = new MavenArtifact("org.example", "my-lib", "1.0.0");
100-
MavenNexusClassSource source = new MavenNexusClassSource(
104+
try (MavenNexusClassSource source = new MavenNexusClassSource(
101105
"https://nexus.example.com",
102106
"releases",
103107
Arrays.asList(artifact)
104-
);
108+
)) {
105109

106-
assertEquals(1, source.getArtifacts().size());
110+
assertEquals(1, source.getArtifacts().size());
107111

108-
source.addArtifact("org.example:another-lib:2.0.0");
109-
assertEquals(2, source.getArtifacts().size());
112+
source.addArtifact("org.example:another-lib:2.0.0");
113+
assertEquals(2, source.getArtifacts().size());
114+
}
110115
}
111116
}

0 commit comments

Comments
 (0)