From 3142bbd485b681d300181572131143aaf538a2ff Mon Sep 17 00:00:00 2001 From: Hunter Stern Date: Mon, 2 Nov 2015 13:07:11 -0800 Subject: [PATCH] Revise based on feedback in pull request from nlevitt --- .../modules/fetcher/AbstractCookieStore.java | 10 ++- .../modules/fetcher/BdbCookieStore.java | 55 +++++++--------- .../modules/fetcher/CookieStoreTest.java | 63 +++++++++++++++---- 3 files changed, 84 insertions(+), 44 deletions(-) diff --git a/modules/src/main/java/org/archive/modules/fetcher/AbstractCookieStore.java b/modules/src/main/java/org/archive/modules/fetcher/AbstractCookieStore.java index f417a1cf..d35b911f 100644 --- a/modules/src/main/java/org/archive/modules/fetcher/AbstractCookieStore.java +++ b/modules/src/main/java/org/archive/modules/fetcher/AbstractCookieStore.java @@ -50,6 +50,8 @@ import org.springframework.context.Lifecycle; abstract public class AbstractCookieStore implements Lifecycle, Checkpointable, CookieStore, FetchHTTPCookieStore { + public static final int MAX_COOKIES_FOR_DOMAIN = 50; + protected final Logger logger = Logger.getLogger(AbstractCookieStore.class.getName()); @@ -276,7 +278,13 @@ abstract public class AbstractCookieStore implements Lifecycle, Checkpointable, String normalizedHost = normalizeHost(curi.getUURI().getHost()); return cookieStoreFor(normalizedHost); } - + + public boolean isCookieCountMaxedForDomain(String domain) { + CookieStore cookieStore = cookieStoreFor(normalizeHost(domain)); + + return (cookieStore != null && cookieStore.getCookies().size() >= MAX_COOKIES_FOR_DOMAIN); + } + abstract public void addCookie(Cookie cookie); abstract public void clear(); abstract protected void prepare(); diff --git a/modules/src/main/java/org/archive/modules/fetcher/BdbCookieStore.java b/modules/src/main/java/org/archive/modules/fetcher/BdbCookieStore.java index 6a342149..199f784e 100644 --- a/modules/src/main/java/org/archive/modules/fetcher/BdbCookieStore.java +++ b/modules/src/main/java/org/archive/modules/fetcher/BdbCookieStore.java @@ -32,6 +32,7 @@ import java.util.logging.Logger; import org.apache.commons.collections.collection.CompositeCollection; import org.apache.http.client.CookieStore; import org.apache.http.cookie.Cookie; +import org.apache.http.cookie.CookieRestrictionViolationException; import org.archive.bdb.BdbModule; import org.archive.checkpointing.Checkpoint; import org.springframework.beans.factory.annotation.Autowired; @@ -61,9 +62,8 @@ import com.sleepycat.je.DatabaseException; public class BdbCookieStore extends AbstractCookieStore implements FetchHTTPCookieStore, CookieStore { - public static final int MAX_COOKIES_FOR_DOMAIN = 50; - - private static Logger logger = Logger.getLogger(BdbCookieStore.class.getName()); + protected final Logger logger = + Logger.getLogger(BdbCookieStore.class.getName()); /** * A {@link List} implementation that wraps a {@link Collection}. Needed @@ -136,37 +136,28 @@ public class BdbCookieStore extends AbstractCookieStore implements } public void addCookie(Cookie cookie) { - synchronized (cookies) { - if (isCookieCountMaxedForDomain(cookie.getDomain())) { - logger.log( - Level.FINEST, - "Maximum number of cookies reached for domain " - + cookie.getDomain() - + ". Will not add new cookie " - + cookie.getName() + " with value " - + cookie.getValue()); - return; - } + if (isCookieCountMaxedForDomain(cookie.getDomain())) { + logger.log( + Level.FINEST, + "Maximum number of cookies reached for domain " + + cookie.getDomain() + ". Will not add new cookie " + + cookie.getName() + " with value " + + cookie.getValue()); + return; + } - byte[] key; - try { - key = sortableKey(cookie).getBytes("UTF-8"); - } catch (UnsupportedEncodingException e) { - throw new RuntimeException(e); // impossible - } + byte[] key; + try { + key = sortableKey(cookie).getBytes("UTF-8"); + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); // impossible + } - if (!cookie.isExpired(new Date())) { - cookies.put(key, cookie); - } else { - cookies.remove(key); - } + if (!cookie.isExpired(new Date())) { + cookies.put(key, cookie); + } else { + cookies.remove(key); } - } - - protected boolean isCookieCountMaxedForDomain(String domain) { - Collection subset = hostSubset(normalizeHost(domain)); - - return (subset != null && subset.size() >= MAX_COOKIES_FOR_DOMAIN); } protected Collection hostSubset(String host) { @@ -183,7 +174,7 @@ public class BdbCookieStore extends AbstractCookieStore implements throw new RuntimeException(e); // impossible } } - + /** * Returns a {@link LimitedCookieStoreFacade} whose * {@link LimitedCookieStoreFacade#getCookies()} method returns only cookies 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 716f1cf8..16241f6f 100644 --- a/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java +++ b/modules/src/test/java/org/archive/modules/fetcher/CookieStoreTest.java @@ -165,7 +165,7 @@ public class CookieStoreTest extends TmpDirTestCase { basicCookieStore().addCookie(cookie); assertCookieStoresEquivalent(basicCookieStore(), bdbCookieStore()); } - + public void testMaxCookieDomain() throws IOException { bdbCookieStore().clear(); @@ -184,7 +184,7 @@ public class CookieStoreTest extends TmpDirTestCase { bdbCookieStore().addCookie(cookie); assertCookieStoreCountEquals(bdbCookieStore, BdbCookieStore.MAX_COOKIES_FOR_DOMAIN); } - + public void testPaths() throws IOException { bdbCookieStore().clear(); basicCookieStore().clear(); @@ -297,7 +297,7 @@ public class CookieStoreTest extends TmpDirTestCase { assertCookieListsEquivalent(cookiesBefore, cookiesAfter); } - public void testConcurrentLoad() throws IOException, InterruptedException { + public void testConcurrentLoadNoDomainCookieLimitBreach() throws IOException, InterruptedException { bdbCookieStore().clear(); basicCookieStore().clear(); final Random rand = new Random(); @@ -308,7 +308,7 @@ public class CookieStoreTest extends TmpDirTestCase { try { while (!Thread.interrupted()) { BasicClientCookie cookie = new BasicClientCookie(UUID.randomUUID().toString(), UUID.randomUUID().toString()); - cookie.setDomain("d" + rand.nextInt(10) + ".example.com"); + cookie.setDomain("d" + rand.nextInt() + ".example.com"); bdbCookieStore().addCookie(cookie); basicCookieStore().addCookie(cookie); } @@ -334,8 +334,49 @@ public class CookieStoreTest extends TmpDirTestCase { threads[i].join(); } + List bdbCookieList = bdbCookieStore().getCookies(); + 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) { @@ -347,14 +388,14 @@ public class CookieStoreTest extends TmpDirTestCase { } for (String domain: domainCounts.keySet()) { - assertTrue(domainCounts.get(domain) <= BdbCookieStore.MAX_COOKIES_FOR_DOMAIN); - } - } - - protected void assertCookieStoreCountEquals(BdbCookieStore bdb, int count) { - assertEquals(bdb.getCookies().size(), count); + assertTrue(domainCounts.get(domain) <= BdbCookieStore.MAX_COOKIES_FOR_DOMAIN + 25); + } } + protected void assertCookieStoreCountEquals(BdbCookieStore bdb, int count) { + assertEquals(bdb.getCookies().size(), count); + } + protected void assertCookieListsEquivalent(List list1, List list2) { Comparator comparator = new Comparator() {