ocache: refuse to close an entry that is still loading (GO-7333) - #769
Open
requilence wants to merge 1 commit into
Open
ocache: refuse to close an entry that is still loading (GO-7333)#769requilence wants to merge 1 commit into
requilence wants to merge 1 commit into
Conversation
TryRemove marked a loading entry closing and then called TryClose on its value - which is nil until oCache.load publishes it. Three failure modes: a nil dereference; the entry left parked in closing behind a close channel nobody ever closes, stranding every later Get/Remove; and an unsynchronised read of e.value, written by load under c.mu. setClosing now refuses the loading state instead of transitioning it, and the two try-close paths act only on prevState == entryStateActive - the state in which the call actually acquired the transition. Bailing out after setClosing has run is not equivalent: the entry is already closing by then, and setActive(true) would make it active with a nil value for the next GC pass to dereference. Remove/RemoveSame/Close wait the load out in removeCtx before they get here, so they are unaffected: Close still cancels loads and closes the values they produce.
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
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.
Refs anytype-heart GO-7333. Follow-up to #746 (GO-7332), found while reviewing it.
Problem
TryRemovereachese.value.TryClose(c.ttl)on an entry that is still inentryStateLoading, wheree.valueis nil:entryStateLoadingis the zero value of the state (entry.go:14) and nothing bailed on it. An entry is loading fromnewEntry(id, nil, entryStateLoading)(ocache.go:150) untiloCache.loadpublishes the value (ocache.go:227), so the window is every in-flight load. Three failure modes:e.valueis a genuinely nilObject, so the method call panics — and it panics in the caller's goroutine, taking the process with it.setClosingflips the entry loading→closing and creates a freshe.close, bute.loadis closed only bydefer close(e.load)insideoCache.load, and the load's success path callssetActive(false), which does not closee.close. Any laterGet(waitClosefirst) parks on that channel forever.e.value.oCache.loadwrites it underc.mu(ocache.go:227);TryRemoveread it after releasingc.mu, holding neitherc.munore.mx.-raceon the new test reports exactly this pair before the fix.TryRemovehas no production callers inside any-sync (tests only), but anytype-heart calls it on a high-volume path —core/indexer/fulltext.goTryRemoveFromCacheafter every fulltext index of an object, pluscore/block/editor/layout/syncer.go— so the exposure is real, gated only on the object still being loaded elsewhere.Fix
setClosingrefuses the transition for a loading entry instead of performing it, and the two try-close paths (TryRemove,GC) act only whenprevState == entryStateActive— the state in which the call actually acquired the transition.Why the transition must be prevented, not undone or skipped. Adding
|| prevState == entryStateLoadingto the old bail condition does not fix the bug, it converts failure mode 1 into failure mode 2: by the time the caller seesprevState,setClosinghas already rune.state = entryStateClosing; e.close = make(chan struct{}), so the entry stays parked in closing with ane.loadthat is still open and ane.closenobody will close. Restoring withsetActive(true)is worse: it marks the entry active whilee.valueis still nil, so the very nextGCpass — which pre-filters one.isActive()— walks straight into the same nil dereference. Both variants were implemented and run against the new test: the naive bail-out fails on the stranded-Getsubtest (and leavesCloseeating its full timeout), and thesetActive(true)variant fails withGC panicked on a loading entry: invalid memory address or nil pointer dereference.The other two setClosing callers
removeCtx(ocache.go:268,wait=true; used byRemove,RemoveSame,Close) callse.waitLoadfirst, and<-e.loadis closed only after the load has either set the value and calledsetActiveor recordedloadErrand deleted the entry — so it never arrives here in the loading state. If it somehow did, it now getscurState != entryStateClosing→(false, nil), which is a safe refusal rather than a nilClose().Closestill cancels every in-flight load (e.cancelLoad()) and then closes the value the load produces; two new subtests pin that, one where the loadFunc ignores the cancellation and yields a value (it must be closed) and one where it honours it (Closemust not hang).GC(ocache.go:413) buildstoClosefrome.isActive()entries. The window between that check andsetClosingcannot expose a loading entry:entryStateLoadingis written in exactly one place,newEntry(ocache.go:150), and a removed entry is replaced by a fresh one rather than reset, so there is no active→loading edge. TheprevState != entryStateActivechange there is defensive and behaviourally identical for closing/closed; it is what keeps thesetActive(true)trap above from being reachable through GC.The GO-7332
for e.state == entryStateClosingloop is untouched — the new guard sits above it, and the loading state never enters that loop anyway (the loop's exit states are active/closed, never loading).Test
TestOCache_TryRemoveWhileLoading(app/ocache/ocache_test.go), deterministic — the loadFunc signals when it is in flight and is released by the test, in the style of the existing load-synchronising tests:refuses a loading entry and leaves the load running— mode 1:TryRemovereturns(false, nil)without panicking, the entry is stillentryStateLoadingafterwards, and the in-flightGetcompletes with its value.a Get arriving after the refused TryRemove is not stranded— mode 2: aGetissued afterTryRemovemust not park forever inwaitClose.GC never closes a loading entry— GC before and after a refusedTryRemove, catching thesetActive(true)restore variant.Close still closes an entry that was loading/Close does not hang on a load it aborted— the shared-behaviour change tosetClosingdoes not regress cache shutdown.TryRemove racing the value publication— mode 3: the load publishese.valuewhileTryRemovereads it, so-raceobserves the pair (100 iterations).Verification: