Release the flush scheduler when a batch storer is abandoned without close()#307
Merged
Conversation
…close() A Batching storer scheduled its background flush as a bound method reference (this::backgroundFlush), so the executor's worker thread — a GC root — strongly pinned the storer. A storer dropped without close() therefore leaked its chunk buffers, hash slots and trusted-object-id set plus one daemon thread for the JVM lifetime, even though the non-batching storers are collectable when abandoned. Move the executor into a BackgroundFlusher helper that holds the storer through a WeakReference. The periodic task now references only the flusher, so an abandoned storer becomes collectable immediately; the next tick observes the cleared referent and shuts the executor down (on the executor thread, without awaitTermination), reclaiming the daemon thread. close() delegates its graceful shutdown to the flusher, preserving the previous behavior. This mirrors EvictionManager.IntervalThread and the JVector BackgroundTaskManager.
There was a problem hiding this comment.
Pull request overview
Fixes a memory/thread leak in BinaryStorer.Batching where scheduling a bound method (this::backgroundFlush) caused the scheduled executor thread to strongly retain the storer, preventing GC when abandoned without close().
Changes:
- Replaced the inlined
ScheduledExecutorServicefield with aBackgroundFlusherhelper that holds the storer viaWeakReference. - Updated shutdown logic so
close()delegates graceful executor termination to the flusher. - Added documentation explaining the retention/leak mechanism and the self-termination behavior when the storer is GC’d.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zdenek-jonas
approved these changes
Jul 21, 2026
This was referenced Jul 21, 2026
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
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.
Summary
A
Batchingstorer (BinaryStorer.Batching, theBatchStorerimplementation) scheduled its background flush as a bound method reference (this::backgroundFlush). Because the scheduled task strongly referenced the storer, the executor's worker thread — a GC root — pinned the storer for the lifetime of the JVM. A storer that was dropped without callingclose()therefore leaked its direct chunk buffers, hash slots and trusted-object-id set, plus one background daemon thread, per abandoned instance. The non-batching storers are held only weakly by the object manager and are collectable when abandoned, so drop-and-forget looked safe.Fix
Move the executor into a
BackgroundFlusherhelper that holds the storer through aWeakReference:awaitTermination(which would dead-lock) — reclaiming the daemon thread.close()delegates its graceful shutdown (shutdown →awaitTermination→shutdownNow) to the flusher; the publicclose()behavior, including the final flush, is unchanged.This mirrors the existing idiom in the tree:
EvictionManager.IntervalThreadand the JVectorBackgroundTaskManager.Testing
A deterministic regression test lives in the eclipse-store repository (
test.eclipse.store.various.storer.BatchStorerAbandonedLeakTest) and covers both symptoms — leakedbatch-storer-flushthreads and the storer instance staying reachable after a full GC sweep (with a non-batch storer as a control). It fails against the previous code and passes with this change. The existingBatchStorer*suite continues to pass.No public API change.