455184: Memory Leak: MultiClientSessionCookieStore hangs on to

ClientSessions
Convert to WeakHashMap. Add ReadWriteLock for better performance.

Change-Id: I8ad441c8ca117142105e121c08dc052091292e42
Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=455184
Signed-off-by: Patrick Bänziger <patrick.baenziger@bsiag.com>
Reviewed-on: https://git.eclipse.org/r/38338
Tested-by: Hudson CI
Reviewed-by: Judith Gull <jgu@bsiag.com>

2 files changed, 276 insertions, 8 deletions

diff --git a/org.eclipse.scout.rt.client.test/src/org/eclipse/scout/rt/client/MultiClientSessionCookieStoreTest.java b/org.eclipse.scout.rt.client.test/src/org/eclipse/scout/rt/client/MultiClientSessionCookieStoreTest.java
new file mode 100644
index 0000000000..47fa851f0e
--- /dev/null
+++ b/org.eclipse.scout.rt.client.test/src/org/eclipse/scout/rt/client/MultiClientSessionCookieStoreTest.java
@@ -0,0 +1,233 @@
+/*******************************************************************************
+ * Copyright (c) 2014 BSI Business Systems Integration AG.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     BSI Business Systems Integration AG - initial API and implementation
+ ******************************************************************************/
+package org.eclipse.scout.rt.client;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.net.HttpCookie;
+import java.net.URI;
+import java.util.Arrays;
+import java.util.List;
+
+import org.eclipse.scout.commons.CollectionUtility;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+/**
+ * Test class for {@link MultiClientSessionCookieStore}
+ */
+public class MultiClientSessionCookieStoreTest {
+
+  private static final HttpCookie COOKIE1 = new HttpCookie("Testname1", "Testvalue1");
+  private static final HttpCookie COOKIE2 = new HttpCookie("Testname2", "Testvalue2");
+  private static final IClientSession SESSION1 = Mockito.mock(IClientSession.class);
+  private static final IClientSession SESSION2 = Mockito.mock(IClientSession.class);
+  private static URI s_testuri1;
+  private static URI s_testuri2;
+
+  @BeforeClass
+  public static void setup() throws Exception {
+    // These URIs must be top-level - and http. Default cookie store of (Oracle) Java
+    // just ignores path and changes https -> http when retrieving URIs.
+    s_testuri1 = new URI("http://www.eclipse.org");
+    s_testuri2 = new URI("http://www.bsiag.com");
+  }
+
+  @Test
+  public void testAddBasic() throws Exception {
+    final IClientSession session1 = Mockito.mock(IClientSession.class);
+
+    TestMultiClientSessionCookieStore cookieStore = new TestMultiClientSessionCookieStore();
+    // Basic storage test
+    cookieStore.setTestClientSession(session1);
+    cookieStore.add(s_testuri1, COOKIE1);
+    cookieStore.add(s_testuri1, COOKIE2);
+    List<HttpCookie> storedCookies = cookieStore.get(s_testuri1);
+    assertNotNull(storedCookies);
+    assertEquals(2, storedCookies.size());
+    assertCookieEquals(COOKIE1, storedCookies.get(0));
+    assertCookieEquals(COOKIE2, storedCookies.get(1));
+  }
+
+  /**
+   * Tests basic operations without setting a client session.
+   */
+  @Test
+  public void testNullSession() {
+    TestMultiClientSessionCookieStore cookieStore = new TestMultiClientSessionCookieStore();
+    // Dont set a client session. Default handling should happen.
+    cookieStore.add(s_testuri1, COOKIE1);
+    List<HttpCookie> storedCookies = cookieStore.get(s_testuri1);
+    assertNotNull(storedCookies);
+    assertEquals(1, storedCookies.size());
+    assertCookieEquals(COOKIE1, storedCookies.get(0));
+
+    // Now switch to a different client session. Should not have any cookies
+    cookieStore.setTestClientSession(Mockito.mock(IClientSession.class));
+    assertTrue(CollectionUtility.isEmpty(cookieStore.getCookies()));
+
+    // Switch back and clear
+    cookieStore.setTestClientSession(null);
+    assertTrue("Cookie store should have contained cookie", cookieStore.remove(s_testuri1, COOKIE1));
+    assertTrue(CollectionUtility.isEmpty(cookieStore.getCookies()));
+
+  }
+
+  /**
+   * Simple test with two client sessions to ensure that adding works and the client sessions
+   * don't see each others' cookies.
+   */
+  @Test
+  public void testAddMultipleClients() throws Exception {
+    // Simulate concurrency by changing the client session
+    TestMultiClientSessionCookieStore cookieStore = new TestMultiClientSessionCookieStore();
+    cookieStore.setTestClientSession(SESSION1);
+    cookieStore.add(s_testuri1, COOKIE1);
+
+    cookieStore.setTestClientSession(SESSION2);
+    cookieStore.add(s_testuri2, COOKIE2);
+
+    // Change back to first
+    cookieStore.setTestClientSession(SESSION1);
+    List<HttpCookie> storedCookies = cookieStore.get(s_testuri1);
+    assertNotNull(storedCookies);
+    assertEquals(1, storedCookies.size());
+    assertCookieEquals(COOKIE1, CollectionUtility.firstElement(storedCookies));
+    // Change back to second
+    cookieStore.setTestClientSession(SESSION2);
+    List<HttpCookie> storedCookies2 = cookieStore.get(s_testuri2);
+    assertNotNull(storedCookies2);
+    assertEquals(1, storedCookies2.size());
+    assertCookieEquals(COOKIE2, CollectionUtility.firstElement(storedCookies2));
+  }
+
+  @Test
+  public void testGetCookies() throws Exception {
+    TestMultiClientSessionCookieStore cookieStore = new TestMultiClientSessionCookieStore();
+    cookieStore.setTestClientSession(SESSION1);
+    createCookies(cookieStore);
+
+    List<HttpCookie> cookies = cookieStore.getCookies();
+    assertNotNull(cookies);
+    assertEquals(2, cookies.size());
+    assertContainsCookies(Arrays.asList(COOKIE1, COOKIE2), cookies);
+  }
+
+  @Test
+  public void testGetURIs() throws Exception {
+    TestMultiClientSessionCookieStore cookieStore = new TestMultiClientSessionCookieStore();
+    cookieStore.setTestClientSession(SESSION1);
+    createCookies(cookieStore);
+
+    List<URI> uris = cookieStore.getURIs();
+    assertTrue(CollectionUtility.equalsCollection(Arrays.asList(s_testuri1, s_testuri2), uris));
+  }
+
+  @Test
+  public void testRemoveAll() throws Exception {
+    TestMultiClientSessionCookieStore cookieStore = new TestMultiClientSessionCookieStore();
+    cookieStore.setTestClientSession(SESSION1);
+    createCookies(cookieStore);
+    assertFalse(CollectionUtility.isEmpty(cookieStore.getCookies()));
+    assertFalse(CollectionUtility.isEmpty(cookieStore.getURIs()));
+
+    // Remove all
+    cookieStore.removeAll();
+
+    assertTrue(CollectionUtility.isEmpty(cookieStore.getCookies()));
+    assertTrue(CollectionUtility.isEmpty(cookieStore.getURIs()));
+  }
+
+  @Test
+  public void testRemoveByUri() throws Exception {
+    TestMultiClientSessionCookieStore cookieStore = new TestMultiClientSessionCookieStore();
+    cookieStore.setTestClientSession(SESSION1);
+    createCookies(cookieStore);
+    assertEquals(2, CollectionUtility.size(cookieStore.getCookies()));
+
+    // Remove cookie from URI1
+    assertTrue("CookieStore should have contained cookie", cookieStore.remove(s_testuri1, COOKIE1));
+
+    List<HttpCookie> retrievedCookies = cookieStore.getCookies();
+    assertEquals(1, CollectionUtility.size(retrievedCookies));
+    assertCookieEquals(COOKIE2, retrievedCookies.get(0));
+  }
+
+  @Test
+  public void testSessionStopped() throws Exception {
+    TestMultiClientSessionCookieStore cookieStore = new TestMultiClientSessionCookieStore();
+    cookieStore.setTestClientSession(SESSION1);
+    createCookies(cookieStore);
+    assertFalse(CollectionUtility.isEmpty(cookieStore.getCookies()));
+    assertFalse(CollectionUtility.isEmpty(cookieStore.getURIs()));
+
+    cookieStore.sessionStopped(SESSION1);
+
+    assertTrue(CollectionUtility.isEmpty(cookieStore.getCookies()));
+    assertTrue(CollectionUtility.isEmpty(cookieStore.getURIs()));
+  }
+
+  /**
+   * Checks if the expected cookie collection is a subset of the actual cookie collection.
+   *
+   * @param expectedCookies
+   * @param actualCookies
+   */
+  private void assertContainsCookies(List<HttpCookie> expectedCookies, List<HttpCookie> actualCookies) {
+    for (HttpCookie cookie : expectedCookies) {
+      if (!actualCookies.contains(cookie)) {
+        fail("Expected cookie not found! Expected: " + cookie + ". Actual cookies: " + actualCookies);
+      }
+      assertCookieEquals(cookie, actualCookies.get(actualCookies.indexOf(cookie)));
+    }
+  }
+
+  private void createCookies(TestMultiClientSessionCookieStore cookieStore) {
+    cookieStore.add(s_testuri1, COOKIE1);
+    cookieStore.add(s_testuri2, COOKIE2);
+  }
+
+  /**
+   * Checks for equality of the cookie (including its value).
+   *
+   * @param expected
+   * @param actual
+   */
+  private void assertCookieEquals(HttpCookie expected, HttpCookie actual) {
+    assertEquals(expected, actual);
+    if (expected == null) {
+      return;
+    }
+    // Now compare value
+    assertEquals(expected.getValue(), actual.getValue());
+  }
+
+  private static class TestMultiClientSessionCookieStore extends MultiClientSessionCookieStore {
+
+    private IClientSession m_clientSession;
+
+    @Override
+    protected IClientSession getClientSession() {
+      return m_clientSession;
+    }
+
+    public void setTestClientSession(IClientSession session) {
+      m_clientSession = session;
+    }
+
+  }
+
+}
diff --git a/org.eclipse.scout.rt.client/src/org/eclipse/scout/rt/client/MultiClientSessionCookieStore.java b/org.eclipse.scout.rt.client/src/org/eclipse/scout/rt/client/MultiClientSessionCookieStore.java
index 27efde7fe6..40417434a5 100644
--- a/org.eclipse.scout.rt.client/src/org/eclipse/scout/rt/client/MultiClientSessionCookieStore.java
+++ b/org.eclipse.scout.rt.client/src/org/eclipse/scout/rt/client/MultiClientSessionCookieStore.java
@@ -4,9 +4,11 @@ import java.net.CookieManager;
 import java.net.CookieStore;
 import java.net.HttpCookie;
 import java.net.URI;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.WeakHashMap;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
  * HTTP cookie store implementation that distinguishes between different {@link IClientSession} connecting concurrently
@@ -14,12 +16,16 @@ import java.util.Map;
  */
 public class MultiClientSessionCookieStore implements CookieStore {
 
-  private final Object m_cookieStoreLock = new Object();
+  private final ReadWriteLock m_cookieStoresLock = new ReentrantReadWriteLock();
+  /**
+   * Access to this map synchronized with the ReadWriteLock {@link #m_cookieStoresLock}.
+   */
   private final Map<IClientSession, CookieStore> m_cookieStores;
   private final CookieStore m_defaultCookieStore;
 
   public MultiClientSessionCookieStore() {
-    m_cookieStores = new HashMap<IClientSession, CookieStore>();
+    // Use a WeakHashMap to ensure that client sessions are not retained when no longer needed.
+    m_cookieStores = new WeakHashMap<IClientSession, CookieStore>();
     m_defaultCookieStore = new CookieManager().getCookieStore();
   }
 
@@ -54,23 +60,52 @@ public class MultiClientSessionCookieStore implements CookieStore {
   }
 
   private CookieStore getDelegate() {
-    IClientSession currentSession = ClientJob.getCurrentSession();
+    IClientSession currentSession = getClientSession();
     if (currentSession == null) {
       return m_defaultCookieStore;
     }
-    synchronized (m_cookieStoreLock) {
+
+    // Check cache with read lock first.
+    m_cookieStoresLock.readLock().lock();
+    try {
+      CookieStore cookieStore = m_cookieStores.get(currentSession);
+      if (cookieStore != null) {
+        return cookieStore;
+      }
+    }
+    finally {
+      m_cookieStoresLock.readLock().unlock();
+    }
+    // No entry found - get write lock to create it
+    m_cookieStoresLock.writeLock().lock();
+    try {
+      // In the meantime, the cookie store might have been created already by another thread - check again.
       CookieStore cookieStore = m_cookieStores.get(currentSession);
-      if (cookieStore == null) {
+      if (cookieStore != null) {
+        return cookieStore;
+      }
+      else {
         cookieStore = new CookieManager().getCookieStore();
         m_cookieStores.put(currentSession, cookieStore);
+        return cookieStore;
       }
-      return cookieStore;
+    }
+    finally {
+      m_cookieStoresLock.writeLock().unlock();
     }
   }
 
+  protected IClientSession getClientSession() {
+    return ClientJob.getCurrentSession();
+  }
+
   public void sessionStopped(IClientSession clientSession) {
-    synchronized (m_cookieStoreLock) {
+    m_cookieStoresLock.writeLock().lock();
+    try {
       m_cookieStores.remove(clientSession);
     }
+    finally {
+      m_cookieStoresLock.writeLock().unlock();
+    }
   }
 }