fix: isolate ProcessorSlotChain links from shared SPI slot instances - #3646
Open
TwistedRiCen wants to merge 1 commit into
Open
fix: isolate ProcessorSlotChain links from shared SPI slot instances#3646TwistedRiCen wants to merge 1 commit into
TwistedRiCen wants to merge 1 commit into
Conversation
This was referenced Sep 4, 2026
TwistedRiCen
force-pushed
the
issue-3007-slot-context
branch
from
September 4, 2026 14:52
7a3d4ae to
28fbd0b
Compare
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
Fixes #3007.
Every slot inserted into a
DefaultProcessorSlotChainis now added through a chain-localProcessorSlotContextwrapper that owns the per-chainnextreference. The SPI-managed slot instance — singleton or prototype — is never linked into chain topology, so building one chain can no longer corrupt another.Problem
Slots annotated
@Spi(isSingleton = true)are loaded once and shared by every chain built fromDefaultSlotChainBuilder.addLast/addFirstlink nodes by writing the slot'snextfield:When a shared singleton slot is followed by a per-chain (prototype) slot, building a second chain overwrites the singleton's
next, so the first chain silently executes the second chain's successor:B → A1B → A2(setsB.next = A2)[B, A2]—A1is lost.This affects any deployment that registers custom slots via SPI where a singleton slot precedes a prototype slot (repro in #3007; also confirmed on current
1.8HEAD).Root cause
Chain topology (the
nextpointers) is stored on the slot instances themselves, but an instance's sharing scope (SPI singleton vs. prototype) is independent of a chain's scope. A shared singleton followed by a per-chain successor means pointer aliasing across chains.Design
Chain-local wrappers own the topology.
DefaultProcessorSlotChain#addFirst/addLastwrap each slot in aProcessorSlotContext(pass-through if it is already one) and link the wrappers — never the delegates. The SPI-loaded instance keeps its singleton semantics: it is shared, but it is no longer part of any chain's link structure.Scoped successor context for delegated propagation. Around each delegation, the wrapper activates a ThreadLocal
ChainContext(source = delegate, next = the wrapper's successor), restored infinally.AbstractLinkedProcessorSlot#getNext()resolves through the active context when it is the source, and falls back to the instance field otherwise. This makes the inheritedfireEntry()/fireExit()— and any custom override that propagates viagetNext()— continue down the calling chain instead of the delegate's stale field.Why not the alternatives
isSingleton = truecontract for custom slots that keep instance-level state; fails for non-public classes; bypassesSpiLoader's lifecycle; and any fallback to the shared instance re-introduces exactly this bug. See the review discussion there.fireEntry()propagates through its ownnextfield, so traversal stops at the first wrapped node unless fire-entry propagation is bridged — which is what the scoped successor context in this PR adds.Behavior preserved
DefaultProcessorSlotChain(directsetNext) behaves exactly as before: no context is active, sogetNext()returns the instance field.Trade-offs and limitations
slotChain.getNext()returnsProcessorSlotContext, soinstanceof-based node inspection must unwrap viagetDelegate()(DefaultSlotChainBuilderTestupdated accordingly).finally). A slot that firesfireEntry()/fireExit()asynchronously — e.g. on another thread, or after the delegation returns — would not see the context. Core slots propagate synchronously, but this is a semantic edge worth documenting.Testing
sentinel-core: full suite green — 228 tests, 0 failures, 0 errors, 0 skipped.mvn test(re-verified 2026-09-04 on current1.8HEAD): all shipped modules green — core, all extension/datasource modules, all adapters, transport, cluster, dashboard, logging, and demos. Two tests excluded as environmentally unavailable locally and unrelated to this change:ConsulDataSourceTest(requires Docker via Testcontainers) andNativeGreetingResourceIT(requires a pre-built GraalVM native image).DefaultProcessorSlotChainTestcases:Bwith per-chain successors; entry and exit order asserted; wrappers are distinct, delegate is the same shared instance;fireEntrywithin one slot;DefaultSlotChainBuilderTestextended: built-in slot order preserved through wrappers; prototype delegates differ per chain, singleton delegates are identical.Relationship to other PRs
fireEntry/fireExitpropagate correctly.