From be43a31dc419c5de87b941f5b2752bc1bc2314ce Mon Sep 17 00:00:00 2001 From: Eric Zhao Date: Wed, 19 Sep 2018 14:01:07 +0800 Subject: [PATCH] Fix internal bug when amount of context exceeds the threshold (#152) - When amount of context exceeds the threshold, the `NullContext` will be set to current thread local. Thus, when checking context in `CtSph#entry`, once `NullContext` detected, the entry will not do rule checking on the slot chain. - Cache the default context during initialization. Then when amount of context exceeds the threshold, entries under default context can do rule checking under default context. - Enhance the error message Signed-off-by: Eric Zhao --- .../com/alibaba/csp/sentinel/Constants.java | 2 +- .../com/alibaba/csp/sentinel/CtEntry.java | 17 +++++- .../java/com/alibaba/csp/sentinel/CtSph.java | 7 ++- .../alibaba/csp/sentinel/context/Context.java | 11 ++++ .../csp/sentinel/context/ContextUtil.java | 26 ++++++++- .../csp/sentinel/context/NullContext.java | 3 +- .../csp/sentinel/context/ContextTest.java | 53 ++++++++++++++++++- 7 files changed, 110 insertions(+), 9 deletions(-) diff --git a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/Constants.java b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/Constants.java index 311f932b..5e0a104b 100755 --- a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/Constants.java +++ b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/Constants.java @@ -31,7 +31,7 @@ public class Constants { public final static int MAX_CONTEXT_NAME_SIZE = 2000; public final static int MAX_SLOT_CHAIN_SIZE = 6000; public final static String ROOT_ID = "machine-root"; - public final static String CONTEXT_DEFAULT_NAME = "default_context_name"; + public final static String CONTEXT_DEFAULT_NAME = "sentinel_default_context"; public final static DefaultNode ROOT = new EntranceNode(new StringResourceWrapper(ROOT_ID, EntryType.IN), Env.nodeBuilder.buildClusterNode()); diff --git a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/CtEntry.java b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/CtEntry.java index b01f983c..6bc2d42a 100644 --- a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/CtEntry.java +++ b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/CtEntry.java @@ -17,6 +17,7 @@ package com.alibaba.csp.sentinel; import com.alibaba.csp.sentinel.context.Context; import com.alibaba.csp.sentinel.context.ContextUtil; +import com.alibaba.csp.sentinel.context.NullContext; import com.alibaba.csp.sentinel.node.Node; import com.alibaba.csp.sentinel.slotchain.ProcessorSlot; import com.alibaba.csp.sentinel.slotchain.ResourceWrapper; @@ -44,6 +45,10 @@ class CtEntry extends Entry { } private void setUpEntryFor(Context context) { + // The entry should not be associated to NullContext. + if (context instanceof NullContext) { + return; + } this.parent = context.getCurEntry(); if (parent != null) { ((CtEntry)parent).child = this; @@ -58,14 +63,21 @@ class CtEntry extends Entry { protected void exitForContext(Context context, int count, Object... args) throws ErrorEntryFreeException { if (context != null) { + // Null context should exit without clean-up. + if (context instanceof NullContext) { + return; + } if (context.getCurEntry() != this) { + String curEntryNameInContext = context.getCurEntry() == null ? null : context.getCurEntry().getResourceWrapper().getName(); // Clean previous call stack. CtEntry e = (CtEntry)context.getCurEntry(); while (e != null) { e.exit(count, args); e = (CtEntry)e.parent; } - throw new ErrorEntryFreeException("The order of entry free is can't be paired with the order of entry"); + String errorMessage = String.format("The order of entry exit can't be paired with the order of entry" + + ", current entry in context: <%s>, but expected: <%s>", curEntryNameInContext, resourceWrapper.getName()); + throw new ErrorEntryFreeException(errorMessage); } else { if (chain != null) { chain.exit(context, resourceWrapper, count, args); @@ -76,7 +88,8 @@ class CtEntry extends Entry { ((CtEntry)parent).child = null; } if (parent == null) { - // Auto-created entry indicates immediate exit. + // Default context (auto entered) will be exited automatically. + // Note: NullContext won't be exited automatically. ContextUtil.exit(); } // Clean the reference of context in current entry to avoid duplicate exit. diff --git a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/CtSph.java b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/CtSph.java index 18e38693..8749d656 100755 --- a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/CtSph.java +++ b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/CtSph.java @@ -56,7 +56,8 @@ public class CtSph implements Sph { private AsyncEntry asyncEntryInternal(ResourceWrapper resourceWrapper, int count, Object... args) throws BlockException { Context context = ContextUtil.getContext(); if (context instanceof NullContext) { - // Init the entry only. No rule checking will occur. + // The {@link NullContext} indicates that the amount of context has exceeded the threshold, + // so here init the entry only. No rule checking will be done. return new AsyncEntry(resourceWrapper, null, context); } if (context == null) { @@ -112,7 +113,8 @@ public class CtSph implements Sph { public Entry entry(ResourceWrapper resourceWrapper, int count, Object... args) throws BlockException { Context context = ContextUtil.getContext(); if (context instanceof NullContext) { - // Init the entry only. No rule checking will occur. + // The {@link NullContext} indicates that the amount of context has exceeded the threshold, + // so here init the entry only. No rule checking will be done. return new CtEntry(resourceWrapper, null, context); } @@ -142,6 +144,7 @@ public class CtSph implements Sph { e.exit(count, args); throw e1; } catch (Throwable e1) { + // This should not happen, unless there are errors existing in Sentinel internal. RecordLog.info("Sentinel unexpected exception", e1); } return e; diff --git a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/Context.java b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/Context.java index d970fb52..cd04899a 100755 --- a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/Context.java +++ b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/Context.java @@ -187,4 +187,15 @@ public class Context { public Node getOriginNode() { return curEntry == null ? null : curEntry.getOriginNode(); } + + @Override + public String toString() { + return "Context{" + + "name='" + name + '\'' + + ", entranceNode=" + entranceNode + + ", curEntry=" + curEntry + + ", origin='" + origin + '\'' + + ", async=" + async + + '}'; + } } diff --git a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/ContextUtil.java b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/ContextUtil.java index 752ea34e..fab8e4cb 100755 --- a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/ContextUtil.java +++ b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/ContextUtil.java @@ -49,13 +49,35 @@ public class ContextUtil { private static ThreadLocal contextHolder = new ThreadLocal(); /** - * Holds all {@link EntranceNode} + * Holds all {@link EntranceNode}. */ private static volatile Map contextNameNodeMap = new HashMap(); private static final ReentrantLock LOCK = new ReentrantLock(); private static final Context NULL_CONTEXT = new NullContext(); + static { + // Cache the entrance node for default context. + initDefaultContext(); + } + + private static void initDefaultContext() { + String defaultContextName = Constants.CONTEXT_DEFAULT_NAME; + EntranceNode node = new EntranceNode(new StringResourceWrapper(defaultContextName, EntryType.IN), null); + Constants.ROOT.addChild(node); + contextNameNodeMap.put(defaultContextName, node); + } + + /** + * Not thread-safe, only for test. + */ + static void resetContextMap() { + if (contextNameNodeMap != null) { + contextNameNodeMap.clear(); + initDefaultContext(); + } + } + /** *

* Enter the invocation context. The context is ThreadLocal, meaning that @@ -99,6 +121,7 @@ public class ContextUtil { DefaultNode node = localCacheNameMap.get(name); if (node == null) { if (localCacheNameMap.size() > Constants.MAX_CONTEXT_NAME_SIZE) { + contextHolder.set(NULL_CONTEXT); return NULL_CONTEXT; } else { try { @@ -106,6 +129,7 @@ public class ContextUtil { node = contextNameNodeMap.get(name); if (node == null) { if (contextNameNodeMap.size() > Constants.MAX_CONTEXT_NAME_SIZE) { + contextHolder.set(NULL_CONTEXT); return NULL_CONTEXT; } else { node = new EntranceNode(new StringResourceWrapper(name, EntryType.IN), null); diff --git a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/NullContext.java b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/NullContext.java index 9c68be20..e07fea75 100755 --- a/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/NullContext.java +++ b/sentinel-core/src/main/java/com/alibaba/csp/sentinel/context/NullContext.java @@ -27,7 +27,6 @@ import com.alibaba.csp.sentinel.Constants; public class NullContext extends Context { public NullContext() { - super(null, null); + super(null, "null_context_internal"); } - } diff --git a/sentinel-core/src/test/java/com/alibaba/csp/sentinel/context/ContextTest.java b/sentinel-core/src/test/java/com/alibaba/csp/sentinel/context/ContextTest.java index 09656a61..61dda04d 100755 --- a/sentinel-core/src/test/java/com/alibaba/csp/sentinel/context/ContextTest.java +++ b/sentinel-core/src/test/java/com/alibaba/csp/sentinel/context/ContextTest.java @@ -15,7 +15,11 @@ */ package com.alibaba.csp.sentinel.context; +import com.alibaba.csp.sentinel.Constants; +import com.alibaba.csp.sentinel.TestUtil; + import org.junit.After; +import org.junit.Before; import org.junit.Test; import static org.junit.Assert.*; @@ -28,9 +32,56 @@ import static org.junit.Assert.*; */ public class ContextTest { + @Before + public void setUp() { + resetContextMap(); + } + @After public void cleanUp() { - ContextUtil.exit(); + TestUtil.cleanUpContext(); + } + + @Test + public void testEnterCustomContextWhenExceedsThreshold() { + fillContext(); + try { + String contextName = "abc"; + ContextUtil.enter(contextName, "bcd"); + Context curContext = ContextUtil.getContext(); + assertNotEquals(contextName, curContext.getName()); + assertTrue(curContext instanceof NullContext); + assertEquals("", curContext.getOrigin()); + } finally { + ContextUtil.exit(); + resetContextMap(); + } + } + + @Test + public void testDefaultContextWhenExceedsThreshold() { + fillContext(); + try { + ContextUtil.trueEnter(Constants.CONTEXT_DEFAULT_NAME, ""); + Context curContext = ContextUtil.getContext(); + assertEquals(Constants.CONTEXT_DEFAULT_NAME, curContext.getName()); + assertNotNull(curContext.getEntranceNode()); + } finally { + ContextUtil.exit(); + resetContextMap(); + } + } + + private void fillContext() { + for (int i = 0; i < Constants.MAX_CONTEXT_NAME_SIZE; i++) { + ContextUtil.enter("test-context-" + i); + ContextUtil.exit(); + } + } + + private void resetContextMap() { + ContextUtil.resetContextMap(); + Constants.ROOT.removeChildList(); } @Test