Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@

public final class ThreadSafeCookieStore implements CookieStore {

// RFC 6265 §5.3 (step 12) lets a user agent cap the cookies it retains. Bounding cookies per domain

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RFC pointer is off. §5.3's storage-model algorithm has no numbered "step 12". The per-domain number limit lives in §5.5 (Implementation Limits, "at least 50 cookies per domain"), and the eviction behavior is the unnumbered "remove excess cookies" prose in §5.3. Also worth noting the RFC breaks ties by least-recently-accessed, not creation time, so we're deviating here on purpose. Suggest citing §5.5 plus §5.3 and saying so in one line. Same fix applies to the javadoc on evictExcessCookies.

// 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. Package-private for tests.
static final int MAX_COOKIES_PER_DOMAIN = 200;

private final Map<String, Map<CookieKey, StoredCookie>> cookieJar = new ConcurrentHashMap<>();
private final AtomicInteger counter = new AtomicInteger();

Expand Down Expand Up @@ -196,6 +202,33 @@ private void add(String requestDomain, String requestPath, Cookie cookie) {
} else {
final Map<CookieKey, StoredCookie> innerMap = cookieJar.computeIfAbsent(keyDomain, domain -> new ConcurrentHashMap<>());
innerMap.put(key, new StoredCookie(cookie, hostOnly, cookie.maxAge() != Cookie.UNDEFINED_MAX_AGE));
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.3 step 12).
* Evicts expired entries first, then the oldest by creation time until back at the cap. Called from
* {@link #add} right after an insert pushes the bucket over the cap, so it normally removes a single
* entry. Concurrency-safe: entries are dropped with the two-arg {@code remove(key, value)} so a cookie
* another thread just replaced under the same key is never collaterally removed; a benign race between
* concurrent adders may evict one extra, which is self-correcting.
*/
private static void evictExcessCookies(Map<CookieKey, StoredCookie> innerMap) {
innerMap.entrySet().removeIf(entry -> hasCookieExpired(entry.getValue().cookie, entry.getValue().createdAt));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this re-scans all ~200 entries and calls System.currentTimeMillis() on each, on every over-cap add, even when nothing is expired. Fine at n=200, just flagging that the 200/201 oscillation means every steady-state add pays a full eviction pass with no low-water hysteresis. Not worth optimizing on its own, but the single-pass sequence-number approach would fold this in.

while (innerMap.size() > MAX_COOKIES_PER_DOMAIN) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two threads can both observe size == 201, both enter here, and each remove a different oldest entry, so the bucket drops to 199 and we silently evict a valid in-cap cookie. Cookie saves run on Netty IO threads, so two concurrent same-origin responses hitting add() is realistic. It's bounded and the victims are the oldest anyway, so not a blocker, but the PR note ("may evict one extra, self-correcting") undersells it: the store can sit below the cap, not just at it. The sequence-number idea below shrinks this window.

Map.Entry<CookieKey, StoredCookie> oldest = null;
for (Map.Entry<CookieKey, StoredCookie> entry : innerMap.entrySet()) {
if (oldest == null || entry.getValue().createdAt < oldest.getValue().createdAt) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering by createdAt is weak. It's millisecond granularity, so under a flood most cookies land in the same millisecond and "oldest" becomes whatever ConcurrentHashMap iteration hits first, effectively random. It's also wall-clock based, so an NTP step backward breaks the order. A per-store AtomicLong stamped onto each StoredCookie at construction gives a strict, tie-free order at basically zero cost, and lets you grab the k smallest in a single pass instead of this O(n) rescan on every removal. That would also tighten the over-eviction race above

oldest = entry;
}
}
if (oldest == null) {
break;
}
innerMap.remove(oldest.getKey(), oldest.getValue());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on the two-arg remove. Add a one-liner noting it only works because StoredCookie has no equals(), so this is identity based and won't clobber a cookie another thread just re-put under the same key. That invariant is load-bearing and easy to break by accident.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,44 @@ public void returnsMultipleDistinctCookiesAtSameDomainPath() {
assertEquals(setOf("ALPHA=AV", "BETA=BV"), namesValues(store.get(uri)));
}

@Test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One gap: consider a test that mixes expired and live cookies over the cap and asserts eviction drops the expired ones first, to lock in the RFC priority order the code implements.

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 returnsEmptyForUnknownDomain() {
ThreadSafeCookieStore store = new ThreadSafeCookieStore();
Expand Down
Loading