Pin lazily-skipped instances in BinaryStorer until commit - #289
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a GC/commit-window consistency hazard in lazy storing: when an instance is skipped because it is already globally known, the storer now has a mechanism to retain a strong reference to that skipped instance until commit completes, preventing the underlying entity from being garbage-collected/deleted while an in-flight commit still writes references to it.
Changes:
- Add a new default no-op callback
PersistenceObjectIdRequestor.registerSkippedOptional(...)to notify storers when lazy logic skips an already-globally-known instance. - Invoke the new callback from
PersistenceObjectManager.Default.ensureObjectId(...)on the global-registry-hit path. - Implement skipped-instance pinning in
BinaryStorervia aPinItemstored in the storer’s hash slots and ensure pins are ignored bylookupOid(...).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| persistence/persistence/src/main/java/org/eclipse/serializer/persistence/types/PersistenceObjectManager.java | Fires the new “skipped optional” hook when an instance is already globally known. |
| persistence/persistence/src/main/java/org/eclipse/serializer/persistence/types/PersistenceObjectIdRequestor.java | Adds the new default no-op registerSkippedOptional(...) callback and documents its intent. |
| persistence/binary/src/main/java/org/eclipse/serializer/persistence/binary/types/BinaryStorer.java | Implements pinning for skipped instances using PinItem and updates lookup behavior to keep pins invisible to lookupOid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hg-ms
approved these changes
Jul 6, 2026
- Pin entries no longer count into size()/isEmpty(): new pinCount, a subset of itemCount, which itself keeps driving the hash rebuild threshold since pins do occupy hash slot entries. Count-based batch flush controllers no longer flush prematurely for mere references to already stored instances, and a pins-only storer is now correctly empty - its commit skips the write instead of persisting an empty chunk. - Eager storers override registerSkippedOptional as a no-op: eager registration already creates a regular item for every encountered instance (strong retention plus serialization), so the pin only duplicated the entry. - Peer lookup (lookupObjectId) excludes pins explicitly instead of relying on them incidentally satisfying the skip-item criterion (null typeHandler).
lookupObjectId broke off the slot-chain search when it hit a pin. A hash rebuild (synchRebuildStoreItems) reverses the chain order within a slot, so a pin can precede a regular item for the same instance (lazy skip pinned it, a later explicit store created the item) - the break then hid the real object<->id association from peer storers. Pins are now ignored without ending the search; skip items keep terminating it (a skip is a user assertion to offer nothing). Latent rather than observable: the peer lookup only runs for instances unknown to the global registry, which a live pin precludes (it holds the instance strongly, keeping the registry's weak entry alive). Fixed for robustness against future invariant changes.
fh-ms
added a commit
to eclipse-store/store
that referenced
this pull request
Jul 6, 2026
…er/serializer#289) (#735) * Add regression test * Add SkipPinningTest: lazy-skipped instances must be pinned until commit Policy-free variant of the internal#73 store-commit-window regression: drops all application references to a lazily-skipped instance between store() and commit(), forces JVM GC + registry cleanup + full storage GC, and asserts the pin keeps the instance (and thereby its entity) alive through the commit and a restart. * Address review: make the pin assertion in SkipPinningTest bite The storer holds the stored parent strongly via its regular item, and parent.payload kept the skipped payload reachable through it - the "must be pinned" probe assertion could never fire, with or without the pin. The parent's field is now cleared right after store() (the serialized state is already captured then), so the storer's pin is the only remaining strong path to the payload. Verified: against a pre-pin serializer build the test now fails deterministically at the pin assertion (previously JVM-timing-dependent); green against main. Also aligns the reproducer's javadoc with its actual guarantees: on an unfixed build it fails only when the JVM actually collects the referent inside the store->commit window; the pass direction (with the fix) is the deterministic one. * Address review: post-commit safety, explicit lazy storer, pin-release test - After commit() the pin is released and (since the previous review fix) nothing else holds the payload, so the post-commit steps could observe a collected probe. The decisive pin assertion now doubles as the strong capture; all later steps use that reference instead of re-polling the probe. Capturing there does not weaken the red direction - on an unfixed build the test fails at that very line. - createStorer() -> createLazyStorer(): only lazy storing logic skips known instances; the intent is now explicit and immune to future changes of the default storer flavor. - New pinIsReleasedOnCommit test covering the pin contract's other direction: with no remaining strong path, the skipped instance must become collectable after commit - otherwise long-lived and batching storers would leak every skipped instance they ever referenced. * Address review: harden the pin-release collection loop GC is not obliged to collect promptly on every JVM/CI configuration. The release assertion now combines a 30 s deadline with bounded memory pressure per iteration instead of a fixed number of System.gc() calls, making collection of the weakly reachable payload overwhelmingly likely before the test can fail spuriously. The pass case remains fast (first iteration on typical JVMs).
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.
What this fixes
When storing an object graph, the storer skips objects it believes are already in the storage and only writes their object ids into the data. Between that skip decision (
store()) and the actual write (commit()), nothing holds on to the skipped object. If the application drops its last reference in exactly that window, the JVM garbage-collects the instance, its object-registry entry disappears, and the storage-side garbage collector may legitimately delete the entity from disk - while the in-flight commit still writes a reference to it.Nothing fails at that moment. The damage only surfaces much later, when a load follows that reference and aborts with
StorageExceptionConsistency: No entity found for objectId N- typically after a restart, with no hint of when or how the reference went dangling.The fix is simple in spirit: while a store is in flight, the storer keeps every skipped object strongly referenced until its commit completes, so the object (and through the existing GC safety nets, its on-disk entity) cannot disappear mid-store. The window is realistic with long-lived and batching storers.
Details
This PR consists of two commits: the fix itself, and a refinement that puts the pins to additional use.
Commit 1 - the fix (pinning):
PersistenceObjectIdRequestor.registerSkippedOptional(long objectId, T instance, PersistenceTypeHandler optionalHandler), fired from the global-registry-hit branch ofPersistenceObjectManager.Default.ensureObjectId- i.e. exactly when lazy storing logic skips an already-known instance and only references its object id.BinaryStorerimplements the hook with a newPinItem extends Iteminserted into the storer-local hash slots: a strong, per-instance deduplicated reference that lives until the storer completes/reinitializes. Pin items are never part of the item chain: they are not serialized and not merged into the global registry.Commit 2 - the refinement (pin visibility split by lookup semantics):
apply(...)paths, a pin is a valid local hit: a pin records exactly the object id the global registry returned when the pin was created, and while the pin holds the instance strongly, its registry entry cannot be reaped nor its id re-mapped - so the cached id cannot go stale. Repeated references to an already-stored instance therefore resolve storer-locally instead of taking the global-registry round trip per reference, which makes reference-dense stores faster than they were before this fix.internalStore) and eager applies, pins remain deliberately invisible to the lookup: there, a local hit means "this storer already handled the instance", and a pin hit would silently suppress the required (re-)serialization. An explicit recovery store (storer.storeAll(parent, child)) still re-serializes the pinned child instead of short-circuiting on the pin. This boundary is documented on both lookup methods; on the store side, the GigaMap suite exercises it heavily (a violation surfaces there as lost segment updates).synchRegisterItem) that constructs either anItemor aPinItem, and the pin deduplication reuses the lazy-apply lookup.Behavioral notes:
Scope and pairing
test/lazy-skip-pinning-73):StorerCommitWindowDanglingReferenceReproTest(the commit-window scenario end to end, red against serializer main, green with this branch) andSkipPinningTest(the four pin properties: retention during the window, release on commit, per-instance deduplication, and - guarding the commit-2 boundary at unit level - invisibility to explicit stores).