From ced1dfe937a3f6f890c5edadf197864a4f68371b Mon Sep 17 00:00:00 2001 From: Narendra Pathai Date: Mon, 15 Oct 2018 17:28:11 +0530 Subject: [PATCH] Resolves #643, test cases failed due to presence of global state in CallsCount. Because AppTest was executed before B2BServiceTest, it scheduled 1 sec timer using ThrottleTimerImpl class. While resetting it used that global CallCount class reset() method, which reset all counters. So that causes thread safety issue because of unintended sharing of application state between test cases, which is not a good practice. --- .../main/java/com/iluwatar/throttling/App.java | 16 ++++++++-------- .../java/com/iluwatar/throttling/B2BService.java | 8 +++++--- .../java/com/iluwatar/throttling/CallsCount.java | 10 +++++----- .../java/com/iluwatar/throttling/Tenant.java | 4 ++-- .../throttling/timer/ThrottleTimerImpl.java | 10 ++++++---- .../com/iluwatar/throttling/B2BServiceTest.java | 9 +++++---- .../java/com/iluwatar/throttling/TenantTest.java | 2 +- 7 files changed, 32 insertions(+), 27 deletions(-) diff --git a/throttling/src/main/java/com/iluwatar/throttling/App.java b/throttling/src/main/java/com/iluwatar/throttling/App.java index d6c619359..95e0dedf7 100644 --- a/throttling/src/main/java/com/iluwatar/throttling/App.java +++ b/throttling/src/main/java/com/iluwatar/throttling/App.java @@ -53,14 +53,14 @@ public class App { * @param args main arguments */ public static void main(String[] args) { - - Tenant adidas = new Tenant("Adidas", 5); - Tenant nike = new Tenant("Nike", 6); + CallsCount callsCount = new CallsCount(); + Tenant adidas = new Tenant("Adidas", 5, callsCount); + Tenant nike = new Tenant("Nike", 6, callsCount); ExecutorService executorService = Executors.newFixedThreadPool(2); - executorService.execute(() -> makeServiceCalls(adidas)); - executorService.execute(() -> makeServiceCalls(nike)); + executorService.execute(() -> makeServiceCalls(adidas, callsCount)); + executorService.execute(() -> makeServiceCalls(nike, callsCount)); executorService.shutdown(); try { @@ -73,9 +73,9 @@ public class App { /** * Make calls to the B2BService dummy API */ - private static void makeServiceCalls(Tenant tenant) { - Throttler timer = new ThrottleTimerImpl(10); - B2BService service = new B2BService(timer); + private static void makeServiceCalls(Tenant tenant, CallsCount callsCount) { + Throttler timer = new ThrottleTimerImpl(10, callsCount); + B2BService service = new B2BService(timer, callsCount); for (int i = 0; i < 20; i++) { service.dummyCustomerApi(tenant); // Sleep is introduced to keep the output in check and easy to view and analyze the results. diff --git a/throttling/src/main/java/com/iluwatar/throttling/B2BService.java b/throttling/src/main/java/com/iluwatar/throttling/B2BService.java index 51ed492eb..4ef30c48d 100644 --- a/throttling/src/main/java/com/iluwatar/throttling/B2BService.java +++ b/throttling/src/main/java/com/iluwatar/throttling/B2BService.java @@ -35,8 +35,10 @@ import java.util.concurrent.ThreadLocalRandom; class B2BService { private static final Logger LOGGER = LoggerFactory.getLogger(B2BService.class); + private final CallsCount callsCount; - public B2BService(Throttler timer) { + public B2BService(Throttler timer, CallsCount callsCount) { + this.callsCount = callsCount; timer.start(); } @@ -46,13 +48,13 @@ class B2BService { */ public int dummyCustomerApi(Tenant tenant) { String tenantName = tenant.getName(); - long count = CallsCount.getCount(tenantName); + long count = callsCount.getCount(tenantName); LOGGER.debug("Counter for {} : {} ", tenant.getName(), count); if (count >= tenant.getAllowedCallsPerSecond()) { LOGGER.error("API access per second limit reached for: {}", tenantName); return -1; } - CallsCount.incrementCount(tenantName); + callsCount.incrementCount(tenantName); return getRandomCustomerId(); } diff --git a/throttling/src/main/java/com/iluwatar/throttling/CallsCount.java b/throttling/src/main/java/com/iluwatar/throttling/CallsCount.java index 9b274849a..2c0afdb35 100644 --- a/throttling/src/main/java/com/iluwatar/throttling/CallsCount.java +++ b/throttling/src/main/java/com/iluwatar/throttling/CallsCount.java @@ -38,13 +38,13 @@ import java.util.concurrent.atomic.AtomicLong; public final class CallsCount { private static final Logger LOGGER = LoggerFactory.getLogger(CallsCount.class); - private static Map tenantCallsCount = new ConcurrentHashMap<>(); + private Map tenantCallsCount = new ConcurrentHashMap<>(); /** * Add a new tenant to the map. * @param tenantName name of the tenant. */ - public static void addTenant(String tenantName) { + public void addTenant(String tenantName) { tenantCallsCount.putIfAbsent(tenantName, new AtomicLong(0)); } @@ -52,7 +52,7 @@ public final class CallsCount { * Increment the count of the specified tenant. * @param tenantName name of the tenant. */ - public static void incrementCount(String tenantName) { + public void incrementCount(String tenantName) { tenantCallsCount.get(tenantName).incrementAndGet(); } @@ -61,14 +61,14 @@ public final class CallsCount { * @param tenantName name of the tenant. * @return the count of the tenant. */ - public static long getCount(String tenantName) { + public long getCount(String tenantName) { return tenantCallsCount.get(tenantName).get(); } /** * Resets the count of all the tenants in the map. */ - public static void reset() { + public void reset() { LOGGER.debug("Resetting the map."); for (Entry e : tenantCallsCount.entrySet()) { tenantCallsCount.put(e.getKey(), new AtomicLong(0)); diff --git a/throttling/src/main/java/com/iluwatar/throttling/Tenant.java b/throttling/src/main/java/com/iluwatar/throttling/Tenant.java index a720e154b..e35251aa0 100644 --- a/throttling/src/main/java/com/iluwatar/throttling/Tenant.java +++ b/throttling/src/main/java/com/iluwatar/throttling/Tenant.java @@ -38,13 +38,13 @@ public class Tenant { * @param allowedCallsPerSecond The number of calls allowed for a particular tenant. * @throws InvalidParameterException If number of calls is less than 0, throws exception. */ - public Tenant(String name, int allowedCallsPerSecond) { + public Tenant(String name, int allowedCallsPerSecond, CallsCount callsCount) { if (allowedCallsPerSecond < 0) { throw new InvalidParameterException("Number of calls less than 0 not allowed"); } this.name = name; this.allowedCallsPerSecond = allowedCallsPerSecond; - CallsCount.addTenant(name); + callsCount.addTenant(name); } public String getName() { diff --git a/throttling/src/main/java/com/iluwatar/throttling/timer/ThrottleTimerImpl.java b/throttling/src/main/java/com/iluwatar/throttling/timer/ThrottleTimerImpl.java index 4ff4ce246..3d1e68287 100644 --- a/throttling/src/main/java/com/iluwatar/throttling/timer/ThrottleTimerImpl.java +++ b/throttling/src/main/java/com/iluwatar/throttling/timer/ThrottleTimerImpl.java @@ -37,10 +37,12 @@ import com.iluwatar.throttling.CallsCount; */ public class ThrottleTimerImpl implements Throttler { - private int throttlePeriod; - - public ThrottleTimerImpl(int throttlePeriod) { + private final int throttlePeriod; + private final CallsCount callsCount; + + public ThrottleTimerImpl(int throttlePeriod, CallsCount callsCount) { this.throttlePeriod = throttlePeriod; + this.callsCount = callsCount; } /** @@ -51,7 +53,7 @@ public class ThrottleTimerImpl implements Throttler { new Timer(true).schedule(new TimerTask() { @Override public void run() { - CallsCount.reset(); + callsCount.reset(); } }, 0, throttlePeriod); } diff --git a/throttling/src/test/java/com/iluwatar/throttling/B2BServiceTest.java b/throttling/src/test/java/com/iluwatar/throttling/B2BServiceTest.java index 39ca5ccd0..bb053fbcb 100644 --- a/throttling/src/test/java/com/iluwatar/throttling/B2BServiceTest.java +++ b/throttling/src/test/java/com/iluwatar/throttling/B2BServiceTest.java @@ -33,18 +33,19 @@ import static org.junit.jupiter.api.Assertions.assertEquals; */ public class B2BServiceTest { - @Disabled + private CallsCount callsCount = new CallsCount(); + @Test public void dummyCustomerApiTest() { - Tenant tenant = new Tenant("testTenant", 2); + Tenant tenant = new Tenant("testTenant", 2, callsCount); // In order to assure that throttling limits will not be reset, we use an empty throttling implementation Throttler timer = () -> { }; - B2BService service = new B2BService(timer); + B2BService service = new B2BService(timer, callsCount); for (int i = 0; i < 5; i++) { service.dummyCustomerApi(tenant); } - long counter = CallsCount.getCount(tenant.getName()); + long counter = callsCount.getCount(tenant.getName()); assertEquals(2, counter, "Counter limit must be reached"); } } diff --git a/throttling/src/test/java/com/iluwatar/throttling/TenantTest.java b/throttling/src/test/java/com/iluwatar/throttling/TenantTest.java index 00e546d02..b16b9a9f0 100644 --- a/throttling/src/test/java/com/iluwatar/throttling/TenantTest.java +++ b/throttling/src/test/java/com/iluwatar/throttling/TenantTest.java @@ -36,7 +36,7 @@ public class TenantTest { @Test public void constructorTest() { assertThrows(InvalidParameterException.class, () -> { - Tenant tenant = new Tenant("FailTenant", -1); + Tenant tenant = new Tenant("FailTenant", -1, new CallsCount()); }); } }