adding config to get the delay in ms and also making a CookieEvictionTask to evict expired cookies.

Author: sivaalli

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
    *

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);
-        }
-      }
     }
   }
 

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,

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);
+  }
 }

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);
+    }
+}

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.
    *
    * <p>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();
 }

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<String, Map<CookieKey, StoredCookie>> 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<Cookie> 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<String, Map<CookieKey, StoredCookie>> getUnderlying() {
+    return new HashMap<>(cookieJar);
+  }
+
   private String requestDomain(Uri requestUri) {
     return requestUri.getHost().toLowerCase();
   }
@@ -206,27 +195,13 @@ private List<Cookie> 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<CookieKey> {
     final String name;
     final String path;

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

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<String> 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;
+  }
 }

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());