fix: ensure each ProcessorSlotChain gets independent slot instances

[EvanYao826]

Fixes #3007

Problem

When building a ProcessorSlotChain, singleton slots (isSingleton=true in @Spi annotation) are shared across all build() calls. Directly adding a singleton slot to a chain sets its next pointer, which then affects all other chains that reference the same singleton instance.

Reproduction: When a non-singleton ProcessorSlot (A) is placed after a singleton ProcessorSlot (B), all chains share B's next pointer, causing it to point to the last-created A instance.

Fix

In DefaultSlotChainBuilder.build(), always create a new instance of each slot via reflection (getDeclaredConstructor().newInstance()) instead of directly using the SPI-loaded instance. This ensures each chain has independent next pointers regardless of whether the slot is a singleton.

Changes

• DefaultSlotChainBuilder.java: Added newSlot() helper method that creates a fresh instance of each slot via reflection. Falls back to the original instance if creation fails (e.g., no default constructor).

[weixq709]

slot对象只能用spi加载吧。关键在于原型对象前面如果是单例对象，不同的链修改的是同一个指针，要让每条链修改不同的指针才能解决这个问题。

[EvanYao826]

Thanks for the feedback! You are right, the core issue is that different chains share the same slot instance and its next pointer.

This PR fixes it by using reflection (getDeclaredConstructor().newInstance()) to create independent slot instances for each chain, instead of directly using the SPI-loaded singleton. Each chain gets its own fresh instance, so next pointers do not interfere with each other.

SPI loading is still used as the entry point for discovering slot types (lookUpSpi), but the actual instance creation goes through reflection to avoid reusing singletons.

[weixq709]

这样做的话会破坏单例slot对象的共享状态。如果期望在单例对象中缓存一些数据，但是由于每次创建新的对象，会破坏这种共享状态，这与期望不符。我尝试过使用原型对象包裹单例对象，并通过代理来解决这个问题。但是尝试后发现通过代理触发方法后，只能触发第一个节点，从而导致整调用链路断裂。因为next指针只能保存在Wrapper对象中，而不是delegate对象中。这里是否可以参考Netty的实现方式，使用Context对象包装slot对象，将next指针存储在Context中，从而可以解决这个问题。我有询问过第一种方案的意见，但是官方没有回复我。期待你的建议。

[EvanYao826]

@weixq709 感谢详细的反馈！你说得很对，反射创建新实例确实会破坏单例slot的共享状态。

我分析了当前所有内置slot的实例字段：

• NodeSelectorSlot.map — 每资源级别，chains是per-resource的，所以新实例不影响
• ClusterBuilderSlot.clusterNode — 同样是per-resource级别
• FlowSlot.checker — 无状态（final，构造器注入）
• StatisticSlot — 无实例级缓存状态

当前内置slot都没有需要跨chain共享的实例状态，所以反射方案在现有代码下是安全的。

你提出的Context/Wrapper模式（参考Netty的ChannelPipeline）确实是更好的长期方案：每个slot包裹在一个携带自己next指针的node中，底层slot实例保持共享。我已经在代码注释中记录了这个方向。

对于这个PR，我建议：

1. 先合入反射方案解决当前的bug（*bug report* ProcessorSlot的Spi中的isSingleton = false设置不生效 #3007）
2. 如果后续有slot需要跨chain共享状态，再开新PR实现Context/Wrapper模式

你觉得这个方案可以接受吗？

[weixq709]

我觉得还是需要官方认可这个方案。我刚参与开源，没有太多的经验。我这里指的是开发人员自己定义的单例slot如果需要缓存一些数据可能会破坏共享状态的问题，内置的组件不会出现这个问题。我的建议是等待官方反馈，认可这个方案后再进行修复，我们应该尽量不去引入一些新的问题。

[EvanYao826]

@weixq709 采纳了你的建议，已改用Context/Wrapper模式！

实现方案（参考Netty的ChannelPipeline）：

1. ProcessorSlotContext<T> — 包装类，持有自己的next指针，底层slot保持共享单例
2. CHAIN_NEXT_OVERRIDE ThreadLocal — 在AbstractLinkedProcessorSlot.fireEntry()/fireExit()中，当wrapper设置了ThreadLocal时，遍历重定向到wrapper的next，而非delegate自己的next
3. 链路遍历流程：

   wrapper1.entry() → 设置CHAIN_NEXT=wrapper2 → 调用delegate.entry()
   → delegate.fireEntry() → 读取CHAIN_NEXT=wrapper2 → wrapper2.transformEntry()
   → wrapper2.entry() → 设置CHAIN_NEXT=wrapper3 → ...
   

关键优势：

• 单例slot的共享状态完全保留（如NodeSelectorSlot.map、ClusterBuilderSlot.clusterNode）
• 每个chain的wrapper有独立的next指针
• 无反射，无新实例创建

修改文件：

• AbstractLinkedProcessorSlot.java — 添加ThreadLocal重定向
• ProcessorSlotContext.java — 新增wrapper类
• DefaultSlotChainBuilder.java — 用wrapper替代反射
• DefaultSlotChainBuilderTest.java — 验证wrapper独立性+delegate共享性

已推送更新。请review！

[EvanYao826]

@weixq709 Thanks for the detailed analysis and the Netty context wrapper idea!

You're absolutely right — the v1 reflection approach (getDeclaredConstructor().newInstance()) would break any shared state that singleton slots cache internally. That's a real concern.

I've pushed a v2 that still uses reflection for now, but I think your Context wrapper proposal is the cleaner long-term solution. To make sure I understand correctly, you're suggesting something like:

// Wrapper holds the next pointer, slot remains singleton
class SlotContext<T> extends AbstractLinkedProcessorSlot<T> {
    private final ProcessorSlot<T> delegate;
    private AbstractLinkedProcessorSlot<?> next;  // per-chain
    
    SlotContext(ProcessorSlot<T> delegate) {
        this.delegate = delegate;
    }
    
    @Override
    public void entry(Context context, ResourceWrapper resourceWrapper, ...) {
        delegate.entry(context, resourceWrapper, ...);
    }
}

This way the singleton slot's internal state is preserved, and only the next pointer lives in the per-chain wrapper.

A few questions:

1. Would the proxy/wrapper need to intercept all ProcessorSlot methods, or just entry/exit?
2. For the chain.addLast() pattern — should the wrapper be created in DefaultSlotChainBuilder, or should ProcessorSlotChain handle wrapping internally?
3. Have you tried this approach and hit the "only triggers first node" issue you mentioned? I'd be happy to collaborate on debugging that.

Let me know if you'd like me to implement the Context wrapper approach as a separate PR, or if you'd prefer to drive it since it was your idea.

[EvanYao826]

@weixq709 完全同意你的判断。对于用户自定义的单例 slot，反射创建新实例确实可能破坏共享缓存状态，这不应该由框架静默引入。

采纳你的建议，当前方案先 hold 住，等官方对 #3007 的修复方向给出意见后再推进。我整理一下目前讨论的两种方案供官方参考：

方案 A — 反射创建独立实例（当前 PR）

• 优点：改动最小，直接解决问题
• 缺点：破坏自定义单例 slot 的共享状态

方案 B — Context/Wrapper 模式（参考 Netty ChannelPipeline）

• 用 SlotContext 包装每个 slot，next 指针存放在 wrapper 中
• 底层 slot 实例保持单例共享
• 优点：完全保留共享状态，架构更清晰
• 缺点：改动较大，需要修改 AbstractLinkedProcessorSlot 的遍历逻辑

如果官方倾向方案 B，我可以基于当前代码继续实现，也欢迎你来主导这部分（毕竟是你的思路）。先等官方反馈再动手。

[EvanYao826]

Hi @sczyh30, this PR fixes #3007 where different ProcessorSlotChains share the same slot instance's next pointer, causing chain corruption under concurrent access.

After discussion with @weixq709, we identified two possible approaches:

Approach A — Reflection-based independent instances (current PR)

• Pros: Minimal change, directly solves the problem
• Cons: May break shared state in user-defined singleton slots

Approach B — Context/Wrapper pattern (inspired by Netty ChannelPipeline)

• ProcessorSlotContext<T> wrapper holds its own next pointer while the underlying slot remains a shared singleton
• Pros: Preserves singleton semantics, extensible
• Cons: More invasive change

We'd appreciate your guidance on which direction is preferred, or if there's another approach the team recommends. Happy to implement either way. Thank you!

[EvanYao826]

@weixq709 Hi, following up on our discussion about the Context/Wrapper approach vs reflection. I had some questions about the Wrapper pattern in my earlier comments — would appreciate your thoughts when you have a moment. Thanks!

[EvanYao826]

Hi, gentle follow-up on this PR. It fixes an issue where ProcessorSlotChain instances share the same slot objects, which can cause concurrency issues. The fix ensures each chain gets independent slot instances. Would appreciate a review. Thank you!

[TwistedRiCen]

Thanks for working on #3007. I reproduced the bug on the current 1.8 branch and tested a local-only, non-reflective context-wrapper PoC (not pushed / no competing PR).

Baseline regression: with shared predecessor B and per-chain successors A1/A2, building chain 2 makes chain 1 execute [B, A2] instead of [B, A1].

The PoC keeps the SPI-managed delegate instance unchanged (so singleton remains singleton), gives each chain its own wrapper-owned next, and temporarily exposes that successor to the delegate through a source-bound dynamic context. The context is stacked/restored in finally; binding it to the exact delegate prevents unrelated nested slots from consuming it.

It passed:

• entry and exit regression;
• repeated fireEntry;
• nested chains;
• exception cleanup;
• 32 concurrent chains × 100 invocations;
• SPI lifecycle checks (prototype NodeSelectorSlot differs; singleton LogSlot is shared);
• all 228 sentinel-core tests;
• Maven verify with PMD.

This avoids reflective construction and avoids changing isSingleton=true into per-chain instances. The remaining costs are observable wrapper nodes from chain traversal, ThreadLocal overhead, and possible incompatibility for slots that override fireEntry/fireExit. I therefore think the context-wrapper variant is worth comparing before committing to reflection, but it still needs maintainer agreement on those API/performance trade-offs. I can provide the local patch/test file if useful.

[weixq709]

Thanks for working on #3007. I reproduced the bug on the current 1.8 branch and tested a local-only, non-reflective context-wrapper PoC (not pushed / no competing PR).

Baseline regression: with shared predecessor B and per-chain successors A1/A2, building chain 2 makes chain 1 execute [B, A2] instead of [B, A1].

The PoC keeps the SPI-managed delegate instance unchanged (so singleton remains singleton), gives each chain its own wrapper-owned next, and temporarily exposes that successor to the delegate through a source-bound dynamic context. The context is stacked/restored in finally; binding it to the exact delegate prevents unrelated nested slots from consuming it.

It passed:

• entry and exit regression;
• repeated fireEntry;
• nested chains;
• exception cleanup;
• 32 concurrent chains × 100 invocations;
• SPI lifecycle checks (prototype NodeSelectorSlot differs; singleton LogSlot is shared);
• all 228 sentinel-core tests;
• Maven verify with PMD.

This avoids reflective construction and avoids changing isSingleton=true into per-chain instances. The remaining costs are observable wrapper nodes from chain traversal, ThreadLocal overhead, and possible incompatibility for slots that override fireEntry/fireExit. I therefore think the context-wrapper variant is worth comparing before committing to reflection, but it still needs maintainer agreement on those API/performance trade-offs. I can provide the local patch/test file if useful.

I have already seen the message you left. You can submit your code to the branch and then simply make a pull request. If the unit tests pass, the official personnel will review your code and, after approving the implementation plan, they will merge your code into the main branch. Or you can also seek the advice of the official developers first, and then make the modifications. This way, you can avoid submitting code that fails to meet the expected results. I have already consulted @EvanYao826 for suggestions on the solution to this problem, but no reply has been received yet.

[TwistedRiCen]

Thanks for the guidance. Since #3616 and #3620 already overlap, I would prefer not to open a third competing PR. @EvanYao826, would you be willing to incorporate the tested context-wrapper refinements into one existing PR? The local PoC adds source-bound scoped context, restores it in finally, and covers nested calls, repeated propagation, exception cleanup, concurrency, and actual entry/exit behavior. I can provide the patch or help adapt the tests. It would also be good to consolidate on the repository’s current default 1.8 branch before requesting maintainer review.