From 84c56994fbaa5a8dbc7f9f4dea99bd379182868f Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Thu, 20 Jan 2022 17:15:15 +0800 Subject: [PATCH 1/6] 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 bbcababa3e7..c940376a4c5 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 81f503fdb698032eee7286b8868c6547557eca70 Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Thu, 20 Jan 2022 21:37:13 +0800 Subject: [PATCH 2/6] 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 c940376a4c5..08b6a0fa876 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 bfa16ea67133f977d4e0269b07f0aef49ec1ab0c Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Thu, 20 Jan 2022 21:38:19 +0800 Subject: [PATCH 3/6] ./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 08b6a0fa876..ba1dd4866b1 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 61de522ba0816f608d944a787bbd56aca0fff29d Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Thu, 20 Jan 2022 21:42:19 +0800 Subject: [PATCH 4/6] 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; From 77740c9ba32dc7cef435f3a4d8ffafb1d8984c0f Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Fri, 21 Jan 2022 14:21:53 +0800 Subject: [PATCH 5/6] fix license error Signed-off-by: iosmanthus --- src/test/java/org/tikv/util/ConcreteBackOfferTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java index 0b8adcf20e9..f94f1af17d7 100644 --- a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java +++ b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java @@ -5,7 +5,7 @@ * 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 + * 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, From 1f9825ea25e7f7815fc136a4d71fe962cfb2a50f Mon Sep 17 00:00:00 2001 From: iosmanthus Date: Fri, 21 Jan 2022 14:23:55 +0800 Subject: [PATCH 6/6] fix license error Signed-off-by: iosmanthus --- src/test/java/org/tikv/util/ConcreteBackOfferTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java index f94f1af17d7..f3fe77d63fa 100644 --- a/src/test/java/org/tikv/util/ConcreteBackOfferTest.java +++ b/src/test/java/org/tikv/util/ConcreteBackOfferTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2017 PingCAP, Inc. + * Copyright 2021 TiKV Project Authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -9,8 +9,10 @@ * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. + * */ package org.tikv.util;