diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index ab0e63235..dff907091 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -24,12 +24,14 @@ import java.util.AbstractMap; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -37,8 +39,19 @@ public final class ThreadSafeCookieStore implements CookieStore { + // RFC 6265 §5.5 (Implementation Limits) lets a user agent bound the cookies it retains per domain (its + // floor is "at least 50 per domain"). Capping this keeps a server from growing the jar — and the + // per-request retrieval scan in get(Uri) — without bound. Chosen generously (well above browser + // per-domain limits of ~50–180) so it only trips under abuse, never for realistic usage. See + // evictExcessCookies for the eviction order. Package-private for tests. + static final int MAX_COOKIES_PER_DOMAIN = 200; + private final Map> cookieJar = new ConcurrentHashMap<>(); private final AtomicInteger counter = new AtomicInteger(); + // Monotonic per-store stamp giving each stored cookie a strict, tie-free, clock-independent insertion + // order for eviction (see evictExcessCookies). Preferred over creation time, which is millisecond- + // granular (so it ties under a flood) and wall-clock based (an NTP step backward would reorder it). + private final AtomicLong cookieSequence = new AtomicLong(); @Override public void add(Uri uri, Cookie cookie) { @@ -195,7 +208,46 @@ private void add(String requestDomain, String requestPath, Cookie cookie) { cookieJar.getOrDefault(keyDomain, Collections.emptyMap()).remove(key); } else { final Map innerMap = cookieJar.computeIfAbsent(keyDomain, domain -> new ConcurrentHashMap<>()); - innerMap.put(key, new StoredCookie(cookie, hostOnly, cookie.maxAge() != Cookie.UNDEFINED_MAX_AGE)); + innerMap.put(key, new StoredCookie(cookie, hostOnly, cookie.maxAge() != Cookie.UNDEFINED_MAX_AGE, cookieSequence.getAndIncrement())); + if (innerMap.size() > MAX_COOKIES_PER_DOMAIN) { + evictExcessCookies(innerMap); + } + } + } + + /** + * Bounds a single domain's cookie bucket at {@link #MAX_COOKIES_PER_DOMAIN}. RFC 6265 §5.5 permits a + * per-domain cap; §5.3's "remove excess cookies" step evicts expired cookies first, then removes more + * until under the limit. The RFC breaks that second tie by least-recently-accessed; we do not track + * access time, so we deliberately deviate and evict in insertion order via the strict, tie-free + * {@link StoredCookie#seq} stamp. + * + *

Called from {@link #add} right after an insert pushes the bucket over the cap, so it normally + * removes a single entry. A single pass drops expired entries and collects the survivors; only if those + * still exceed the cap are they ordered by {@code seq} and the oldest excess removed. Victims are dropped + * with the two-arg {@code remove(key, value)}, which is identity-based (StoredCookie has no + * {@code equals()}): a cookie another thread just re-put under the same key is never collaterally + * removed. Two adders evicting concurrently pick the same seq-ordered victims, so their redundant removes + * no-op — the bucket may still briefly sit a little below the cap until the next add, but never grows + * unbounded. + */ + private static void evictExcessCookies(Map innerMap) { + List> live = new ArrayList<>(innerMap.size()); + for (Map.Entry entry : innerMap.entrySet()) { + if (hasCookieExpired(entry.getValue().cookie, entry.getValue().createdAt)) { + innerMap.remove(entry.getKey(), entry.getValue()); + } else { + live.add(entry); + } + } + int excess = live.size() - MAX_COOKIES_PER_DOMAIN; + if (excess <= 0) { + return; + } + live.sort(Comparator.comparingLong(entry -> entry.getValue().seq)); + for (int i = 0; i < excess; i++) { + Map.Entry victim = live.get(i); + innerMap.remove(victim.getKey(), victim.getValue()); } } @@ -292,11 +344,14 @@ private static class StoredCookie { final boolean hostOnly; final boolean persistent; final long createdAt = System.currentTimeMillis(); + // Strict, tie-free insertion order for eviction; see ThreadSafeCookieStore.cookieSequence. + final long seq; - StoredCookie(Cookie cookie, boolean hostOnly, boolean persistent) { + StoredCookie(Cookie cookie, boolean hostOnly, boolean persistent, long seq) { this.cookie = cookie; this.hostOnly = hostOnly; this.persistent = persistent; + this.seq = seq; } @Override diff --git a/client/src/test/java/org/asynchttpclient/cookie/ThreadSafeCookieStoreGetTest.java b/client/src/test/java/org/asynchttpclient/cookie/ThreadSafeCookieStoreGetTest.java index 6234549fa..a8db3fd88 100644 --- a/client/src/test/java/org/asynchttpclient/cookie/ThreadSafeCookieStoreGetTest.java +++ b/client/src/test/java/org/asynchttpclient/cookie/ThreadSafeCookieStoreGetTest.java @@ -112,6 +112,73 @@ public void returnsMultipleDistinctCookiesAtSameDomainPath() { assertEquals(setOf("ALPHA=AV", "BETA=BV"), namesValues(store.get(uri))); } + @Test + public void perDomainCookieCountIsCappedUnderFlood() { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + Uri uri = Uri.create("http://www.foo.com/"); + int flood = ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN + 50; + for (int i = 0; i < flood; i++) { + store.add(uri, ClientCookieDecoder.LAX.decode("c" + i + "=v" + i + "; Domain=www.foo.com; Path=/")); + } + assertEquals(ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN, store.getUnderlying().get("www.foo.com").size(), + "a single domain's cookies must be capped at MAX_COOKIES_PER_DOMAIN"); + } + + @Test + public void cookiesUnderTheCapAreAllRetained() { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + Uri uri = Uri.create("http://www.foo.com/"); + for (int i = 0; i < 20; i++) { + store.add(uri, ClientCookieDecoder.LAX.decode("c" + i + "=v" + i + "; Domain=www.foo.com; Path=/")); + } + assertEquals(20, store.getUnderlying().get("www.foo.com").size(), "nothing is evicted below the cap"); + assertEquals(20, store.get(uri).size()); + } + + @Test + public void capIsPerDomainNotGlobal() { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + Uri foo = Uri.create("http://www.foo.com/"); + for (int i = 0; i < ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN + 20; i++) { + store.add(foo, ClientCookieDecoder.LAX.decode("c" + i + "=v" + i + "; Domain=www.foo.com; Path=/")); + } + store.add(Uri.create("http://www.bar.com/"), + ClientCookieDecoder.LAX.decode("only=1; Domain=www.bar.com; Path=/")); + + assertEquals(ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN, store.getUnderlying().get("www.foo.com").size()); + assertEquals(1, store.getUnderlying().get("www.bar.com").size(), + "flooding one domain must not evict another domain's cookies"); + } + + @Test + public void evictionDropsExpiredCookiesBeforeLiveOnes() throws InterruptedException { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + Uri uri = Uri.create("http://www.foo.com/"); + int cap = ThreadSafeCookieStore.MAX_COOKIES_PER_DOMAIN; + int live = cap - 5; + + // Fill the bucket exactly to the cap: (cap - 5) session cookies that never expire ... + for (int i = 0; i < live; i++) { + store.add(uri, ClientCookieDecoder.LAX.decode("live" + i + "=v; Domain=www.foo.com; Path=/")); + } + // ... plus 5 short-lived cookies that will expire before the next add. + for (int i = 0; i < 5; i++) { + store.add(uri, ClientCookieDecoder.LAX.decode("exp" + i + "=v; Domain=www.foo.com; Path=/; Max-Age=1")); + } + assertEquals(cap, store.getUnderlying().get("www.foo.com").size(), "precondition: bucket filled to the cap"); + + // Max-Age is second-granular, so let > 1s pass for the five short-lived cookies to expire. + Thread.sleep(2100); + + // This add pushes the bucket over the cap and triggers eviction. RFC 6265 §5.3 drops expired + // cookies first, so all five expired ones go and no live cookie is evicted. + store.add(uri, ClientCookieDecoder.LAX.decode("trigger=v; Domain=www.foo.com; Path=/")); + + assertEquals(live + 1, store.getUnderlying().get("www.foo.com").size(), + "eviction must drop the expired cookies first, leaving every live cookie in place"); + assertEquals(live + 1, store.get(uri).size(), "all live cookies (and only those) remain retrievable"); + } + @Test public void returnsEmptyForUnknownDomain() { ThreadSafeCookieStore store = new ThreadSafeCookieStore();