Repository: apache/shardingsphere
Issue: read-write splitting  many thread

Read and write separation, no proper routing when multithreading

version:3.0.0

Our requirement is to force the main library to be routed at the beginning of the "add", "create", "save", "insert", "update", "del" methods. In the process of using high-concurrency, the route is not correct。

Found in the middleware, found that the judgment route is a static variable， `private static boolean isMasterRouteOnly` 。

This static variable is shared by all threads. The a thread is not executed. The b thread modifies this value. Then, when routing, it uses this variable to judge the route, resulting in no correct route.

sharding code
``` java
@NoArgsConstructor(access = AccessLevel.PRIVATE)
public final class HintManagerHolder {
    
    public static final String DB_TABLE_NAME = "DB_TABLE_NAME";
    
    public static final String DB_COLUMN_NAME = "DB_COLUMN_NAME";
    
    private static final ThreadLocal<HintManager> HINT_MANAGER_HOLDER = new ThreadLocal<>();
    
    private static final Multimap<String, Comparable<?>> DATABASE_SHARDING_VALUES = HashMultimap.create();
    
    private static final Multimap<String, Comparable<?>> TABLE_SHARDING_VALUES = HashMultimap.create();
    
    @Setter
    private static boolean databaseShardingOnly;
    
    @Setter
    private static boolean isMasterRouteOnly;
```

our code
``` java
private static final ThreadLocal<Boolean> CUS_HINT_MANAGER_HOLDER = new ThreadLocal<>();

    @Order(Ordered.HIGHEST_PRECEDENCE)
    @Around("execution(public * com.xhg..dao..*.*(..)) || execution(public * com.xhg..service.impl..*.*(..))")
    public Object hintMasterCalls(ProceedingJoinPoint jp) throws Throwable {
        String methodName = jp.getSignature().getName();

        if (HintManagerHolder.isMasterRouteOnly() || HintManagerHolder.get() != null) {
            if (CUS_HINT_MANAGER_HOLDER.get() == null || !CUS_HINT_MANAGER_HOLDER.get()) {
                CUS_HINT_MANAGER_HOLDER.set(true);
            }
            return jp.proceed();
        }

        if (CUS_HINT_MANAGER_HOLDER.get() == null || !CUS_HINT_MANAGER_HOLDER.get()) {
            String[] methodPreArr = {"add", "create", "save", "insert", "update", "del"};
            for (String methodPre : methodPreArr) {
                if (methodName.startsWith(methodPre)) {
                    CUS_HINT_MANAGER_HOLDER.set(true);
                    break;
                }
            }
        }

        if (CUS_HINT_MANAGER_HOLDER.get() != null && CUS_HINT_MANAGER_HOLDER.get()) {
            HintManager hintManager = HintManager.getInstance();
            hintManager.setMasterRouteOnly();

        }
        return jp.proceed();
    }
```



Changed files:
sharding-core/src/test/java/io/shardingsphere/core/hint/HintManagerHolderTest.java
sharding-core/src/main/java/io/shardingsphere/core/hint/HintManagerHolder.java
sharding-core/src/main/java/io/shardingsphere/api/HintManager.java

diff --git a/sharding-core/src/main/java/io/shardingsphere/api/HintManager.java b/sharding-core/src/main/java/io/shardingsphere/api/HintManager.java
index d060ef2db29..01c725f4c8d 100644
--- a/sharding-core/src/main/java/io/shardingsphere/api/HintManager.java
+++ b/sharding-core/src/main/java/io/shardingsphere/api/HintManager.java
@@ -17,8 +17,11 @@
 
 package io.shardingsphere.api;
 
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.Multimap;
 import io.shardingsphere.core.hint.HintManagerHolder;
 import lombok.AccessLevel;
+import lombok.Getter;
 import lombok.NoArgsConstructor;
 
 /**
@@ -31,6 +34,18 @@ import lombok.NoArgsConstructor;
 @NoArgsConstructor(access = AccessLevel.PRIVATE)
 public final class HintManager implements AutoCloseable {
     
+    @Getter
+    private final Multimap<String, Comparable<?>> databaseShardingValues = HashMultimap.create();
+
+    @Getter
+    private final Multimap<String, Comparable<?>> tableShardingValues = HashMultimap.create();
+    
+    @Getter
+    private boolean databaseShardingOnly;
+
+    @Getter
+    private boolean masterRouteOnly;
+
     /**
      * Get a new instance for {@code HintManager}.
      *
@@ -51,14 +66,16 @@ public final class HintManager implements AutoCloseable {
      * @param value sharding value
      */
     public void setDatabaseShardingValue(final Comparable<?> value) {
-        HintManagerHolder.setDatabaseShardingValue(value);
+        databaseShardingValues.clear();
+        addDatabaseShardingValue(HintManagerHolder.DB_TABLE_NAME, value);
+        databaseShardingOnly = true;
     }
     
     /**
      * Set CRUD operation force route to master database only.
      */
     public void setMasterRouteOnly() {
-        HintManagerHolder.setMasterRouteOnly(true);
+        masterRouteOnly = true;
     }
     
     /**
@@ -70,7 +87,8 @@ public final class HintManager implements AutoCloseable {
      * @param value sharding value
      */
     public void addDatabaseShardingValue(final String logicTable, final Comparable<?> value) {
-        HintManagerHolder.addDatabaseShardingValue(logicTable, value);
+        databaseShardingValues.put(logicTable, value);
+        databaseShardingOnly = false;
     }
     
     /**
@@ -82,7 +100,8 @@ public final class HintManager implements AutoCloseable {
      * @param value sharding value
      */
     public void addTableShardingValue(final String logicTable, final Comparable<?> value) {
-        HintManagerHolder.addTableShardingValue(logicTable, value);
+        tableShardingValues.put(logicTable, value);
+        databaseShardingOnly = false;
     }
     
     @Override
diff --git a/sharding-core/src/main/java/io/shardingsphere/core/hint/HintManagerHolder.java b/sharding-core/src/main/java/io/shardingsphere/core/hint/HintManagerHolder.java
index 01af1ca25de..199a572d7c2 100644
--- a/sharding-core/src/main/java/io/shardingsphere/core/hint/HintManagerHolder.java
+++ b/sharding-core/src/main/java/io/shardingsphere/core/hint/HintManagerHolder.java
@@ -19,14 +19,11 @@ package io.shardingsphere.core.hint;
 
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.HashMultimap;
-import com.google.common.collect.Multimap;
 import io.shardingsphere.api.HintManager;
 import io.shardingsphere.api.algorithm.sharding.ListShardingValue;
 import io.shardingsphere.api.algorithm.sharding.ShardingValue;
 import lombok.AccessLevel;
 import lombok.NoArgsConstructor;
-import lombok.Setter;
 
 import java.util.Collection;
 
@@ -47,16 +44,6 @@ public final class HintManagerHolder {
     
     private static final ThreadLocal<HintManager> HINT_MANAGER_HOLDER = new ThreadLocal<>();
     
-    private static final Multimap<String, Comparable<?>> DATABASE_SHARDING_VALUES = HashMultimap.create();
-    
-    private static final Multimap<String, Comparable<?>> TABLE_SHARDING_VALUES = HashMultimap.create();
-    
-    @Setter
-    private static boolean databaseShardingOnly;
-    
-    @Setter
-    private static boolean isMasterRouteOnly;
-    
     /**
      * Set hint manager.
      *
@@ -67,53 +54,13 @@ public final class HintManagerHolder {
         HINT_MANAGER_HOLDER.set(hintManager);
     }
     
-    /**
-     * Add sharding value for one certain sharding database.
-     *
-     * <p>The sharding operator is {@code =}</p>
-     * When you need to assign the values to one certain sharding database, use this method to add sharding value for this database.
-     *
-     * @param value sharding value
-     */
-    public static void setDatabaseShardingValue(final Comparable<?> value) {
-        DATABASE_SHARDING_VALUES.clear();
-        addDatabaseShardingValue(DB_TABLE_NAME, value);
-        databaseShardingOnly = true;
-    }
-    
-    /**
-     * Add sharding value for database.
-     *
-     * <p>The sharding operator is {@code =}</p>
-     *
-     * @param logicTable logic table name
-     * @param value sharding value
-     */
-    public static void addDatabaseShardingValue(final String logicTable, final Comparable<?> value) {
-        DATABASE_SHARDING_VALUES.put(logicTable, value);
-        databaseShardingOnly = false;
-    }
-    
-    /**
-     * Add sharding value for table.
-     *
-     * <p>The sharding operator is {@code =}</p>
-     *
-     * @param logicTable logic table name
-     * @param value sharding value
-     */
-    public static void addTableShardingValue(final String logicTable, final Comparable<?> value) {
-        TABLE_SHARDING_VALUES.put(logicTable, value);
-        databaseShardingOnly = false;
-    }
-    
     /**
      * Judge whether only database is sharding.
      *
      * @return database sharding or not
      */
     public static boolean isDatabaseShardingOnly() {
-        return null != HINT_MANAGER_HOLDER.get() && databaseShardingOnly;
+        return null != HINT_MANAGER_HOLDER.get() && HINT_MANAGER_HOLDER.get().isDatabaseShardingOnly();
     }
     
     /**
@@ -122,7 +69,7 @@ public final class HintManagerHolder {
      * @return is force route to master database only or not
      */
     public static boolean isMasterRouteOnly() {
-        return null != HINT_MANAGER_HOLDER.get() && isMasterRouteOnly;
+        return null != HINT_MANAGER_HOLDER.get() && HINT_MANAGER_HOLDER.get().isMasterRouteOnly();
     }
     
     /**
@@ -132,10 +79,10 @@ public final class HintManagerHolder {
      * @return database sharding value
      */
     public static Optional<ShardingValue> getDatabaseShardingValue(final String logicTable) {
-        if (null == HINT_MANAGER_HOLDER.get() || !DATABASE_SHARDING_VALUES.containsKey(logicTable)) {
+        if (null == HINT_MANAGER_HOLDER.get() || !HINT_MANAGER_HOLDER.get().getDatabaseShardingValues().containsKey(logicTable)) {
             return Optional.absent();
         }
-        return Optional.of(getShardingValue(logicTable, DATABASE_SHARDING_VALUES.get(logicTable)));
+        return Optional.of(getShardingValue(logicTable, HINT_MANAGER_HOLDER.get().getDatabaseShardingValues().get(logicTable)));
     }
     
     /**
@@ -145,10 +92,10 @@ public final class HintManagerHolder {
      * @return table sharding value
      */
     public static Optional<ShardingValue> getTableShardingValue(final String logicTable) {
-        if (null == HINT_MANAGER_HOLDER.get() || !TABLE_SHARDING_VALUES.containsKey(logicTable)) {
+        if (null == HINT_MANAGER_HOLDER.get() || !HINT_MANAGER_HOLDER.get().getTableShardingValues().containsKey(logicTable)) {
             return Optional.absent();
         }
-        return Optional.of(getShardingValue(logicTable, TABLE_SHARDING_VALUES.get(logicTable)));
+        return Optional.of(getShardingValue(logicTable, HINT_MANAGER_HOLDER.get().getTableShardingValues().get(logicTable)));
     }
     
     @SuppressWarnings("unchecked")
@@ -171,9 +118,5 @@ public final class HintManagerHolder {
      */
     public static void clear() {
         HINT_MANAGER_HOLDER.remove();
-        DATABASE_SHARDING_VALUES.clear();
-        TABLE_SHARDING_VALUES.clear();
-        databaseShardingOnly = false;
-        isMasterRouteOnly = false;
     }
 }
diff --git a/sharding-core/src/test/java/io/shardingsphere/core/hint/HintManagerHolderTest.java b/sharding-core/src/test/java/io/shardingsphere/core/hint/HintManagerHolderTest.java
index e6927db6c9b..7b22d770d25 100644
--- a/sharding-core/src/test/java/io/shardingsphere/core/hint/HintManagerHolderTest.java
+++ b/sharding-core/src/test/java/io/shardingsphere/core/hint/HintManagerHolderTest.java
@@ -66,6 +66,12 @@ public final class HintManagerHolderTest {
         hintManager.close();
         assertFalse(HintManagerHolder.isMasterRouteOnly());
     }
+
+    @Test
+    public void assertIsDatabaseShardingOnly() {
+        hintManager.setDatabaseShardingValue("1");
+        assertTrue(HintManagerHolder.isDatabaseShardingOnly());
+    }
     
     @Test
     public void assertIsDatabaseShardingOnlyWithoutSet() {
@@ -77,6 +83,8 @@ public final class HintManagerHolderTest {
     public void assertIsMasterRouteOnly() {
         hintManager.setMasterRouteOnly();
         assertTrue(HintManagerHolder.isMasterRouteOnly());
+        hintManager.close();
+        assertFalse(HintManagerHolder.isMasterRouteOnly());
     }
     
     @Test