From a5c03d9b73b000d61b8fc4874f9a90cc884a4937 Mon Sep 17 00:00:00 2001 From: Alex Osborne Date: Mon, 12 Aug 2019 18:08:43 +0900 Subject: [PATCH 1/2] Mitigate random CookieStore.testConcurrentLoad test failures The arbitary value `25` was used but in prace it's quite possible for more than 25 writing threads to have checked the cookie count limit before adding their cookie. In practice we see Travis failing on this test quite often, every few builds in fact. I think using `threads.length` (i.e. 200) should cover the worst case possibility where every thread reads a stale count and tries to add their cookie. Fixes #274 --- .../java/org/archive/modules/fetcher/CookieStoreTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java b/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java index a621a57d..37632a04 100644 --- a/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java +++ b/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java @@ -388,7 +388,9 @@ public class CookieStoreTest extends TmpDirTestCase { } for (String domain: domainCounts.keySet()) { - assertTrue(domainCounts.get(domain) <= BdbCookieStore.MAX_COOKIES_FOR_DOMAIN + 25); + // the cookie store intentionally doesn't synchronize so we allow up to thread.length + // additional cookies over the limit + assertTrue(domainCounts.get(domain) <= BdbCookieStore.MAX_COOKIES_FOR_DOMAIN + threads.length); } } From 2a5bd87e5114ec4becde464a4b5a709fdaf1e625 Mon Sep 17 00:00:00 2001 From: Alex Osborne Date: Fri, 16 Aug 2019 22:11:26 +0900 Subject: [PATCH 2/2] Remove testConcurrentLoad Noah wrote in #280: > Maybe we should just drop the test. The assumption when we wrote the > test was that a race condition would not be so frequent in practice. > We've seen that under the contrived conditions created by the test > case, it is frequent. But that's ok --- .../modules/fetcher/CookieStoreTest.java | 55 ------------------- 1 file changed, 55 deletions(-) diff --git a/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java b/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java index 37632a04..35ff08d0 100644 --- a/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java +++ b/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java @@ -338,62 +338,7 @@ public class CookieStoreTest extends TmpDirTestCase { assertTrue(bdbCookieList.size() > 3000); assertCookieListsEquivalent(bdbCookieList, basicCookieStore().getCookies()); } - - public void testConcurrentLoad() throws IOException, InterruptedException { - bdbCookieStore().clear(); - basicCookieStore().clear(); - final Random rand = new Random(); - Runnable runnable = new Runnable() { - @Override - public void run() { - try { - while (!Thread.interrupted()) { - BasicClientCookie cookie = new BasicClientCookie(UUID.randomUUID().toString(), UUID.randomUUID().toString()); - cookie.setDomain("d" + rand.nextInt(20) + ".example.com"); - bdbCookieStore().addCookie(cookie); - basicCookieStore().addCookie(cookie); - } - } catch (Exception e) { - throw new RuntimeException(e); - } - } - }; - - Thread[] threads = new Thread[200]; - for (int i = 0; i < threads.length; i++) { - threads[i] = new Thread(runnable); - threads[i].setName("cookie-load-test-" + i); - threads[i].start(); - } - - Thread.sleep(1000); - - for (int i = 0; i < threads.length; i++) { - threads[i].interrupt(); - } - for (int i = 0; i < threads.length; i++) { - threads[i].join(); - } - - ArrayList bdbCookieArrayList = new ArrayList(bdbCookieStore().getCookies()); - Map domainCounts = new HashMap(); - for (Cookie cookie : bdbCookieArrayList) { - if (domainCounts.get(cookie.getDomain()) == null) { - domainCounts.put(cookie.getDomain(), 1); - } - else { - domainCounts.put(cookie.getDomain(), domainCounts.get(cookie.getDomain()) + 1); - } - } - - for (String domain: domainCounts.keySet()) { - // the cookie store intentionally doesn't synchronize so we allow up to thread.length - // additional cookies over the limit - assertTrue(domainCounts.get(domain) <= BdbCookieStore.MAX_COOKIES_FOR_DOMAIN + threads.length); - } - } - protected void assertCookieStoreCountEquals(BdbCookieStore bdb, int count) { assertEquals(bdb.getCookies().size(), count); }