Conversation
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
July 23, 2022 22:15
940dca6 to
2f271e9
Compare
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
August 27, 2022 03:02
ac8e049 to
08b24cd
Compare
ben-manes
force-pushed
the
master
branch
17 times, most recently
from
September 10, 2022 01:17
a337c19 to
48d5246
Compare
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
September 26, 2022 02:50
53f222c to
00b1503
Compare
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
October 2, 2022 09:40
73f40d3 to
6abd616
Compare
ben-manes
force-pushed
the
master
branch
2 times, most recently
from
October 14, 2022 06:02
a7d31f5 to
d74756d
Compare
…lock The JSR-107 CacheWriter contract requires the non-batch writer methods to be "atomic with respect to the corresponding cache operation", but CacheProxy.remove(K)/getAndRemove(K) called writer::delete before an unconditional cache removal -- so a racing same-key put interleaved between the store delete and the cache removal, leaving the store's value while the cache went absent (a conformance violation; adversarial F21, distinct from the putIfAbsent/invoke ordering fixed earlier). removeNoCopyOrAwait now takes a publishToWriter flag and uses compute, firing the writer under the per-key bin lock atomically with the removal (mirroring putNoCopyOrAwait); it still fires unconditionally for an absent key. This matches the RI and every peer (Ehcache3 uses the same writer-inside-compute mechanism); only Coherence's in-process localcache shares the old window. removeAll stays on the batch deleteAll, which the spec explicitly does not require to be atomic. Honoring deleteAll's residual collection on a clean return (the RI's reading) would make removeAll a cache no-op for any writer that does not clear the collection -- every mock and most naive writers -- so remove-all-on-success is kept; the residual is honored on the throw path. A per-key delete loop is permitted but neither required nor more correct (Ehcache3 and Coherence-partitioned also batch). Pinned by a 2-thread interleaving test and a non-clearing-writer guard in CacheWriterTest. Catalogued in jsr107-conformance.md, with the write-through-under-compute convention in the adapter rule and the asyncReload refreshes-bin-lock context added to synchronization.md. Verified with :jcache:test and :jcache:tckTest.
CacheProxy recorded the operation duration on every write and delete path even when nothing was stored or removed, while the paired put/removal counter only moves on an actual mutation, so getAveragePutTime and getAverageRemoveTime were skewed high by no-op calls. Gate each timing call on the same condition as its counter, matching the two-arg replace(K,V), the iterator remove(), and the JSR-107 reference implementation.
putIfAbsent derived its hit/miss from whether a value was stored, so an absent key under a zero-creation-expiry -- where the new value is immediately expired and not stored -- recorded a hit instead of a miss. The JSR-107 statistics table counts a putIfAbsent miss whenever the key is absent, independent of whether anything was stored; getAndPut already classifies off prior-presence and gets this right. Thread prior-presence out of putIfAbsentNoAwait (a present flag set in the live-mapping branch) and classify the hit/miss by it, leaving the put and put-time gated on the store. Only the zero-creation-expiry configuration is affected; the normal, present, and expired-under-normal cases were already correct. Pinned by a strengthened JCacheCreationExpiryTest case asserting the absent zero-creation-expiry putIfAbsent records a miss, not a hit.
A refresh whose value or writeTime changed while in-flight takes the completion reject branch, which set only preserveTimestamps. remap then ran a by-key discardRefresh that removed a newer refresh's registration; the successor completed un-owned and its freshly loaded value was silently discarded, leaving the cache stale until the next refresh-eligible read. The refreshes map holds one future per key, so a by-key discard is safe only for the owner. Set preserveRefresh = !owned on the reject branch of all three completion paths (LocalLoadingCache.refresh, LocalAsyncLoadingCache.tryComputeRefresh, BoundedLocalCache .refreshIfNeeded), mirroring the already owner-scoped error path.
When a refresh completes while its entry is absent its compute returns null, and remap's absent-exit discarded the refresh by key — stealing a successor's registration so its loaded value was dropped and the entry stayed absent. Reachable in sync mode: refresh(k) on an absent key does an asyncLoad that registers without inserting the entry. Honor preserveRefresh at the absent exits (remap n==null and evicted-retire; unbounded absent) and set it in refreshIfNeeded's absent branch, so a non-owning stale completion leaves the successor intact.
A refresh of an absent key registers a reload in refreshes with no data-map node, so invalidate(k)/clear() — which reach discardRefresh only through a present-node path — left it alive; the completing reload then re-inserted the value, resurrecting the key past the purge (sync-only, since async's refresh-of-absent is a physical entry). remove(Object) now uses data.compute so an absent key enters the lambda under the bin lock and discards there, serializing against the reload completion (which commits under the same lock); doing it after a computeIfPresent miss would race the completion. clear() purges the refreshes map up front. remove(k,v) is unchanged: a conditional remove that matched nothing preserves the refresh (query-style no-op). Also carries the .claude design-decision notes for the refresh owner-scoping fix (#1) and the sync-view coalescing adjudication (#2).
UnboundedLocalCache.remap's blanket catch discarded a racing refresh even when the user function threw on an absent key — but the compute created nothing, so no mapping was made, and remap-as-completion never throws there (its own lambda can't throw before materialization). BoundedLocalCache already preserves it: its absent branch runs the function before the create finally, so a throw propagates without a discard. Narrow ULC's catch to discard only for a present entry (value != null), matching BLC's synchronized-branch catch and the pinned "a throw is an aborted op with no verdict, and an independent in-flight refresh may still legitimately populate the absent key" doctrine. Also corrects the design-decisions "remap discards on every exit" note, inaccurate for this path.
A sync refresh(k) on an absent key registers the reload only in refreshes() — invisible to a concurrent get, which loads again — while the async view inserts a first-class in-flight entry a get can join. This sync/async divergence is structurally forced (a sync cache cannot join an invisible in-progress computeIfAbsent) and intentional; document it (A2-F1a) and pin the dup-load with a latch-gated test: sync loads twice, async once. No user-facing javadoc change. Also add a discardRefresh-across-the-compute-family reference table to design-decisions, consolidating the per-verdict rules just verified consistent across both siblings.
CaffeinatedGuavaLoadingCache.asyncReload's catch (Throwable) completed the future exceptionally but dropped the thread's interrupt status, unlike the synchronous load path (and native Guava), which restore it. Re-interrupt on InterruptedException before completing.
WriteBehindCacheWriter's subscription caught only RuntimeException from the write action, so an Error (e.g. AssertionError) propagated to the terminal error consumer and disposed the subscription — silently dropping every subsequent write. Catch Throwable so one failing write action cannot tear down the pipeline.
The jcache access-expiry paths ran policy().setExpiresAfter (which calls afterWrite) from inside their own cache.asMap().compute* lambdas on the failed-conditional remove/replace and invoke-READ branches, violating Cache.policy(): under the bin lock afterWrite can block on the eviction lock (ABBA) or run maintenance inline (a nested same-bin CHM mutation). On those write paths the reschedule is redundant: the enclosing compute's Expiry (ExpirableToExpiry, which derives the deadline from the Expirable timestamp) refreshes the native timer on commit, and expireAfterUpdate fires even on a same-instance return. Split setAccessExpireTime into getAccessExpireTime (eval), setAccessExpireTime (write the timestamp, all paths), and setVariableExpiration (poke the native timer, reads only); write paths now write only the timestamp and let the compute refresh it. Add native-timer regression tests for the three write paths; the prior mismatch/invoke tests asserted only the Expirable field, leaving the native timer on write paths untested.
Records the rationale for three jcache read/load semantics so future audits don't re-raise them: - Read-through getAll clobbers an entry that materializes during the load (Caffeine bulkLoad's put). Intentional: the loaded value is the freshest SoR read and always fires CREATED, so a resource-tracking listener is notified; putIfAbsent would silently drop the load. - close() shuts down the executor when it is an ExecutorService. Factory<Executor> means each cache creates and owns its executor; the default commonPool ignores shutdown, and a shared executor is passed as a plain Executor (the instanceof gate then skips shutdown). - Read-through loadAll stores the loader-returned key uncopied under store-by-value. Not a violation: the application-boundary keys are already copied (getAll in, copyMap/EntryProxy out), so the stored key is internal and never reaches the application uncopied.
postProcess fired CacheWriter.write before copying the processor-set value on the CREATED and UPDATED paths, so a store-by-value Copier failure left the store written but the cache unchanged. Hoist the copy above the writer (matching the put path, which copies before the compute), so a Copier throw aborts with neither side written. The writer still receives the entry, so writer-mutation-visibility also matches put.
A native refresh (or a loadAll past the 10s await, or invoke) in flight at close() can invoke the closed loader/ExpiryPolicy afterward. JCache close() is not an atomic gate (spec silent on in-progress operations), the straggler fails gracefully (logged, recordLoadFailure), and it never resurrects (invalidateAll purges the refreshes map). Awaiting would stall close() on the user's executor. Extends the racing-close note.
EventDispatcher.publish's executor-rejection legs are the accepted M3 racing-close divergence, and the sync-pending ThreadLocal leak needs the executor to flip accepting to rejecting mid-loop to occur at all. The bulk ops getAll and putAll already wrap their loop in a try..finally so awaitSynchronous clears pending even when the loop throws, but removeAll(Set) awaited outside any finally -- bring it into line with putAll so a mid-loop throw still clears the queued synchronous futures. Documents the leg analysis in the racing-close (M3) note.
Six no-defect jcache notes: - The per-key dispatch-queue slot (compute-append vs whenComplete-remove) is race-safe without extra locking: compute is atomic (CHM bin lock) so a completing future's cleanup can't interleave the chain-append, and cleanup is a conditional remove(key, future) that fires only if that future is still the head. No lost/reordered event and no leaked slot. - Listener registrations dedup by MutableCacheEntryListenerConfiguration field-equality (Registration wraps the config in an immutable MCELC and both register and deregister key by it), not the caller's config equals. Two field-equal but custom-equals-distinct configs register once, not twice. Intentional and the more spec-defensible reading: it dedups an accidental double-register, gives a stable mutation-immune key, and only diverges from the RI on absurdly contrived input. - putAll/removeAll fire the batch writer (writeAll/deleteAll) once up front, then per-key computes mutate with publishToWriter=false, so a racing same-key single-key op can invert cache vs store across the batch window. Spec-sanctioned: the CacheWriter contract exempts batch methods from atomicity, and the single-key remove/put serialization fixes deliberately do not extend to the batch ops. Symmetric to the already-catalogued removeAll(Set) decision; putAll now named too. - The read-path access-expiry native poke (setExpiresAfter, by key, kept lock-free for read throughput) has no identity guard: a concurrent write replacing the entry between the lock-free capture and the poke lands the access duration on the new entry. Never a stale read (the per-wrapper Expirable.expireTimeMillis gates every lazy read); at worst the new entry is eagerly evicted slightly early with a phantom EXPIRED, an accepted concurrency edge. Extra-benign because ExpiryPolicy is parameterless, so the misapplied duration is the one the policy would assign anyway (zero deviation for standard Accessed/Touched policies). Documents the existing "benign lock-free race" note with its concrete worst case. - A Duration.ZERO creation expiry suppresses the store/CREATED/put-stat but NOT write-through: CacheWriter.write fires before the creation-expiry guard on every write path, so the SoR persists the value though the cache stores nothing. RI parity -- RICache calls writeCacheEntry unconditionally before getExpiryForCreation -- and the defensible reading (write-through persists the caller's write intent). Fills the gap in the zero-creation catalogue entry, which named the suppressions but not the writer. - Store-by-value events hand listeners the caller's uncopied key too, not just the stored value instance the catalogue already noted. Benign for the key (the cache's own stored key copy is never exposed, so a listener mutating event.getKey() can't corrupt the lookup -- only intra-app aliasing), where the value hands out the stored instance. Same spec-silent, TCK-blind, per-event-copy-taxes-every-op rationale; extends the catalogue entry to name the key.
JSR-107 requires a synchronous CacheEntryListener exception to propagate to the caller, wrapped as a CacheEntryListenerException (javax.cache.event package-info: "this will propagate back to the caller"; CacheEntryListener: "catch any other Exception ... wrap and rethrow"). The EventDispatcher swallowed it: EventTypeAwareListener.dispatch caught and logged, so the synchronous future completed normally and awaitSynchronous never saw the failure. Now dispatch returns the exception (a CacheEntryListenerException as-is, or a RuntimeException wrapped in one; an Error is logged and rethrown as-is, not wrapped, matching the RI which catches only Exception); the per-key chain future carries it as its result -- so a throwing listener never fails the future and same-key ordering is preserved -- and awaitSynchronous throws the first (extras addSuppressed). An asynchronous or quiet (background refresh reload / native eviction) failure is logged, not propagated (no synchronous caller); an executor-level failure surfaces as a CacheEntryListenerException, and a listener Error propagates as-is. The ecosystem is split (all source-verified): spec + RI + cache2k + Hazelcast + Infinispan propagate; Ehcache 3 and Coherence -- the two executor-dispatch implementations, as Caffeine was -- swallow and log. Followed the spec and the RI. Pinned by EventDispatcherTest (sync propagates and wraps, async swallowed, same-key chain survives a throw, multiple exceptions suppressed) and EventTypeAwareListenerTest. Catalogued in jsr107-conformance.md, whose filter-exception entry is corrected: a filter runs inside the compute so it must swallow, while the listener runs after commit so it can propagate.
postProcess handed CacheWriter.write the live EntryProcessorEntry on the CREATED and UPDATED paths — the only write-through site not passing an immutable EntryProxy. A CacheWriter that downcasts its Cache.Entry to MutableEntry could mutate the entry the cache is committing; a CREATED remove() flips the action to NONE and skips recordPuts. Wrap the uncopied processor value in an EntryProxy, matching put/replace, so the writer sees the same value but cannot perturb the create/update.
A CaffeineConfiguration extension Weigher or native Expiry runs in core after the adapter's compute fn has already published CREATED/UPDATED and fired the write-through writer, so a throw aborts the store while the notification stands. It's misuse (weigher/Expiry must not throw; the standard ExpiryPolicy surface is already guarded), can't be made atomic with the notification, and the notification is valid anyway — the value was created/updated, just not persisted. Record it as by-design.
The static default loader captured the class-init (OSGi activation) context classloader as its JCacheClassLoader parent, a strong static reference that pins an undeployed webapp/bundle loader under a shared provider jar. loadClass already prefers the live TCCL dynamically, so the snapshot is only a last-resort fallback (added for OSGi, #447). Hold it in a WeakReference instead: the loader stays collectible on undeploy, and the fallback resolves only while it remains alive. The super parent is now null.
CacheProxy.getWriteExpireTimeMillis returned Long.MIN_VALUE for a null duration on both the create and update paths, but MIN_VALUE is the "keep the prior deadline" sentinel — meaningful only for an update. A create has no prior, so the create arms stored the raw sentinel as the deadline: not eternal, and (via the native MIN_VALUE - ticker deadline) born-expired under some clocks. Return eternal for a null creation duration, matching this method's own catch block and the loader adapter.
JCacheLoaderAdapter.reload publishes UPDATED for a replacement and EXPIRED for a zero-expiry reload, but returned null on a null reload (the system-of-record row is gone) with no event, so the refresh dropped the entry silently: the eviction listener only sees automatic removals (SIZE/EXPIRED/COLLECTED), not this EXPLICIT one. Publish REMOVED so a listener sees present -> REMOVED -> absent.
EntryIterator.remove gated removal on the value still equaling the one returned by next(), so a replacement between next() and remove() made it a silent no-op (no removal, no REMOVED, no CacheWriter.delete). No JCache provider value-gates. Routing through remove(K) also gates on close and fires EXPIRED for an entry that expires in the next() / remove() window, matching them; only the RI keeps an inline unconditional removal.
loadAll's worker published CREATED/UPDATED, discarded the pending synchronous-listener futures (ignoreSynchronous), then called onCompletion -- so a sync listener could complete after onCompletion, and a throwing one was swallowed. Instead chain onCompletion after the listeners via a new EventDispatcher.chainSynchronous() that drains and returns their futures; a listener exception routes to onException (a CacheException, allowed by loadAll's contract). Chained not awaited so a single-thread executor can't self-deadlock. awaitSynchronous now joins the same future, sharing the reduction logic.
LoadingCacheProxy.getOrLoad recorded the miss after cache.get runs the read-through load, so a throwing loader propagated before the miss was counted -- getCacheMisses stayed 0. The loading getAll, the RI (increaseCacheMisses before cacheLoader.load), and core all count the miss regardless of load outcome (the find already failed). Record it before the load. Not TCK-observable (its stats tests use no loader). Pin untested jcache close-gate and config paths Add coverage for public-API paths the TCK never exercises: clear() and register/deregisterCacheEntryListener on a closed cache throw ISE (CG-1, CG-2); createCache of a name defined in application.conf throws CacheException (CG-6); and createCache with a plain non-Complete Configuration (types + store-by-value only) succeeds via the basic resolveConfigurationFor branch (CG-7). Test-only; the code was already correct.
Test-only pins on already-correct behavior plus catalogue notes; no production change. Closes the jcache coverage-pin backlog. Store-by-value copy points (CacheProxyTest): putIfAbsent/getAndReplace store copies, iterator returned-key copy, invoke prior-value isolation, and bulk read-through value copy; store-by-reference identity for iterator/invoke/getAndRemove. loadAll(replaceExistingValues=true) on a present key applies the update expiry and fires UPDATED (JCacheUpdateExpiryTest); a bulk read-through dispatches to the loader's loadAll, not per-key load; the writer's own CacheWriterException propagates unwrapped from writeAll/deleteAll (CacheWriterTest). getAndPut under a zero creation expiry records a miss-not-hit (JCacheCreationExpiryTest); the EventDispatcher dedups distinct but field-equal listener configurations (EventDispatcherTest). jsr107-conformance.md: catalogue the RI-parity/spec-defensible corners P1/P2/P5-P12 (quiet event bridges, zero-creation writer, clear over expired, HOCON storeByValue default, enable* no-op, EP corners, duplicate EXPIRED, removeAll snapshot, maximumWeight ISE) and extend the ObjectName and LOADED catalogue entries.
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
6 participants
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.
No description provided.