From a44aac86616f4e8ffe6977dfef0f0aa460e79d07 Mon Sep 17 00:00:00 2001 From: Stephane Landelle Date: Wed, 8 Apr 2020 09:34:34 +0200 Subject: [PATCH] Fix regression not allowing disabling CookieStore, close #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. --- .../DefaultAsyncHttpClient.java | 27 ++-- .../DefaultAsyncHttpClientTest.java | 117 +++++++++++------- 2 files changed, 85 insertions(+), 59 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java index 0e97fcef79..7cc3e6e341 100644 --- a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java +++ b/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(); diff --git a/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java b/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java index eadd41226a..82a58860a0 100644 --- a/client/src/test/java/org/asynchttpclient/DefaultAsyncHttpClientTest.java +++ b/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)) { + // } } }