From 6855e15cb37b6eadeae1691e864144b0a7191185 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Wed, 18 Dec 2019 21:14:31 -0500 Subject: [PATCH] Updates addWeightConfig to remove synchronized. Instead of modifying GroupWeightConfigs create a new one and replace it. Fixes gh-1459 --- .../filter/WeightCalculatorWebFilter.java | 121 ++++++++++-------- ...ghtCalculatorWebFilterConcurrentTests.java | 4 +- 2 files changed, 69 insertions(+), 56 deletions(-) diff --git a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/WeightCalculatorWebFilter.java b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/WeightCalculatorWebFilter.java index 2d371371..51942486 100644 --- a/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/WeightCalculatorWebFilter.java +++ b/spring-cloud-gateway-core/src/main/java/org/springframework/cloud/gateway/filter/WeightCalculatorWebFilter.java @@ -131,8 +131,8 @@ public class WeightCalculatorWebFilter addWeightConfig(((WeightDefinedEvent) event).getWeightConfig()); } else if (event instanceof RefreshRoutesEvent && routeLocator != null) { - routeLocator.ifAvailable(locator -> locator.getRoutes().subscribe()); // forces - // initialization + // forces initialization + routeLocator.ifAvailable(locator -> locator.getRoutes().subscribe()); } } @@ -159,47 +159,57 @@ public class WeightCalculatorWebFilter /* for testing */ void addWeightConfig(WeightConfig weightConfig) { String group = weightConfig.getGroup(); - GroupWeightConfig c = groupWeights.get(group); - if (c == null) { - c = new GroupWeightConfig(group); - groupWeights.put(group, c); + GroupWeightConfig config; + // only create new GroupWeightConfig rather than modify + // and put at end of calculations. This avoids concurency problems + // later during filter execution. + if (groupWeights.containsKey(group)) { + config = new GroupWeightConfig(groupWeights.get(group)); } - GroupWeightConfig config = c; - synchronized (config) { - config.weights.put(weightConfig.getRouteId(), weightConfig.getWeight()); - - // recalculate - - // normalize weights - int weightsSum = config.weights.values().stream().mapToInt(Integer::intValue) - .sum(); - - final AtomicInteger index = new AtomicInteger(0); - config.weights.forEach((routeId, weight) -> { - Double nomalizedWeight = weight / (double) weightsSum; - config.normalizedWeights.put(routeId, nomalizedWeight); - - // recalculate rangeIndexes - config.rangeIndexes.put(index.getAndIncrement(), routeId); - }); - - // TODO: calculate ranges - config.ranges.clear(); - - config.ranges.add(0.0); - - List values = new ArrayList<>(config.normalizedWeights.values()); - for (int i = 0; i < values.size(); i++) { - Double currentWeight = values.get(i); - Double previousRange = config.ranges.get(i); - Double range = previousRange + currentWeight; - config.ranges.add(range); - } - - if (log.isTraceEnabled()) { - log.trace("Recalculated group weight config " + config); - } + else { + config = new GroupWeightConfig(group); } + + config.weights.put(weightConfig.getRouteId(), weightConfig.getWeight()); + + // recalculate + + // normalize weights + int weightsSum = 0; + + for (Integer weight : config.weights.values()) { + weightsSum += weight; + } + + final AtomicInteger index = new AtomicInteger(0); + for (Map.Entry entry : config.weights.entrySet()) { + String routeId = entry.getKey(); + Integer weight = entry.getValue(); + Double nomalizedWeight = weight / (double) weightsSum; + config.normalizedWeights.put(routeId, nomalizedWeight); + + // recalculate rangeIndexes + config.rangeIndexes.put(index.getAndIncrement(), routeId); + } + + // TODO: calculate ranges + config.ranges.clear(); + + config.ranges.add(0.0); + + List values = new ArrayList<>(config.normalizedWeights.values()); + for (int i = 0; i < values.size(); i++) { + Double currentWeight = values.get(i); + Double previousRange = config.ranges.get(i); + Double range = previousRange + currentWeight; + config.ranges.add(range); + } + + if (log.isTraceEnabled()) { + log.trace("Recalculated group weight config " + config); + } + // only update after all calculations + groupWeights.put(group, config); } /* for testing */ Map getGroupWeights() { @@ -222,20 +232,18 @@ public class WeightCalculatorWebFilter double r = this.random.nextDouble(); - synchronized (config) { - List ranges = config.ranges; + List ranges = config.ranges; - if (log.isTraceEnabled()) { - log.trace("Weight for group: " + group + ", ranges: " + ranges - + ", r: " + r); - } + if (log.isTraceEnabled()) { + log.trace("Weight for group: " + group + ", ranges: " + ranges + ", r: " + + r); + } - for (int i = 0; i < ranges.size() - 1; i++) { - if (r >= ranges.get(i) && r < ranges.get(i + 1)) { - String routeId = config.rangeIndexes.get(i); - weights.put(group, routeId); - break; - } + for (int i = 0; i < ranges.size() - 1; i++) { + if (r >= ranges.get(i) && r < ranges.get(i + 1)) { + String routeId = config.rangeIndexes.get(i); + weights.put(group, routeId); + break; } } } @@ -263,6 +271,13 @@ public class WeightCalculatorWebFilter this.group = group; } + GroupWeightConfig(GroupWeightConfig other) { + this.group = other.group; + this.weights = new LinkedHashMap<>(other.weights); + this.normalizedWeights = new LinkedHashMap<>(other.normalizedWeights); + this.rangeIndexes = new LinkedHashMap<>(other.rangeIndexes); + } + @Override public String toString() { return new ToStringCreator(this).append("group", group) diff --git a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/WeightCalculatorWebFilterConcurrentTests.java b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/WeightCalculatorWebFilterConcurrentTests.java index c73a5f45..18ad99c4 100644 --- a/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/WeightCalculatorWebFilterConcurrentTests.java +++ b/spring-cloud-gateway-core/src/test/java/org/springframework/cloud/gateway/filter/WeightCalculatorWebFilterConcurrentTests.java @@ -23,7 +23,6 @@ import java.util.concurrent.TimeUnit; import io.netty.util.internal.ThreadLocalRandom; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mockito; @@ -58,7 +57,7 @@ import static org.springframework.boot.test.context.SpringBootTest.WebEnvironmen @DirtiesContext public class WeightCalculatorWebFilterConcurrentTests { - @Value("${test.concurrent.execution.timeInSeconds}") + @Value("${test.concurrent.execution.timeInSeconds:5}") private int maxTestTimeSeconds; @Autowired @@ -77,7 +76,6 @@ public class WeightCalculatorWebFilterConcurrentTests { startTime = System.currentTimeMillis(); } - @Ignore @Test public void WeightCalculatorWebFilter_threadSafeTest() { generateEvents();