CAMEL-23459: Add TTL cleanup for pending async tasks to prevent memory leak#23125
Conversation
…y leak DoclingComponent now tracks pending async-conversion task IDs with timestamps in AsyncTaskEntry wrapper objects. A scheduled cleanup task runs periodically (every 10% of TTL, minimum 1 minute) to evict entries older than the configured asyncTaskTtl (default 24 hours). - Added AsyncTaskEntry class to wrap CompletableFuture with creation timestamp - Added asyncTaskTtl configuration option (default 86400000ms / 24 hours) - Implemented scheduled cleanup using Camel's ScheduledExecutorService - Added DEBUG logging for evictions with task ID and age - Updated documentation in docling-component.adoc - Added unit tests for TTL eviction behavior Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
Changes not staged for commit: |
gnodet
left a comment
There was a problem hiding this comment.
Nice work addressing the memory leak, Andrea — the TTL-based eviction approach is clean and backward-compatible.
A few items to address before this can merge:
Blocking
-
Missing regenerated files — CI is failing on the "uncommitted changes" check. As Claus already noted,
catalog/camel-catalog/.../docling.json,dsl/camel-componentdsl/.../DoclingComponentBuilderFactory.java, anddsl/camel-endpointdsl/.../DoclingEndpointBuilderFactory.javaneed to be regenerated and committed. -
Thread.sleep()in test code —testAsyncTaskNotEvictedBeforeTtl()usesThread.sleep(1000). The project convention requires Awaitility instead (see CLAUDE.md "Asynchronous Testing" section). See inline comment for a suggested replacement.
Non-blocking suggestions
-
Test comment vs actual behavior — The comment in
testAsyncTaskTtlEvictionsays "for 2s TTL that's 200ms" but the code enforces a 1-minute minimum (Math.max(ttl / 10, 60000)). If the cleanup interval is actually 60s, theawait().atMost(5, SECONDS)assertion may be fragile. Worth double-checking this passes reliably. -
Logging level —
doStart(),doStop(), and periodic cleanup all log atINFO. For a component library, routine lifecycle and housekeeping are typicallyDEBUGto avoid noisy output for end users. See inline comment.
Claude Code on behalf of Guillaume Nodet
- Replace Thread.sleep with Awaitility during/atMost pattern in DoclingAsyncTaskTtlTest - Lower cleanup-interval floor from 60s to 1s so short TTLs (e.g. test TTLs) actually fire; cleanup is cheap when no tasks are expired so the lower minimum is safe - Override isUseAdviceWith() in DoclingAsyncTaskTtlTest so the component's doStart sees the test-configured TTL instead of the default - Update DoclingAsyncConversionTest to use AsyncTaskEntry (was still inserting CompletableFuture into the map, causing ClassCastException) - Lower lifecycle/housekeeping LOG.info calls to LOG.debug - Regenerate top-level catalog/DSL artifacts (docling.json, DoclingComponentBuilderFactory, DoclingEndpointBuilderFactory) so the asyncTaskTtl option is exposed there too Bob on behalf of Andrea Cosentino
|
Pushed 7c1479f addressing all of @gnodet's and @davsclaus's review feedback: Blocking items
Non-blocking suggestions (both applied)
Drive-by Verification
cc @Croway @davsclaus @gnodet for re-review when convenient. Claude Code on behalf of Andrea Cosentino |
|
🧪 CI tested the following changed modules:
All tested modules (11 modules)
|
gnodet
left a comment
There was a problem hiding this comment.
All feedback addressed — Thread.sleep replaced with Awaitility, misleading comment fixed, LOG.info demoted to DEBUG, and generated files regenerated. CI is green. LGTM.
Claude Code on behalf of Guillaume Nodet
Summary
Fixes memory leak in camel-docling component where pending async conversion tasks accumulate indefinitely in the shared
pendingAsyncTasksmap.Changes
CompletableFuturewith creation timestampdocling-component.adocwith new configuration optionDoclingAsyncTaskTtlTestwith three test scenarios (Awaitility-based, noThread.sleep)Risk Assessment
Low risk - The change is additive and backward compatible:
Testing
mvn clean install -DskipTests): SUCCESS, no uncommitted regen artifactsReview feedback addressed (7c1479f)
Thread.sleepin tests with Awaitilityduring/atMostpattern (@gnodet)INFO→DEBUG(@gnodet)docling.json,DoclingComponentBuilderFactory,DoclingEndpointBuilderFactory) (@davsclaus, @gnodet)DoclingAsyncConversionTestto useAsyncTaskEntry(drive-by — the old test was still inserting rawCompletableFutureand throwingClassCastException)Related
Bob on behalf of Andrea Cosentino