fix(plugins): serialize loadJob per definition id to prevent file-handle leak#83
Merged
Merged
Conversation
…dle leak Two concurrent loadJob calls for the same definition id could both pass the already-loaded guard, each open a DynamicJobClassLoader (JAR file-handle), and the losing thread's classLoaders.put would overwrite the winner's entry without closing it — leaking the winner's handle. Wrap the load body (extracted into doLoadJob) in a per-definition ReentrantLock so the guard, classloader creation, registry write, and classLoaders.put execute atomically for a given id. The loser now observes the winner's registration and is rejected before opening a second handle. Adds a deterministic concurrency test that reproduces the leak by parking the winner inside registerDynamicPlugin and asserting the contender cannot enter the load body.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDynamicJobLoaderService now serializes same-definition loadJob calls with per-id ReentrantLocks. The load path is split so the lock is held only around the actual load work, and tests cover missing IDs plus concurrent same-id loads. ChangesConcurrent job load serialization
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
fronzec
marked this pull request as ready for review
July 2, 2026 07:07
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoaderServiceTest.java (1)
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the wildcard concurrency import with explicit imports.
Proposed fix
-import java.util.concurrent.*; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger;As per coding guidelines,
fr-batch-service/**/*.java: “Avoid wildcard imports (import com.example.*) and prefer explicit imports”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoaderServiceTest.java` around lines 22 - 23, Replace the wildcard java.util.concurrent import in DynamicJobLoaderServiceTest with explicit imports for only the concurrency types actually used, keeping AtomicInteger as a direct import and removing the blanket import to match the no-wildcard-import guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoaderService.java`:
- Around line 54-63: The per-definition lock map in DynamicJobLoaderService is
retaining ReentrantLock entries for IDs that do not exist because the lock is
created before the repository check. Change the flow around loadJob(Long) so
existence is validated via findById before calling computeIfAbsent on loadLocks,
and keep the second existence check inside doLoadJob under the lock for
correctness. Preserve the existing atomic load behavior while ensuring
nonexistent definition IDs do not leave permanent entries in loadLocks.
In
`@fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoaderServiceTest.java`:
- Around line 526-538: The concurrent load test in DynamicJobLoaderServiceTest
leaves the real classloader open after a successful load, so add cleanup by
calling the matching unload path after the assertions to release the temp JAR
handle deterministically. Use the existing DynamicJobLoaderService/unloadJob
flow (with the loaded definition id from the test setup) so the classloader
tracked by the load is closed before the test finishes.
---
Nitpick comments:
In
`@fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoaderServiceTest.java`:
- Around line 22-23: Replace the wildcard java.util.concurrent import in
DynamicJobLoaderServiceTest with explicit imports for only the concurrency types
actually used, keeping AtomicInteger as a direct import and removing the blanket
import to match the no-wildcard-import guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a552ff94-040f-4da5-a876-a22603569bc1
📒 Files selected for processing (2)
fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoaderService.javafr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoaderServiceTest.java
Gate per-definition lock allocation on findById existence so that a load
request for an unknown id (e.g. POST /definitions/{id}/load with a bogus
id) no longer stores a ReentrantLock in loadLocks forever. The
authoritative existence check still runs inside doLoadJob under the lock.
Also clean up the concurrent-load test: unload after the assertions to
release the real classloader/temp-JAR handle, release the register latch
in finally so shutdown cannot hang, and replace the wildcard
java.util.concurrent import with explicit imports.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Makes
DynamicJobLoaderService.loadJobatomic per definition id, closing a file-handle leak on concurrent loads (spine finding P1).Why
loadJobhad a check-then-act race with no per-definition mutual exclusion:getRegisteredJobNames().contains(jobName)).DynamicJobClassLoader— aURLClassLoaderthat opens the JAR file-handle.classLoaders.put(definitionId, classLoader).Two concurrent
POST /definitions/{id}/loadfor the same id both pass step 1 (neither is registered yet), both open the JAR, and the losing thread'sputoverwrites the winner's map entry without closing it. The overwritten classloader is nevercleanup()-ed → leaked file-handle (thecatchonly cleans up on exception, not on the overwrite race).How
ConcurrentHashMap<Long, ReentrantLock> loadLocks.doLoadJoband wrap it in a per-definition lock (lock()outsidetry,unlock()infinally— released on every path).classLoaders.putnow execute atomically for a given id. The loser observes the winner's registration and is rejected withIllegalStateExceptionbefore opening a second handle.Since
job_nameisuniqueper definition (DB constraint) andidis the PK, locking by id is equivalent to locking by job name — no two ids collide on a name.Testing
loadJob_concurrentSameId_serializesAndOpensSingleClassloader: parks the winner insideregisterDynamicPlugin(holding the lock) and asserts the contender cannot enter the load body and never triggers a second registration. Fails without the fix, passes with it. Uses a realDynamicJobClassLoader(Mockito construction mocking is thread-confined).DynamicJobLoaderServiceTest: 18/18 green.DynamicJobLoadingIntegrationTest: 13/13 green.Known limitation (out of scope — tracked for PR-C)
This closes the load-vs-load leak only.
unloadJob/reloadJobdo not yet acquire the lock, so a load racing an unload/reload of the same id can still leave an orphaned classloader. That is the P2 slice (unload + transaction boundary) and lands in PR-C.Summary by CodeRabbit