Fix regression not allowing disabling CookieStore, close AsyncHttpClient#1714

Motivation:

Setting CookieStore to null in conf should be supported to disable it. Latest commit introduced a NPE regression.

Modification:

* Protect against null CookieStore
* Make sure to decrement CookieStore ref count when AHC instance is closed.

Result:

No more NPE when CookieStore is null.

Author: slandelle

client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java

@@ -23,6 +23,7 @@
 import io.netty.util.concurrent.DefaultThreadFactory;
 import org.asynchttpclient.channel.ChannelPool;
 import org.asynchttpclient.cookie.CookieEvictionTask;
+import org.asynchttpclient.cookie.CookieStore;
 import org.asynchttpclient.filter.FilterContext;
 import org.asynchttpclient.filter.FilterException;
 import org.asynchttpclient.filter.RequestFilter;
@@ -91,21 +92,17 @@ public DefaultAsyncHttpClient(AsyncHttpClientConfig config) {
     channelManager = new ChannelManager(config, nettyTimer);
     requestSender = new NettyRequestSender(config, channelManager, nettyTimer, new AsyncHttpClientState(closed));
     channelManager.configureBootstraps(requestSender);
-    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;
+    CookieStore cookieStore = config.getCookieStore();
+    if (cookieStore != null) {
+      int cookieStoreCount = config.getCookieStore().incrementAndGet();
+      if (
+        allowStopNettyTimer // timer is not shared
+        || cookieStoreCount == 1 // this is the first AHC instance for the shared (user-provided) timer
+      ) {
+        nettyTimer.newTimeout(new CookieEvictionTask(config.expiredCookieEvictionDelay(), cookieStore),
+          config.expiredCookieEvictionDelay(), TimeUnit.MILLISECONDS);
       }
-    } else {
-      // If Timer is not shared.
-      scheduleCookieEviction = true;
-    }
-    if (scheduleCookieEviction) {
-      nettyTimer.newTimeout(new CookieEvictionTask(config.expiredCookieEvictionDelay(), config.getCookieStore()),
-                            config.expiredCookieEvictionDelay(), TimeUnit.MILLISECONDS);
     }
   }
 
@@ -124,6 +121,10 @@ public void close() {
       } catch (Throwable t) {
         LOGGER.warn("Unexpected error on ChannelManager close", t);
       }
+      CookieStore cookieStore = config.getCookieStore();
+      if (cookieStore != null) {
+        cookieStore.decrementAndGet();
+      }
       if (allowStopNettyTimer) {
         try {
           nettyTimer.stop();

client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java

@@ -1,16 +1,15 @@
 package org.asynchttpclient;
 
 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.asynchttpclient.Dsl.*;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.*;
@@ -20,62 +19,88 @@ public class DefaultAsyncHttpClientTest {
 
   @Test
   public void testWithSharedNettyTimerShouldScheduleCookieEvictionOnlyOnce() throws IOException {
-    final Timer nettyTimerMock = mock(Timer.class);
-    final CookieStore cookieStore = new ThreadSafeCookieStore();
-    final DefaultAsyncHttpClientConfig config = new Builder().setNettyTimer(nettyTimerMock).setCookieStore(cookieStore).build();
-    final AsyncHttpClient client1 = new DefaultAsyncHttpClient(config);
-    final AsyncHttpClient client2 = new DefaultAsyncHttpClient(config);
-
-    assertEquals(cookieStore.count(), 2);
-    verify(nettyTimerMock, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class));
-
-    closeSilently(client1);
-    closeSilently(client2);
+    Timer nettyTimerMock = mock(Timer.class);
+    CookieStore cookieStore = new ThreadSafeCookieStore();
+    AsyncHttpClientConfig config = config().setNettyTimer(nettyTimerMock).setCookieStore(cookieStore).build();
+
+    try (AsyncHttpClient client1 = asyncHttpClient(config)) {
+      try (AsyncHttpClient client2 = asyncHttpClient(config)) {
+        assertEquals(cookieStore.count(), 2);
+        verify(nettyTimerMock, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class));
+      }
+    }
   }
 
   @Test
   public void testWitDefaultConfigShouldScheduleCookieEvictionForEachAHC() throws IOException {
-    final AsyncHttpClientConfig config1 = new DefaultAsyncHttpClientConfig.Builder().build();
-    final DefaultAsyncHttpClient client1 = new DefaultAsyncHttpClient(config1);
+    AsyncHttpClientConfig config1 = config().build();
+    try (AsyncHttpClient client1 = asyncHttpClient(config1)) {
+      AsyncHttpClientConfig config2 = config().build();
+      try (AsyncHttpClient client2 = asyncHttpClient(config2)) {
+        assertEquals(config1.getCookieStore().count(), 1);
+        assertEquals(config2.getCookieStore().count(), 1);
+      }
+    }
+  }
 
-    final AsyncHttpClientConfig config2 = new DefaultAsyncHttpClientConfig.Builder().build();
-    final DefaultAsyncHttpClient client2 = new DefaultAsyncHttpClient(config2);
+  @Test
+  public void testWithSharedCookieStoreButNonSharedTimerShouldScheduleCookieEvictionForFirstAHC() throws IOException {
+    CookieStore cookieStore = new ThreadSafeCookieStore();
+    Timer nettyTimerMock1 = mock(Timer.class);
+    AsyncHttpClientConfig config1 = config()
+      .setCookieStore(cookieStore).setNettyTimer(nettyTimerMock1).build();
+
+    try (AsyncHttpClient client1 = asyncHttpClient(config1)) {
+      Timer nettyTimerMock2 = mock(Timer.class);
+      AsyncHttpClientConfig config2 = config()
+        .setCookieStore(cookieStore).setNettyTimer(nettyTimerMock2).build();
+      try (AsyncHttpClient client2 = asyncHttpClient(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));
+      }
+    }
 
-    assertEquals(config1.getCookieStore().count(), 1);
-    assertEquals(config2.getCookieStore().count(), 1);
+    Timer nettyTimerMock3 = mock(Timer.class);
+    AsyncHttpClientConfig config3 = config()
+      .setCookieStore(cookieStore).setNettyTimer(nettyTimerMock3).build();
 
-    closeSilently(client1);
-    closeSilently(client2);
+    try (AsyncHttpClient client2 = asyncHttpClient(config3)) {
+      assertEquals(config1.getCookieStore().count(), 1);
+      verify(nettyTimerMock3, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class));
+    }
   }
 
   @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);
+  public void testWithSharedCookieStoreButNonSharedTimerShouldReScheduleCookieEvictionWhenFirstInstanceGetClosed() throws IOException {
+    CookieStore cookieStore = new ThreadSafeCookieStore();
+    Timer nettyTimerMock1 = mock(Timer.class);
+    AsyncHttpClientConfig config1 = config()
+      .setCookieStore(cookieStore).setNettyTimer(nettyTimerMock1).build();
+
+    try (AsyncHttpClient client1 = asyncHttpClient(config1)) {
+      assertEquals(config1.getCookieStore().count(), 1);
+      verify(nettyTimerMock1, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class));
+    }
+
+    assertEquals(config1.getCookieStore().count(), 0);
+
+    Timer nettyTimerMock2 = mock(Timer.class);
+    AsyncHttpClientConfig config2 = config()
+      .setCookieStore(cookieStore).setNettyTimer(nettyTimerMock2).build();
+
+    try (AsyncHttpClient client2 = asyncHttpClient(config2)) {
+      assertEquals(config1.getCookieStore().count(), 1);
+      verify(nettyTimerMock2, times(1)).newTimeout(any(CookieEvictionTask.class), anyLong(), any(TimeUnit.class));
+    }
   }
 
-  private static void closeSilently(Closeable closeable) {
-    if (closeable != null) {
-      try {
-        closeable.close();
-      } catch (IOException e) {
-        // swallow
-      }
+  @Test
+  public void testDisablingCookieStore() throws IOException {
+    AsyncHttpClientConfig config = config()
+      .setCookieStore(null).build();
+    try (AsyncHttpClient client = asyncHttpClient(config)) {
+      //
     }
   }
 }