From 0e9c54ec141bfc7d038f0ca563e9298292d5cbbd Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Thu, 20 Jan 2022 17:15:15 +0800 Subject: [PATCH 1/4] fix backoffer data race Signed-off-by: iosmanthus --- src/main/java/org/tikv/common/util/ConcreteBackOffer.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/tikv/common/util/ConcreteBackOffer.java b/src/main/java/org/tikv/common/util/ConcreteBackOffer.java index b550b9fdd98..e7e865bd695 100644 --- a/src/main/java/org/tikv/common/util/ConcreteBackOffer.java +++ b/src/main/java/org/tikv/common/util/ConcreteBackOffer.java @@ -22,9 +22,9 @@ import com.google.common.base.Preconditions; import io.prometheus.client.Histogram; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.tikv.common.TiConfiguration; @@ -57,7 +57,7 @@ private ConcreteBackOffer(int maxSleep, long deadline, SlowLog slowLog) { Preconditions.checkArgument(deadline >= 0, "Deadline cannot be less than 0."); this.maxSleep = maxSleep; this.errors = new ArrayList<>(); - this.backOffFunctionMap = new HashMap<>(); + this.backOffFunctionMap = new ConcurrentHashMap<>(); this.deadline = deadline; this.slowLog = slowLog; } From 2ee3e069b1204d24253eec5b25c034d3c47ef8ef Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Thu, 20 Jan 2022 21:37:13 +0800 Subject: [PATCH 2/4] add unit tests for data race Signed-off-by: iosmanthus --- .../tikv/common/util/ConcreteBackOffer.java | 12 +++- .../org/tikv/util/ConcreteBackOfferTest.java | 67 +++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) create mode 100644 src/test/java/org/tikv/util/ConcreteBackOfferTest.java diff --git a/src/main/java/org/tikv/common/util/ConcreteBackOffer.java b/src/main/java/org/tikv/common/util/ConcreteBackOffer.java index e7e865bd695..ef99613b367 100644 --- a/src/main/java/org/tikv/common/util/ConcreteBackOffer.java +++ b/src/main/java/org/tikv/common/util/ConcreteBackOffer.java @@ -19,9 +19,11 @@ import static org.tikv.common.ConfigUtils.TIKV_BO_REGION_MISS_BASE_IN_MS; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import io.prometheus.client.Histogram; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -37,8 +39,12 @@ public class ConcreteBackOffer implements BackOffer { private static final Logger logger = LoggerFactory.getLogger(ConcreteBackOffer.class); private final int maxSleep; - private final Map backOffFunctionMap; - private final List errors; + + @VisibleForTesting + public final Map backOffFunctionMap; + + @VisibleForTesting + public final List errors; private int totalSleep; private final long deadline; private final SlowLog slowLog; @@ -56,7 +62,7 @@ private ConcreteBackOffer(int maxSleep, long deadline, SlowLog slowLog) { Preconditions.checkArgument(maxSleep >= 0, "Max sleep time cannot be less than 0."); Preconditions.checkArgument(deadline >= 0, "Deadline cannot be less than 0."); this.maxSleep = maxSleep; - this.errors = new ArrayList<>(); + this.errors = Collections.synchronizedList(new ArrayList<>()); this.backOffFunctionMap = new ConcurrentHashMap<>(); this.deadline = deadline; this.slowLog = slowLog; diff --git a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java new file mode 100644 index 00000000000..b18c3870e78 --- /dev/null +++ b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java @@ -0,0 +1,67 @@ +package org.tikv.util; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map.Entry; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import org.junit.Assert; +import org.junit.Test; +import org.tikv.common.exception.GrpcException; +import org.tikv.common.util.BackOffFunction; +import org.tikv.common.util.BackOffFunction.BackOffFuncType; +import org.tikv.common.util.ConcreteBackOffer; + +public class ConcreteBackOfferTest { + + private static void ignoreException(Callable callable) + throws Exception { + try { + callable.call(); + } catch (Exception ignored) { + } + } + + @Test + public void raceMapTest() throws Exception { + ConcreteBackOffer backOffer = ConcreteBackOffer.newRawKVBackOff(); + ignoreException(() -> { + backOffer.doBackOff(BackOffFuncType.BoRegionMiss, new Exception("first backoff")); + return null; + }); + ignoreException(() -> { + backOffer.doBackOff(BackOffFuncType.BoTiKVRPC, new Exception("second backoff")); + return null; + }); + for (Entry item : backOffer.backOffFunctionMap.entrySet()) { + backOffer.backOffFunctionMap.remove(item.getKey()); + } + } + + @Test + public void raceErrorsTest() throws Exception { + int timeToSleep = 1; + int threadCnt = Runtime.getRuntime().availableProcessors() * 2; + int taskCnt = threadCnt * 2; + + ExecutorService executorService = Executors.newFixedThreadPool(threadCnt); + ConcreteBackOffer backOffer = ConcreteBackOffer.newCustomBackOff(timeToSleep); + List> tasks = new ArrayList<>(); + for (int i = 0; i < taskCnt; i++) { + int idx = i; + Future task = executorService.submit(() -> { + try { + backOffer.doBackOff(BackOffFuncType.BoUpdateLeader, new Exception("backoff " + idx)); + } catch (GrpcException ignored) { + } + }); + tasks.add(task); + } + for (Future task : tasks) { + task.get(); + } + Assert.assertEquals(backOffer.errors.size(), taskCnt); + } +} From 45b83b15d539df2baf2bf771a7e20ce6146c6bf4 Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Thu, 20 Jan 2022 21:38:19 +0800 Subject: [PATCH 3/4] ./dev/javafmt Signed-off-by: iosmanthus --- .../tikv/common/util/ConcreteBackOffer.java | 3 +- .../org/tikv/util/ConcreteBackOfferTest.java | 36 ++++++++++--------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/tikv/common/util/ConcreteBackOffer.java b/src/main/java/org/tikv/common/util/ConcreteBackOffer.java index ef99613b367..e3a103a9f2c 100644 --- a/src/main/java/org/tikv/common/util/ConcreteBackOffer.java +++ b/src/main/java/org/tikv/common/util/ConcreteBackOffer.java @@ -43,8 +43,7 @@ public class ConcreteBackOffer implements BackOffer { @VisibleForTesting public final Map backOffFunctionMap; - @VisibleForTesting - public final List errors; + @VisibleForTesting public final List errors; private int totalSleep; private final long deadline; private final SlowLog slowLog; diff --git a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java index b18c3870e78..a971991b0b3 100644 --- a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java +++ b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java @@ -16,8 +16,7 @@ public class ConcreteBackOfferTest { - private static void ignoreException(Callable callable) - throws Exception { + private static void ignoreException(Callable callable) throws Exception { try { callable.call(); } catch (Exception ignored) { @@ -27,14 +26,16 @@ private static void ignoreException(Callable callable) @Test public void raceMapTest() throws Exception { ConcreteBackOffer backOffer = ConcreteBackOffer.newRawKVBackOff(); - ignoreException(() -> { - backOffer.doBackOff(BackOffFuncType.BoRegionMiss, new Exception("first backoff")); - return null; - }); - ignoreException(() -> { - backOffer.doBackOff(BackOffFuncType.BoTiKVRPC, new Exception("second backoff")); - return null; - }); + ignoreException( + () -> { + backOffer.doBackOff(BackOffFuncType.BoRegionMiss, new Exception("first backoff")); + return null; + }); + ignoreException( + () -> { + backOffer.doBackOff(BackOffFuncType.BoTiKVRPC, new Exception("second backoff")); + return null; + }); for (Entry item : backOffer.backOffFunctionMap.entrySet()) { backOffer.backOffFunctionMap.remove(item.getKey()); } @@ -51,12 +52,15 @@ public void raceErrorsTest() throws Exception { List> tasks = new ArrayList<>(); for (int i = 0; i < taskCnt; i++) { int idx = i; - Future task = executorService.submit(() -> { - try { - backOffer.doBackOff(BackOffFuncType.BoUpdateLeader, new Exception("backoff " + idx)); - } catch (GrpcException ignored) { - } - }); + Future task = + executorService.submit( + () -> { + try { + backOffer.doBackOff( + BackOffFuncType.BoUpdateLeader, new Exception("backoff " + idx)); + } catch (GrpcException ignored) { + } + }); tasks.add(task); } for (Future task : tasks) { From 81e5c0f36c57c320cc2d889280f9e14db6f10205 Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Thu, 20 Jan 2022 21:42:19 +0800 Subject: [PATCH 4/4] add license header Signed-off-by: iosmanthus --- .../java/org/tikv/util/ConcreteBackOfferTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java index a971991b0b3..0b8adcf20e9 100644 --- a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java +++ b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java @@ -1,3 +1,18 @@ +/* + * Copyright 2017 PingCAP, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.tikv.util; import java.util.ArrayList;