From b5500b494acf0015ad2caad4e435fd48ba54e0c7 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Wed, 18 Mar 2020 18:42:05 -0400 Subject: [PATCH 01/15] making domain as the key for fast lookups. --- .../cookie/ThreadSafeCookieStore.java | 90 ++++++++++++++----- .../org/asynchttpclient/CookieStoreTest.java | 6 +- 2 files changed, 71 insertions(+), 25 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index 277db387ce..d47b123a29 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -21,12 +21,14 @@ import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import java.util.function.Predicate; import java.util.stream.Collectors; public final class ThreadSafeCookieStore implements CookieStore { - private Map cookieJar = new ConcurrentHashMap<>(); + private Map> cookieJar = new ConcurrentHashMap<>(); @Override public void add(Uri uri, Cookie cookie) { @@ -45,15 +47,16 @@ public List get(Uri uri) { public List getAll() { final boolean[] removeExpired = {false}; List result = cookieJar - .entrySet() + .values() .stream() + .flatMap(map -> map.values().stream()) .filter(pair -> { - boolean hasCookieExpired = hasCookieExpired(pair.getValue().cookie, pair.getValue().createdAt); + boolean hasCookieExpired = hasCookieExpired(pair.cookie, pair.createdAt); if (hasCookieExpired && !removeExpired[0]) removeExpired[0] = true; return !hasCookieExpired; }) - .map(pair -> pair.getValue().cookie) + .map(pair -> pair.cookie) .collect(Collectors.toList()); if (removeExpired[0]) @@ -64,7 +67,13 @@ public List getAll() { @Override public boolean remove(Predicate predicate) { - return cookieJar.entrySet().removeIf(v -> predicate.test(v.getValue().cookie)); + final boolean[] removed = {false}; + cookieJar.values().forEach(cookieMap -> { + if (!removed[0]) { + removed[0] = cookieMap.values().removeIf(v -> predicate.test(v.cookie)); + } + }); + return removed[0]; } @Override @@ -145,45 +154,66 @@ private void add(String requestDomain, String requestPath, Cookie cookie) { String keyDomain = pair.getKey(); boolean hostOnly = pair.getValue(); String keyPath = cookiePath(cookie.path(), requestPath); - CookieKey key = new CookieKey(cookie.name().toLowerCase(), keyDomain, keyPath); + CookieKey key = new CookieKey(cookie.name().toLowerCase(), keyPath); if (hasCookieExpired(cookie, 0)) - cookieJar.remove(key); - else - cookieJar.put(key, new StoredCookie(cookie, hostOnly, cookie.maxAge() != Cookie.UNDEFINED_MAX_AGE)); + cookieJar.getOrDefault(keyDomain, Collections.emptyMap()).remove(key); + else { + cookieJar.putIfAbsent(keyDomain, new ConcurrentHashMap<>()); + cookieJar.get(keyDomain).put(key, new StoredCookie(cookie, hostOnly, cookie.maxAge() != Cookie.UNDEFINED_MAX_AGE)); + } } private List get(String domain, String path, boolean secure) { final boolean[] removeExpired = {false}; + boolean exactDomainMatch = true; + String subDomain = domain; + + final List result = new ArrayList<>(); + while (subDomain != null) { + result.addAll(getStoredCookies(subDomain, path, secure, removeExpired, exactDomainMatch)); + exactDomainMatch = false; + subDomain = DomainUtils.getHigherDomain(subDomain); + } + + if (removeExpired[0]) + removeExpired(); - List result = cookieJar.entrySet().stream().filter(pair -> { + return result; + } + + private List getStoredCookies(String domain, String path, boolean secure, + boolean[] removeExpired, boolean isExactMatch) { + if (!cookieJar.containsKey(domain)) { + return Collections.emptyList(); + } + + return cookieJar.get(domain).entrySet().stream().filter(pair -> { CookieKey key = pair.getKey(); StoredCookie storedCookie = pair.getValue(); boolean hasCookieExpired = hasCookieExpired(storedCookie.cookie, storedCookie.createdAt); if (hasCookieExpired && !removeExpired[0]) removeExpired[0] = true; - return !hasCookieExpired && domainsMatch(key.domain, domain, storedCookie.hostOnly) && pathsMatch(key.path, path) && (secure || !storedCookie.cookie.isSecure()); + return !hasCookieExpired && + (isExactMatch || !storedCookie.hostOnly) && + pathsMatch(key.path, path) && + (secure || !storedCookie.cookie.isSecure()); }).map(v -> v.getValue().cookie).collect(Collectors.toList()); - - if (removeExpired[0]) - removeExpired(); - - return result; } private void removeExpired() { - cookieJar.entrySet().removeIf(v -> hasCookieExpired(v.getValue().cookie, v.getValue().createdAt)); + cookieJar.values().forEach(cookieMap -> { + cookieMap.values().removeIf(v -> hasCookieExpired(v.cookie, v.createdAt)); + }); } private static class CookieKey implements Comparable { final String name; - final String domain; final String path; - CookieKey(String name, String domain, String path) { + CookieKey(String name, String path) { this.name = name; - this.domain = domain; this.path = path; } @@ -192,7 +222,6 @@ public int compareTo(CookieKey o) { Assertions.assertNotNull(o, "Parameter can't be null"); int result; if ((result = this.name.compareTo(o.name)) == 0) - if ((result = this.domain.compareTo(o.domain)) == 0) result = this.path.compareTo(o.path); return result; @@ -207,14 +236,13 @@ public boolean equals(Object obj) { public int hashCode() { int result = 17; result = 31 * result + name.hashCode(); - result = 31 * result + domain.hashCode(); result = 31 * result + path.hashCode(); return result; } @Override public String toString() { - return String.format("%s: %s; %s", name, domain, path); + return String.format("%s: %s", name, path); } } @@ -235,4 +263,20 @@ public String toString() { return String.format("%s; hostOnly %s; persistent %s", cookie.toString(), hostOnly, persistent); } } + + public static final class DomainUtils { + private static final char DOT = '.'; + public static String getHigherDomain(String domain) { + if (domain == null || domain.isEmpty()) { + return null; + } + final int indexOfDot = domain.indexOf(DOT); + if (indexOfDot == -1) { + return null; + } + return domain.substring(indexOfDot + 1); + } + + private DomainUtils() {} + } } diff --git a/client/src/test/java/org/asynchttpclient/CookieStoreTest.java b/client/src/test/java/org/asynchttpclient/CookieStoreTest.java index e16a477c25..964d7375bf 100644 --- a/client/src/test/java/org/asynchttpclient/CookieStoreTest.java +++ b/client/src/test/java/org/asynchttpclient/CookieStoreTest.java @@ -27,6 +27,7 @@ import org.testng.annotations.Test; import java.util.List; +import java.util.stream.Collectors; import static org.testng.Assert.assertTrue; @@ -284,8 +285,9 @@ private void returnMultipleCookiesEvenIfTheyHaveSameName() { assertTrue(cookies1.size() == 2); assertTrue(cookies1.stream().filter(c -> c.value().equals("FOO") || c.value().equals("BAR")).count() == 2); - String result = ClientCookieEncoder.LAX.encode(cookies1.get(0), cookies1.get(1)); - assertTrue(result.equals("JSESSIONID=FOO; JSESSIONID=BAR")); + List encodedCookieStrings = cookies1.stream().map(ClientCookieEncoder.LAX::encode).collect(Collectors.toList()); + assertTrue(encodedCookieStrings.contains("JSESSIONID=FOO")); + assertTrue(encodedCookieStrings.contains("JSESSIONID=BAR")); } // rfc6265#section-1 Cookies for a given host are shared across all the ports on that host From ee75fa9e57cf1bc2b09e2d65ab2633de92f6a238 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Wed, 18 Mar 2020 18:42:28 -0400 Subject: [PATCH 02/15] removing unnecessary imports added by me. --- .../java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index d47b123a29..cfba9c0e98 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -21,8 +21,6 @@ import java.util.*; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; import java.util.function.Predicate; import java.util.stream.Collectors; From 27849507ba930638d5c0efbd7bb217410527d1e2 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Wed, 18 Mar 2020 19:44:28 -0400 Subject: [PATCH 03/15] added a single thread service to cleanup expired cookies. --- .../DefaultAsyncHttpClient.java | 8 +++ .../asynchttpclient/cookie/CookieStore.java | 3 +- .../cookie/ThreadSafeCookieStore.java | 70 ++++++++++++------- 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java index 18425801fc..17da6586a0 100644 --- a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java +++ b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java @@ -31,6 +31,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.IOException; import java.util.List; import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; @@ -114,6 +115,13 @@ public void close() { LOGGER.warn("Unexpected error on HashedWheelTimer close", t); } } + if (config.getCookieStore() != null) { + try { + config.getCookieStore().close(); + } catch (IOException e) { + LOGGER.warn("IOException closing CookieStore", e); + } + } } } diff --git a/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java index 0c5ad544ed..0d126ea6cb 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java @@ -17,6 +17,7 @@ import io.netty.handler.codec.http.cookie.Cookie; import org.asynchttpclient.uri.Uri; +import java.io.Closeable; import java.net.CookieManager; import java.util.List; import java.util.function.Predicate; @@ -31,7 +32,7 @@ * * @since 2.1 */ -public interface CookieStore { +public interface CookieStore extends Closeable { /** * Adds one {@link Cookie} to the store. This is called for every incoming HTTP response. * If the given cookie has already expired it will not be added, but existing values will still be removed. diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index cfba9c0e98..82178c4f8c 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -18,15 +18,24 @@ import org.asynchttpclient.uri.Uri; import org.asynchttpclient.util.Assertions; import org.asynchttpclient.util.MiscUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import java.io.Closeable; +import java.io.IOException; import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; import java.util.function.Predicate; import java.util.stream.Collectors; -public final class ThreadSafeCookieStore implements CookieStore { +public final class ThreadSafeCookieStore implements CookieStore, Closeable { + private static final Logger log = LoggerFactory.getLogger(ThreadSafeCookieStore.class); - private Map> cookieJar = new ConcurrentHashMap<>(); + private final Map> cookieJar = new ConcurrentHashMap<>(); + private final ScheduledExecutorService scheduledExecutor = Executors.newScheduledThreadPool(1); @Override public void add(Uri uri, Cookie cookie) { @@ -36,6 +45,20 @@ public void add(Uri uri, Cookie cookie) { add(thisRequestDomain, thisRequestPath, cookie); } + public ThreadSafeCookieStore() { + scheduleExpiredCookiesRemoval(); + } + + private void scheduleExpiredCookiesRemoval() { + scheduledExecutor.scheduleWithFixedDelay(() -> { + if (cookieJar.isEmpty()) { + return; + } + + removeExpired(); + }, 10, 10, TimeUnit.SECONDS); + } + @Override public List get(Uri uri) { return get(requestDomain(uri), requestPath(uri), uri.isSecured()); @@ -57,18 +80,15 @@ public List getAll() { .map(pair -> pair.cookie) .collect(Collectors.toList()); - if (removeExpired[0]) - removeExpired(); - return result; } @Override public boolean remove(Predicate predicate) { final boolean[] removed = {false}; - cookieJar.values().forEach(cookieMap -> { + cookieJar.entrySet().forEach(cookieMap -> { if (!removed[0]) { - removed[0] = cookieMap.values().removeIf(v -> predicate.test(v.cookie)); + removed[0] = cookieMap.getValue().entrySet().removeIf(v -> predicate.test(v.getValue().cookie)); } }); return removed[0]; @@ -133,13 +153,6 @@ private boolean hasCookieExpired(Cookie cookie, long whenCreated) { return false; } - // rfc6265#section-5.1.3 - // check "The string is a host name (i.e., not an IP address)" ignored - private boolean domainsMatch(String cookieDomain, String requestDomain, boolean hostOnly) { - return (hostOnly && Objects.equals(requestDomain, cookieDomain)) || - (Objects.equals(requestDomain, cookieDomain) || requestDomain.endsWith("." + cookieDomain)); - } - // rfc6265#section-5.1.4 private boolean pathsMatch(String cookiePath, String requestPath) { return Objects.equals(cookiePath, requestPath) || @@ -165,20 +178,17 @@ private void add(String requestDomain, String requestPath, Cookie cookie) { private List get(String domain, String path, boolean secure) { final boolean[] removeExpired = {false}; + final List results = new ArrayList<>(); boolean exactDomainMatch = true; String subDomain = domain; - final List result = new ArrayList<>(); - while (subDomain != null) { - result.addAll(getStoredCookies(subDomain, path, secure, removeExpired, exactDomainMatch)); + while (MiscUtils.isNonEmpty(subDomain)) { + results.addAll(getStoredCookies(subDomain, path, secure, removeExpired, exactDomainMatch)); exactDomainMatch = false; - subDomain = DomainUtils.getHigherDomain(subDomain); + subDomain = DomainUtils.getSubDomain(subDomain); } - if (removeExpired[0]) - removeExpired(); - - return result; + return results; } private List getStoredCookies(String domain, String path, boolean secure, @@ -202,8 +212,20 @@ private List getStoredCookies(String domain, String path, boolean secure private void removeExpired() { cookieJar.values().forEach(cookieMap -> { - cookieMap.values().removeIf(v -> hasCookieExpired(v.cookie, v.createdAt)); + cookieMap.entrySet().removeIf(v -> hasCookieExpired(v.getValue().cookie, v.getValue().createdAt)); }); + cookieJar.entrySet().removeIf(entry -> entry.getValue() == null || entry.getValue().isEmpty()); + } + + @Override + public void close() throws IOException { + scheduledExecutor.shutdown(); + try { + scheduledExecutor.awaitTermination(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + log.debug("Interrupted while shutting down " + ThreadSafeCookieStore.class.getName()); + Thread.currentThread().interrupt(); + } } private static class CookieKey implements Comparable { @@ -264,7 +286,7 @@ public String toString() { public static final class DomainUtils { private static final char DOT = '.'; - public static String getHigherDomain(String domain) { + public static String getSubDomain(String domain) { if (domain == null || domain.isEmpty()) { return null; } From 3f01937cbe083b20727820d8cf830dce1f87b26e Mon Sep 17 00:00:00 2001 From: sivaalli Date: Wed, 18 Mar 2020 19:44:49 -0400 Subject: [PATCH 04/15] removing redundant interface declaration --- .../java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index 82178c4f8c..0e5b19ebe0 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -31,7 +31,7 @@ import java.util.function.Predicate; import java.util.stream.Collectors; -public final class ThreadSafeCookieStore implements CookieStore, Closeable { +public final class ThreadSafeCookieStore implements CookieStore { private static final Logger log = LoggerFactory.getLogger(ThreadSafeCookieStore.class); private final Map> cookieJar = new ConcurrentHashMap<>(); From 6b81e1a3d8af83d84bf9652578a898f00fd1ea58 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Wed, 18 Mar 2020 19:45:04 -0400 Subject: [PATCH 05/15] removing unnecessary import --- .../java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java | 1 - 1 file changed, 1 deletion(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index 0e5b19ebe0..cb4dce1f21 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -21,7 +21,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.Closeable; import java.io.IOException; import java.util.*; import java.util.concurrent.ConcurrentHashMap; From dff16e665620b2349a3a02a1fa8ad6bcb506a856 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Wed, 18 Mar 2020 20:34:28 -0400 Subject: [PATCH 06/15] not removing cookies during retrieval operations --- .../cookie/ThreadSafeCookieStore.java | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index cb4dce1f21..145b2fbabb 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -65,17 +65,11 @@ public List get(Uri uri) { @Override public List getAll() { - final boolean[] removeExpired = {false}; List result = cookieJar .values() .stream() .flatMap(map -> map.values().stream()) - .filter(pair -> { - boolean hasCookieExpired = hasCookieExpired(pair.cookie, pair.createdAt); - if (hasCookieExpired && !removeExpired[0]) - removeExpired[0] = true; - return !hasCookieExpired; - }) + .filter(pair -> !hasCookieExpired(pair.cookie, pair.createdAt)) .map(pair -> pair.cookie) .collect(Collectors.toList()); @@ -85,9 +79,9 @@ public List getAll() { @Override public boolean remove(Predicate predicate) { final boolean[] removed = {false}; - cookieJar.entrySet().forEach(cookieMap -> { + cookieJar.forEach((key, value) -> { if (!removed[0]) { - removed[0] = cookieMap.getValue().entrySet().removeIf(v -> predicate.test(v.getValue().cookie)); + removed[0] = value.entrySet().removeIf(v -> predicate.test(v.getValue().cookie)); } }); return removed[0]; @@ -200,8 +194,6 @@ private List getStoredCookies(String domain, String path, boolean secure CookieKey key = pair.getKey(); StoredCookie storedCookie = pair.getValue(); boolean hasCookieExpired = hasCookieExpired(storedCookie.cookie, storedCookie.createdAt); - if (hasCookieExpired && !removeExpired[0]) - removeExpired[0] = true; return !hasCookieExpired && (isExactMatch || !storedCookie.hostOnly) && pathsMatch(key.path, path) && From cedd3c99c26be91f175fc1eccf78d5f52a481096 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Wed, 18 Mar 2020 20:39:00 -0400 Subject: [PATCH 07/15] removing from outermap only if inner map entries are removed. --- .../cookie/ThreadSafeCookieStore.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index 145b2fbabb..c7340e03e0 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -84,6 +84,9 @@ public boolean remove(Predicate predicate) { removed[0] = value.entrySet().removeIf(v -> predicate.test(v.getValue().cookie)); } }); + if (removed[0]) { + cookieJar.entrySet().removeIf(entry -> entry.getValue() == null || entry.getValue().isEmpty()); + } return removed[0]; } @@ -202,10 +205,15 @@ private List getStoredCookies(String domain, String path, boolean secure } private void removeExpired() { + final boolean[] removed = {false}; cookieJar.values().forEach(cookieMap -> { - cookieMap.entrySet().removeIf(v -> hasCookieExpired(v.getValue().cookie, v.getValue().createdAt)); + if (!removed[0]) { + removed[0] = cookieMap.entrySet().removeIf(v -> hasCookieExpired(v.getValue().cookie, v.getValue().createdAt)); + } }); - cookieJar.entrySet().removeIf(entry -> entry.getValue() == null || entry.getValue().isEmpty()); + if (removed[0]) { + cookieJar.entrySet().removeIf(entry -> entry.getValue() == null || entry.getValue().isEmpty()); + } } @Override From a0c036581ccb2c30114820a432254ae37d180c2f Mon Sep 17 00:00:00 2001 From: sivaalli Date: Sat, 28 Mar 2020 22:10:31 -0400 Subject: [PATCH 08/15] adding config to get the delay in ms and also making a CookieEvictionTask to evict expired cookies. --- .../AsyncHttpClientConfig.java | 7 +++ .../DefaultAsyncHttpClient.java | 11 ++-- .../DefaultAsyncHttpClientConfig.java | 15 ++++++ .../config/AsyncHttpClientConfigDefaults.java | 5 ++ .../cookie/CookieEvictionTask.java | 30 +++++++++++ .../asynchttpclient/cookie/CookieStore.java | 9 +++- .../cookie/ThreadSafeCookieStore.java | 51 +++++-------------- .../config/ahc-default.properties | 1 + .../org/asynchttpclient/CookieStoreTest.java | 30 ++++++++++- .../AsyncHttpClientTypesafeConfig.java | 5 ++ 10 files changed, 115 insertions(+), 49 deletions(-) create mode 100644 client/src/main/java/org/asynchttpclient/cookie/CookieEvictionTask.java diff --git a/client/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java b/client/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java index 1e0e2ed809..ccfe9679d4 100644 --- a/client/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java +++ b/client/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java @@ -198,6 +198,13 @@ public interface AsyncHttpClientConfig { */ CookieStore getCookieStore(); + /** + * Return the delay in milliseconds to evict expired cookies from {@linkplain CookieStore} + * + * @return the delay in milliseconds to evict expired cookies from {@linkplain CookieStore} + */ + int expiredCookieEvictionDelay(); + /** * Return the number of time the library will retry when an {@link java.io.IOException} is throw by the remote server * diff --git a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java index 17da6586a0..d78bb96a17 100644 --- a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java +++ b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java @@ -22,6 +22,7 @@ import io.netty.util.Timer; import io.netty.util.concurrent.DefaultThreadFactory; import org.asynchttpclient.channel.ChannelPool; +import org.asynchttpclient.cookie.CookieEvictionTask; import org.asynchttpclient.filter.FilterContext; import org.asynchttpclient.filter.FilterException; import org.asynchttpclient.filter.RequestFilter; @@ -31,7 +32,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.IOException; import java.util.List; import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; @@ -91,6 +91,8 @@ public DefaultAsyncHttpClient(AsyncHttpClientConfig config) { channelManager = new ChannelManager(config, nettyTimer); requestSender = new NettyRequestSender(config, channelManager, nettyTimer, new AsyncHttpClientState(closed)); channelManager.configureBootstraps(requestSender); + nettyTimer.newTimeout(new CookieEvictionTask(config.expiredCookieEvictionDelay(), config.getCookieStore()), + config.expiredCookieEvictionDelay(), TimeUnit.MILLISECONDS); } private Timer newNettyTimer(AsyncHttpClientConfig config) { @@ -115,13 +117,6 @@ public void close() { LOGGER.warn("Unexpected error on HashedWheelTimer close", t); } } - if (config.getCookieStore() != null) { - try { - config.getCookieStore().close(); - } catch (IOException e) { - LOGGER.warn("IOException closing CookieStore", e); - } - } } } diff --git a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java index daf043356a..0f4e62c560 100644 --- a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java +++ b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java @@ -109,6 +109,7 @@ public class DefaultAsyncHttpClientConfig implements AsyncHttpClientConfig { // cookie store private final CookieStore cookieStore; + private final int expiredCookieEvictionDelay; // internals private final String threadPoolName; @@ -192,6 +193,7 @@ private DefaultAsyncHttpClientConfig(// http // cookie store CookieStore cookieStore, + int expiredCookieEvictionDelay, // tuning boolean tcpNoDelay, @@ -283,6 +285,7 @@ private DefaultAsyncHttpClientConfig(// http // cookie store this.cookieStore = cookieStore; + this.expiredCookieEvictionDelay = expiredCookieEvictionDelay; // tuning this.tcpNoDelay = tcpNoDelay; @@ -558,6 +561,11 @@ public CookieStore getCookieStore() { return cookieStore; } + @Override + public int expiredCookieEvictionDelay() { + return expiredCookieEvictionDelay; + } + // tuning @Override public boolean isTcpNoDelay() { @@ -746,6 +754,7 @@ public static class Builder { // cookie store private CookieStore cookieStore = new ThreadSafeCookieStore(); + private int expiredCookieEvictionDelay = defaultExpiredCookieEvictionDelay(); // tuning private boolean tcpNoDelay = defaultTcpNoDelay(); @@ -1146,6 +1155,11 @@ public Builder setCookieStore(CookieStore cookieStore) { return this; } + public Builder setExpiredCookieEvictionDelay(int expiredCookieEvictionDelay) { + this.expiredCookieEvictionDelay = expiredCookieEvictionDelay; + return this; + } + // tuning public Builder setTcpNoDelay(boolean tcpNoDelay) { this.tcpNoDelay = tcpNoDelay; @@ -1330,6 +1344,7 @@ public DefaultAsyncHttpClientConfig build() { responseFilters.isEmpty() ? Collections.emptyList() : Collections.unmodifiableList(responseFilters), ioExceptionFilters.isEmpty() ? Collections.emptyList() : Collections.unmodifiableList(ioExceptionFilters), cookieStore, + expiredCookieEvictionDelay, tcpNoDelay, soReuseAddress, soKeepAlive, diff --git a/client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigDefaults.java b/client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigDefaults.java index 641c37d538..14dcec3bfd 100644 --- a/client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigDefaults.java +++ b/client/src/main/java/org/asynchttpclient/config/AsyncHttpClientConfigDefaults.java @@ -73,6 +73,7 @@ public final class AsyncHttpClientConfigDefaults { public static final String IO_THREADS_COUNT_CONFIG = "ioThreadsCount"; public static final String HASHED_WHEEL_TIMER_TICK_DURATION = "hashedWheelTimerTickDuration"; public static final String HASHED_WHEEL_TIMER_SIZE = "hashedWheelTimerSize"; + public static final String EXPIRED_COOKIE_EVICTION_DELAY = "expiredCookieEvictionDelay"; public static final String AHC_VERSION; @@ -304,4 +305,8 @@ public static int defaultHashedWheelTimerTickDuration() { public static int defaultHashedWheelTimerSize() { return AsyncHttpClientConfigHelper.getAsyncHttpClientConfig().getInt(ASYNC_CLIENT_CONFIG_ROOT + HASHED_WHEEL_TIMER_SIZE); } + + public static int defaultExpiredCookieEvictionDelay() { + return AsyncHttpClientConfigHelper.getAsyncHttpClientConfig().getInt(ASYNC_CLIENT_CONFIG_ROOT + EXPIRED_COOKIE_EVICTION_DELAY); + } } diff --git a/client/src/main/java/org/asynchttpclient/cookie/CookieEvictionTask.java b/client/src/main/java/org/asynchttpclient/cookie/CookieEvictionTask.java new file mode 100644 index 0000000000..be32e20b0b --- /dev/null +++ b/client/src/main/java/org/asynchttpclient/cookie/CookieEvictionTask.java @@ -0,0 +1,30 @@ +package org.asynchttpclient.cookie; + +import java.util.concurrent.TimeUnit; + +import org.asynchttpclient.AsyncHttpClientConfig; + +import io.netty.util.Timeout; +import io.netty.util.TimerTask; + +/** + * Evicts expired cookies from the {@linkplain CookieStore} periodically. + * The default delay is 30 seconds. Ypu may override the default using + * {@linkplain AsyncHttpClientConfig#expiredCookieEvictionDelay()}. + */ +public class CookieEvictionTask implements TimerTask { + + private long evictDelayInMs; + private CookieStore cookieStore; + + public CookieEvictionTask(long evictDelayInMs, CookieStore cookieStore) { + this.evictDelayInMs = evictDelayInMs; + this.cookieStore = cookieStore; + } + + @Override + public void run(Timeout timeout) throws Exception { + cookieStore.evictExpired(); + timeout.timer().newTimeout(this, evictDelayInMs, TimeUnit.MILLISECONDS); + } +} diff --git a/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java index 0d126ea6cb..d0fed66f88 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java @@ -32,10 +32,10 @@ * * @since 2.1 */ -public interface CookieStore extends Closeable { +public interface CookieStore { /** * Adds one {@link Cookie} to the store. This is called for every incoming HTTP response. - * If the given cookie has already expired it will not be added, but existing values will still be removed. + * If the given cookie has already expired it will not be added. * *

A cookie to store may or may not be associated with an URI. If it * is not associated with an URI, the cookie's domain and path attribute @@ -83,4 +83,9 @@ public interface CookieStore extends Closeable { * @return true if any cookies were purged. */ boolean clear(); + + /** + * Evicts all the cookies that expired as of the time this method is run. + */ + void evictExpired(); } diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index c7340e03e0..46c00c064c 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -15,26 +15,19 @@ package org.asynchttpclient.cookie; import io.netty.handler.codec.http.cookie.Cookie; + import org.asynchttpclient.uri.Uri; import org.asynchttpclient.util.Assertions; import org.asynchttpclient.util.MiscUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import java.io.IOException; import java.util.*; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; import java.util.function.Predicate; import java.util.stream.Collectors; public final class ThreadSafeCookieStore implements CookieStore { - private static final Logger log = LoggerFactory.getLogger(ThreadSafeCookieStore.class); private final Map> cookieJar = new ConcurrentHashMap<>(); - private final ScheduledExecutorService scheduledExecutor = Executors.newScheduledThreadPool(1); @Override public void add(Uri uri, Cookie cookie) { @@ -44,20 +37,6 @@ public void add(Uri uri, Cookie cookie) { add(thisRequestDomain, thisRequestPath, cookie); } - public ThreadSafeCookieStore() { - scheduleExpiredCookiesRemoval(); - } - - private void scheduleExpiredCookiesRemoval() { - scheduledExecutor.scheduleWithFixedDelay(() -> { - if (cookieJar.isEmpty()) { - return; - } - - removeExpired(); - }, 10, 10, TimeUnit.SECONDS); - } - @Override public List get(Uri uri) { return get(requestDomain(uri), requestPath(uri), uri.isSecured()); @@ -97,8 +76,18 @@ public boolean clear() { return result; } + @Override + public void evictExpired() { + removeExpired(); + } + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + // Visible for test + public Map> getUnderlying() { + return new HashMap<>(cookieJar); + } + private String requestDomain(Uri requestUri) { return requestUri.getHost().toLowerCase(); } @@ -206,27 +195,13 @@ private List getStoredCookies(String domain, String path, boolean secure private void removeExpired() { final boolean[] removed = {false}; - cookieJar.values().forEach(cookieMap -> { - if (!removed[0]) { - removed[0] = cookieMap.entrySet().removeIf(v -> hasCookieExpired(v.getValue().cookie, v.getValue().createdAt)); - } - }); + cookieJar.values().forEach(cookieMap -> removed[0] |= cookieMap.entrySet().removeIf( + v -> hasCookieExpired(v.getValue().cookie, v.getValue().createdAt))); if (removed[0]) { cookieJar.entrySet().removeIf(entry -> entry.getValue() == null || entry.getValue().isEmpty()); } } - @Override - public void close() throws IOException { - scheduledExecutor.shutdown(); - try { - scheduledExecutor.awaitTermination(5, TimeUnit.SECONDS); - } catch (InterruptedException e) { - log.debug("Interrupted while shutting down " + ThreadSafeCookieStore.class.getName()); - Thread.currentThread().interrupt(); - } - } - private static class CookieKey implements Comparable { final String name; final String path; diff --git a/client/src/main/resources/org/asynchttpclient/config/ahc-default.properties b/client/src/main/resources/org/asynchttpclient/config/ahc-default.properties index 4c78250445..62bc177726 100644 --- a/client/src/main/resources/org/asynchttpclient/config/ahc-default.properties +++ b/client/src/main/resources/org/asynchttpclient/config/ahc-default.properties @@ -52,3 +52,4 @@ org.asynchttpclient.useNativeTransport=false org.asynchttpclient.ioThreadsCount=0 org.asynchttpclient.hashedWheelTimerTickDuration=100 org.asynchttpclient.hashedWheelTimerSize=512 +org.asynchttpclient.expiredCookieEvictionDelay=30000 diff --git a/client/src/test/java/org/asynchttpclient/CookieStoreTest.java b/client/src/test/java/org/asynchttpclient/CookieStoreTest.java index 964d7375bf..e248e9a0c4 100644 --- a/client/src/test/java/org/asynchttpclient/CookieStoreTest.java +++ b/client/src/test/java/org/asynchttpclient/CookieStoreTest.java @@ -17,6 +17,8 @@ import io.netty.handler.codec.http.cookie.ClientCookieDecoder; import io.netty.handler.codec.http.cookie.ClientCookieEncoder; import io.netty.handler.codec.http.cookie.Cookie; +import io.netty.handler.codec.http.cookie.DefaultCookie; + import org.asynchttpclient.cookie.CookieStore; import org.asynchttpclient.cookie.ThreadSafeCookieStore; import org.asynchttpclient.uri.Uri; @@ -26,11 +28,14 @@ import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; +import java.util.Collection; import java.util.List; import java.util.stream.Collectors; import static org.testng.Assert.assertTrue; +import com.google.common.collect.Sets; + public class CookieStoreTest { private final Logger logger = LoggerFactory.getLogger(getClass()); @@ -47,7 +52,7 @@ public void tearDownGlobal() { } @Test - public void runAllSequentiallyBecauseNotThreadSafe() { + public void runAllSequentiallyBecauseNotThreadSafe() throws Exception { addCookieWithEmptyPath(); dontReturnCookieForAnotherDomain(); returnCookieWhenItWasSetOnSamePath(); @@ -78,6 +83,7 @@ public void runAllSequentiallyBecauseNotThreadSafe() { shouldAlsoServeNonSecureCookiesBasedOnTheUriScheme(); shouldNotServeSecureCookiesForDefaultRetrievedHttpUriScheme(); shouldServeSecureCookiesForSpecificallyRetrievedHttpUriScheme(); + shouldCleanExpiredCookieFromUnderlyingDataStructure(); } private void addCookieWithEmptyPath() { @@ -339,4 +345,26 @@ private void shouldServeSecureCookiesForSpecificallyRetrievedHttpUriScheme() { assertTrue(store.get(uri).get(0).value().equals("VALUE3")); assertTrue(store.get(uri).get(0).isSecure()); } + + private void shouldCleanExpiredCookieFromUnderlyingDataStructure() throws Exception { + ThreadSafeCookieStore store = new ThreadSafeCookieStore(); + store.add(Uri.create("https://foo.org/moodle/"), getCookie("JSESSIONID", "FOO", 1)); + store.add(Uri.create("https://bar.org/moodle/"), getCookie("JSESSIONID", "BAR", 1)); + store.add(Uri.create("https://bar.org/moodle/"), new DefaultCookie("UNEXPIRED_BAR", "BAR")); + store.add(Uri.create("https://foobar.org/moodle/"), new DefaultCookie("UNEXPIRED_FOOBAR", "FOOBAR")); + + + assertTrue(store.getAll().size() == 4); + Thread.sleep(2000); + store.evictExpired(); + assertTrue(store.getUnderlying().size() == 2); + Collection unexpiredCookieNames = store.getAll().stream().map(Cookie::name).collect(Collectors.toList()); + assertTrue(unexpiredCookieNames.containsAll(Sets.newHashSet("UNEXPIRED_BAR", "UNEXPIRED_FOOBAR"))); + } + + private static Cookie getCookie(String key, String value, int maxAge) { + DefaultCookie cookie = new DefaultCookie(key, value); + cookie.setMaxAge(maxAge); + return cookie; + } } diff --git a/extras/typesafeconfig/src/main/java/org/asynchttpclient/extras/typesafeconfig/AsyncHttpClientTypesafeConfig.java b/extras/typesafeconfig/src/main/java/org/asynchttpclient/extras/typesafeconfig/AsyncHttpClientTypesafeConfig.java index 38cd870289..fa5d87bcf3 100644 --- a/extras/typesafeconfig/src/main/java/org/asynchttpclient/extras/typesafeconfig/AsyncHttpClientTypesafeConfig.java +++ b/extras/typesafeconfig/src/main/java/org/asynchttpclient/extras/typesafeconfig/AsyncHttpClientTypesafeConfig.java @@ -164,6 +164,11 @@ public CookieStore getCookieStore() { return new ThreadSafeCookieStore(); } + @Override + public int expiredCookieEvictionDelay() { + return getIntegerOpt(EXPIRED_COOKIE_EVICTION_DELAY).orElse(defaultExpiredCookieEvictionDelay()); + } + @Override public int getMaxRequestRetry() { return getIntegerOpt(MAX_REQUEST_RETRY_CONFIG).orElse(defaultMaxRequestRetry()); From 4152df1daf2cc1cd486cc5bae86a766857c93b77 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Sun, 29 Mar 2020 00:08:14 -0400 Subject: [PATCH 09/15] removing unnecessary imports. --- client/src/main/java/org/asynchttpclient/cookie/CookieStore.java | 1 - .../java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java | 1 - 2 files changed, 2 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java index d0fed66f88..a02bbb1c4e 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java @@ -17,7 +17,6 @@ import io.netty.handler.codec.http.cookie.Cookie; import org.asynchttpclient.uri.Uri; -import java.io.Closeable; import java.net.CookieManager; import java.util.List; import java.util.function.Predicate; diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index 46c00c064c..e8d00eaf16 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -15,7 +15,6 @@ package org.asynchttpclient.cookie; import io.netty.handler.codec.http.cookie.Cookie; - import org.asynchttpclient.uri.Uri; import org.asynchttpclient.util.Assertions; import org.asynchttpclient.util.MiscUtils; From 6ddeb720a9da6e74831e38dd222f0bf8f3a5e13b Mon Sep 17 00:00:00 2001 From: sivaalli Date: Sun, 29 Mar 2020 12:01:00 -0400 Subject: [PATCH 10/15] making necessary suggestions as requested by @slandelle --- .../cookie/ThreadSafeCookieStore.java | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index e8d00eaf16..1c73c6a3fa 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -24,6 +24,8 @@ import java.util.function.Predicate; import java.util.stream.Collectors; +import javax.print.attribute.standard.NumberUp; + public final class ThreadSafeCookieStore implements CookieStore { private final Map> cookieJar = new ConcurrentHashMap<>(); @@ -144,7 +146,6 @@ private boolean pathsMatch(String cookiePath, String requestPath) { } private void add(String requestDomain, String requestPath, Cookie cookie) { - AbstractMap.SimpleEntry pair = cookieDomain(cookie.domain(), requestDomain); String keyDomain = pair.getKey(); boolean hostOnly = pair.getValue(); @@ -154,34 +155,39 @@ private void add(String requestDomain, String requestPath, Cookie cookie) { if (hasCookieExpired(cookie, 0)) cookieJar.getOrDefault(keyDomain, Collections.emptyMap()).remove(key); else { - cookieJar.putIfAbsent(keyDomain, new ConcurrentHashMap<>()); - cookieJar.get(keyDomain).put(key, new StoredCookie(cookie, hostOnly, cookie.maxAge() != Cookie.UNDEFINED_MAX_AGE)); + final Map innerMap = cookieJar.computeIfAbsent(keyDomain, domain -> new ConcurrentHashMap<>()); + innerMap.put(key, new StoredCookie(cookie, hostOnly, cookie.maxAge() != Cookie.UNDEFINED_MAX_AGE)); } } private List get(String domain, String path, boolean secure) { - - final boolean[] removeExpired = {false}; - final List results = new ArrayList<>(); boolean exactDomainMatch = true; String subDomain = domain; + List results = null; while (MiscUtils.isNonEmpty(subDomain)) { - results.addAll(getStoredCookies(subDomain, path, secure, removeExpired, exactDomainMatch)); - exactDomainMatch = false; + final List storedCookies = getStoredCookies(subDomain, path, secure, exactDomainMatch); subDomain = DomainUtils.getSubDomain(subDomain); + exactDomainMatch = false; + if (storedCookies.isEmpty()) { + continue; + } + if (results == null) { + results = new ArrayList<>(4); + } + results.addAll(storedCookies); } - return results; + return results == null ? Collections.emptyList() : results; } - private List getStoredCookies(String domain, String path, boolean secure, - boolean[] removeExpired, boolean isExactMatch) { - if (!cookieJar.containsKey(domain)) { + private List getStoredCookies(String domain, String path, boolean secure, boolean isExactMatch) { + final Map innerMap = cookieJar.get(domain); + if (innerMap == null) { return Collections.emptyList(); } - return cookieJar.get(domain).entrySet().stream().filter(pair -> { + return innerMap.entrySet().stream().filter(pair -> { CookieKey key = pair.getKey(); StoredCookie storedCookie = pair.getValue(); boolean hasCookieExpired = hasCookieExpired(storedCookie.cookie, storedCookie.createdAt); From 747bee9f62f8a723ad27095298ea378f275fb106 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Sun, 29 Mar 2020 12:01:19 -0400 Subject: [PATCH 11/15] removing unnecessary import --- .../java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index 1c73c6a3fa..3cf9ace3bb 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -24,8 +24,6 @@ import java.util.function.Predicate; import java.util.stream.Collectors; -import javax.print.attribute.standard.NumberUp; - public final class ThreadSafeCookieStore implements CookieStore { private final Map> cookieJar = new ConcurrentHashMap<>(); From 4c8f18c5889e620837c5efe32b473135a2cf1347 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Sun, 29 Mar 2020 12:01:59 -0400 Subject: [PATCH 12/15] removing visible for test on public method --- .../java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java | 1 - 1 file changed, 1 deletion(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index 3cf9ace3bb..ae0d775d91 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -82,7 +82,6 @@ public void evictExpired() { //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// - // Visible for test public Map> getUnderlying() { return new HashMap<>(cookieJar); } From 60a2c5b46b66f45a722cfdb8f37a3f1d7046048d Mon Sep 17 00:00:00 2001 From: sivaalli Date: Sun, 29 Mar 2020 12:09:49 -0400 Subject: [PATCH 13/15] fixing a typo --- .../java/org/asynchttpclient/cookie/CookieEvictionTask.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/cookie/CookieEvictionTask.java b/client/src/main/java/org/asynchttpclient/cookie/CookieEvictionTask.java index be32e20b0b..b5ce4aed0a 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/CookieEvictionTask.java +++ b/client/src/main/java/org/asynchttpclient/cookie/CookieEvictionTask.java @@ -9,13 +9,13 @@ /** * Evicts expired cookies from the {@linkplain CookieStore} periodically. - * The default delay is 30 seconds. Ypu may override the default using + * The default delay is 30 seconds. You may override the default using * {@linkplain AsyncHttpClientConfig#expiredCookieEvictionDelay()}. */ public class CookieEvictionTask implements TimerTask { - private long evictDelayInMs; - private CookieStore cookieStore; + private final long evictDelayInMs; + private final CookieStore cookieStore; public CookieEvictionTask(long evictDelayInMs, CookieStore cookieStore) { this.evictDelayInMs = evictDelayInMs; From 1a57d0b7430d3179dedf4fba96f50075674b9c5c Mon Sep 17 00:00:00 2001 From: sivaalli Date: Tue, 31 Mar 2020 21:34:53 -0400 Subject: [PATCH 14/15] scheduling a single taks for shared cookiestore and netty timer. --- .../DefaultAsyncHttpClient.java | 18 ++++- .../asynchttpclient/cookie/CookieStore.java | 3 +- .../cookie/ThreadSafeCookieStore.java | 18 +++++ .../org/asynchttpclient/util/Counted.java | 13 ++++ .../DefaultAsyncHttpClientTest.java | 65 +++++++++++++++++++ 5 files changed, 114 insertions(+), 3 deletions(-) create mode 100644 client/src/main/java/org/asynchttpclient/util/Counted.java create mode 100644 client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java diff --git a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java index d78bb96a17..0e97fcef79 100644 --- a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java +++ b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java @@ -91,8 +91,22 @@ public DefaultAsyncHttpClient(AsyncHttpClientConfig config) { channelManager = new ChannelManager(config, nettyTimer); requestSender = new NettyRequestSender(config, channelManager, nettyTimer, new AsyncHttpClientState(closed)); channelManager.configureBootstraps(requestSender); - nettyTimer.newTimeout(new CookieEvictionTask(config.expiredCookieEvictionDelay(), config.getCookieStore()), - config.expiredCookieEvictionDelay(), TimeUnit.MILLISECONDS); + boolean scheduleCookieEviction = false; + + final int cookieStoreCount = config.getCookieStore().incrementAndGet(); + if (!allowStopNettyTimer) { + if (cookieStoreCount == 1) { + // If this is the first AHC instance for the shared (user-provided) netty timer. + scheduleCookieEviction = true; + } + } else { + // If Timer is not shared. + scheduleCookieEviction = true; + } + if (scheduleCookieEviction) { + nettyTimer.newTimeout(new CookieEvictionTask(config.expiredCookieEvictionDelay(), config.getCookieStore()), + config.expiredCookieEvictionDelay(), TimeUnit.MILLISECONDS); + } } private Timer newNettyTimer(AsyncHttpClientConfig config) { diff --git a/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java index a02bbb1c4e..6cd540226c 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/CookieStore.java @@ -16,6 +16,7 @@ import io.netty.handler.codec.http.cookie.Cookie; import org.asynchttpclient.uri.Uri; +import org.asynchttpclient.util.Counted; import java.net.CookieManager; import java.util.List; @@ -31,7 +32,7 @@ * * @since 2.1 */ -public interface CookieStore { +public interface CookieStore extends Counted { /** * Adds one {@link Cookie} to the store. This is called for every incoming HTTP response. * If the given cookie has already expired it will not be added. diff --git a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java index ae0d775d91..8cdc29f45e 100644 --- a/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java +++ b/client/src/main/java/org/asynchttpclient/cookie/ThreadSafeCookieStore.java @@ -21,12 +21,14 @@ import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Predicate; import java.util.stream.Collectors; public final class ThreadSafeCookieStore implements CookieStore { private final Map> cookieJar = new ConcurrentHashMap<>(); + private final AtomicInteger counter = new AtomicInteger(); @Override public void add(Uri uri, Cookie cookie) { @@ -80,6 +82,22 @@ public void evictExpired() { removeExpired(); } + + @Override + public int incrementAndGet() { + return counter.incrementAndGet(); + } + + @Override + public int decrementAndGet() { + return counter.decrementAndGet(); + } + + @Override + public int count() { + return counter.get(); + } + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// public Map> getUnderlying() { diff --git a/client/src/main/java/org/asynchttpclient/util/Counted.java b/client/src/main/java/org/asynchttpclient/util/Counted.java new file mode 100644 index 0000000000..f13a73d4ee --- /dev/null +++ b/client/src/main/java/org/asynchttpclient/util/Counted.java @@ -0,0 +1,13 @@ +package org.asynchttpclient.util; + +/** + * A contract that has methods to represent how many AHC instances + * the implementation is shared with. + */ +public interface Counted { + int incrementAndGet(); + + int decrementAndGet(); + + int count(); +} diff --git a/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java b/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java new file mode 100644 index 0000000000..58dbedf9fb --- /dev/null +++ b/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java @@ -0,0 +1,65 @@ +package org.asynchttpclient; + +import io.netty.util.HashedWheelTimer; +import io.netty.util.Timer; +import org.asynchttpclient.DefaultAsyncHttpClientConfig.Builder; +import org.asynchttpclient.cookie.CookieEvictionTask; +import org.asynchttpclient.cookie.CookieStore; +import org.asynchttpclient.cookie.ThreadSafeCookieStore; +import org.testng.annotations.Test; + +import java.io.Closeable; +import java.io.IOException; +import java.util.concurrent.TimeUnit; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.*; +import static org.testng.Assert.assertEquals; + +public class DefaultAsyncHttpClientTest { + + @Test + public void testWithSharedNettyTimerShouldScheduleCookieEvictionOnlyOnce() throws IOException { + final Timer nettyTimer = mock(HashedWheelTimer.class); + final CookieStore cookieStore = new ThreadSafeCookieStore(); + final DefaultAsyncHttpClientConfig config = new Builder().setNettyTimer(nettyTimer).setCookieStore(cookieStore).build(); + final AsyncHttpClient client1 = new DefaultAsyncHttpClient(config); + final AsyncHttpClient client2 = new DefaultAsyncHttpClient(config); + + try { + assertEquals(cookieStore.count(), 2); + verify(nettyTimer, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class)); + } finally { + closeSilently(client1); + closeSilently(client2); + } + } + + @Test + public void testWithNonNettyTimerShouldScheduleCookieEvictionForEachAHC() throws IOException { + final AsyncHttpClientConfig config1 = new DefaultAsyncHttpClientConfig.Builder().build(); + final DefaultAsyncHttpClient client1 = new DefaultAsyncHttpClient(config1); + + final AsyncHttpClientConfig config2 = new DefaultAsyncHttpClientConfig.Builder().build(); + final DefaultAsyncHttpClient client2 = new DefaultAsyncHttpClient(config2); + + try { + assertEquals(config1.getCookieStore().count(), 1); + assertEquals(config2.getCookieStore().count(), 1); + } finally { + closeSilently(client1); + closeSilently(client2); + } + } + + private static void closeSilently(Closeable closeable) { + if (closeable != null) { + try { + closeable.close(); + } catch (IOException e) { + // swallow + } + } + } +} From 1437c052388cd9a184dc0006320ab714060e6b08 Mon Sep 17 00:00:00 2001 From: sivaalli Date: Thu, 2 Apr 2020 20:33:30 -0400 Subject: [PATCH 15/15] added another test case and added docs on Counted --- .../org/asynchttpclient/util/Counted.java | 14 ++++- .../DefaultAsyncHttpClientTest.java | 52 ++++++++++++------- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/util/Counted.java b/client/src/main/java/org/asynchttpclient/util/Counted.java index f13a73d4ee..b8791e2fea 100644 --- a/client/src/main/java/org/asynchttpclient/util/Counted.java +++ b/client/src/main/java/org/asynchttpclient/util/Counted.java @@ -1,13 +1,23 @@ package org.asynchttpclient.util; /** - * A contract that has methods to represent how many AHC instances - * the implementation is shared with. + * An interface that defines useful methods to check how many {@linkplain org.asynchttpclient.AsyncHttpClient} + * instances this particular implementation is shared with. */ public interface Counted { + + /** + * Increment counter and return the incremented value + */ int incrementAndGet(); + /** + * Decrement counter and return the decremented value + */ int decrementAndGet(); + /** + * Return the current counter + */ int count(); } diff --git a/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java b/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java index 58dbedf9fb..eadd41226a 100644 --- a/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java +++ b/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java @@ -1,6 +1,5 @@ package org.asynchttpclient; -import io.netty.util.HashedWheelTimer; import io.netty.util.Timer; import org.asynchttpclient.DefaultAsyncHttpClientConfig.Builder; import org.asynchttpclient.cookie.CookieEvictionTask; @@ -21,36 +20,53 @@ public class DefaultAsyncHttpClientTest { @Test public void testWithSharedNettyTimerShouldScheduleCookieEvictionOnlyOnce() throws IOException { - final Timer nettyTimer = mock(HashedWheelTimer.class); + final Timer nettyTimerMock = mock(Timer.class); final CookieStore cookieStore = new ThreadSafeCookieStore(); - final DefaultAsyncHttpClientConfig config = new Builder().setNettyTimer(nettyTimer).setCookieStore(cookieStore).build(); + final DefaultAsyncHttpClientConfig config = new Builder().setNettyTimer(nettyTimerMock).setCookieStore(cookieStore).build(); final AsyncHttpClient client1 = new DefaultAsyncHttpClient(config); final AsyncHttpClient client2 = new DefaultAsyncHttpClient(config); - try { - assertEquals(cookieStore.count(), 2); - verify(nettyTimer, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class)); - } finally { - closeSilently(client1); - closeSilently(client2); - } + assertEquals(cookieStore.count(), 2); + verify(nettyTimerMock, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class)); + + closeSilently(client1); + closeSilently(client2); } @Test - public void testWithNonNettyTimerShouldScheduleCookieEvictionForEachAHC() throws IOException { + public void testWitDefaultConfigShouldScheduleCookieEvictionForEachAHC() throws IOException { final AsyncHttpClientConfig config1 = new DefaultAsyncHttpClientConfig.Builder().build(); final DefaultAsyncHttpClient client1 = new DefaultAsyncHttpClient(config1); final AsyncHttpClientConfig config2 = new DefaultAsyncHttpClientConfig.Builder().build(); final DefaultAsyncHttpClient client2 = new DefaultAsyncHttpClient(config2); - try { - assertEquals(config1.getCookieStore().count(), 1); - assertEquals(config2.getCookieStore().count(), 1); - } finally { - closeSilently(client1); - closeSilently(client2); - } + assertEquals(config1.getCookieStore().count(), 1); + assertEquals(config2.getCookieStore().count(), 1); + + closeSilently(client1); + closeSilently(client2); + } + + @Test + public void testWithSharedCookieStoreButNonSharedTimerShouldScheduleCookieEvictionForFirstAHC() throws IOException { + final CookieStore cookieStore = new ThreadSafeCookieStore(); + final Timer nettyTimerMock1 = mock(Timer.class); + final AsyncHttpClientConfig config1 = new DefaultAsyncHttpClientConfig.Builder() + .setCookieStore(cookieStore).setNettyTimer(nettyTimerMock1).build(); + final DefaultAsyncHttpClient client1 = new DefaultAsyncHttpClient(config1); + + final Timer nettyTimerMock2 = mock(Timer.class); + final AsyncHttpClientConfig config2 = new DefaultAsyncHttpClientConfig.Builder() + .setCookieStore(cookieStore).setNettyTimer(nettyTimerMock2).build(); + final DefaultAsyncHttpClient client2 = new DefaultAsyncHttpClient(config2); + + assertEquals(config1.getCookieStore().count(), 2); + verify(nettyTimerMock1, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class)); + verify(nettyTimerMock2, never()).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class)); + + closeSilently(client1); + closeSilently(client2); } private static void closeSilently(Closeable closeable) {