-
Notifications
You must be signed in to change notification settings - Fork 8.1k
fix: ensure each ProcessorSlotChain gets independent slot instances #3620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,9 +46,39 @@ public ProcessorSlotChain build() { | |
| continue; | ||
| } | ||
|
|
||
| chain.addLast((AbstractLinkedProcessorSlot<?>) slot); | ||
| // Always create a new instance of the slot to ensure each chain has its own | ||
| // copy with independent next pointers. This is necessary because singleton slots | ||
| // (isSingleton=true in @Spi annotation) are shared across all build() calls. | ||
| // If a singleton slot is directly used in multiple chains, modifying its 'next' | ||
| // pointer when building one chain will affect all other chains that use it. | ||
| // See: https://github.com/alibaba/Sentinel/issues/3007 | ||
| AbstractLinkedProcessorSlot<?> newSlot = newSlot(slot); | ||
| chain.addLast(newSlot); | ||
| } | ||
|
|
||
| return chain; | ||
| } | ||
|
|
||
| /** | ||
| * Create a new instance of the given slot. | ||
| * <p> | ||
| * This method creates a fresh instance of the slot's concrete class via reflection, | ||
| * ensuring that each chain gets its own independent copy of every slot. This prevents | ||
| * the issue where singleton slots shared across multiple chains would have their | ||
| * {@code next} pointer overwritten by the last chain built. | ||
| * </p> | ||
| * | ||
| * @param slot the slot to create a new instance of | ||
| * @return a new instance of the slot, or the original slot if creation fails | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Info] Retracted: my earlier comment on this line was incorrect — the Automated correction by github-manager-bot |
||
| private static AbstractLinkedProcessorSlot<?> newSlot(ProcessorSlot slot) { | ||
| try { | ||
| return slot.getClass().getDeclaredConstructor().newInstance(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Info] Retracted: my earlier comment was incorrect — the Automated correction by github-manager-bot |
||
| } catch (Exception e) { | ||
| RecordLog.warn("Failed to create new instance of ProcessorSlot(" | ||
| + slot.getClass().getCanonicalName() + "), using original instance", e); | ||
| return (AbstractLinkedProcessorSlot<?>) slot; | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using reflection (getDeclaredConstructor().newInstance()) for every slot on every build() adds overhead and will fail for slots whose no-arg constructor is not accessible. Although build() is invoked once per resource, this approach also bypasses any initialization or lifecycle guarantees provided by SpiLoader. |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The catch block returns the original SPI singleton instance when reflective construction fails. This silently reintroduces issue #3007 for that slot because its 'next' pointer is still shared across all ProcessorSlotChain instances, so one chain build will corrupt the topology of previously built chains. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a fresh instance of every slot per chain ignores the @SPI(isSingleton=true) contract. Built-in singleton slots (StatisticSlot, FlowSlot, DegradeSlot, SystemSlot, AuthoritySlot, LogSlot, DefaultCircuitBreakerSlot) hold no required shared instance state, but custom singleton slots that rely on shared instance-level state will be silently copied per chain, breaking their shared-state semantics.