# HG changeset patch
# User Severin Gehwolf <sgehwolf@redhat.com>
# Date 1360847770 -3600
# Node ID af7bd0b8070f0de37dfff73a47b5dc5fa5d7eaf8
# Parent  eda9dc44feda3e8f6210d25442222438f8e53748
Do better cleanup of resources in web servlet/mongo storage.
Reviewed-by: neugens
Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-February/005661.html

diff -r eda9dc44feda -r af7bd0b8070f storage/core/src/main/java/com/redhat/thermostat/storage/core/QueuedStorage.java
--- a/storage/core/src/main/java/com/redhat/thermostat/storage/core/QueuedStorage.java	Tue Feb 12 14:01:51 2013 -0500
+++ b/storage/core/src/main/java/com/redhat/thermostat/storage/core/QueuedStorage.java	Thu Feb 14 14:16:10 2013 +0100
@@ -274,6 +274,7 @@
 
     @Override
     public void shutdown() {
+        delegate.shutdown();
         executor.shutdown();
         fileExecutor.shutdown();
     }
diff -r eda9dc44feda -r af7bd0b8070f storage/core/src/test/java/com/redhat/thermostat/storage/core/QueuedStorageTest.java
--- a/storage/core/src/test/java/com/redhat/thermostat/storage/core/QueuedStorageTest.java	Tue Feb 12 14:01:51 2013 -0500
+++ b/storage/core/src/test/java/com/redhat/thermostat/storage/core/QueuedStorageTest.java	Thu Feb 14 14:16:10 2013 +0100
@@ -41,6 +41,7 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.eq;
@@ -406,5 +407,13 @@
         assertNull(executor.getTask());
         assertNull(fileExecutor.getTask());
     }
+    
+    @Test
+    public void testShutdown() {
+        queuedStorage.shutdown();
+        verify(delegateStorage).shutdown();
+        assertTrue(executor.isShutdown());
+        assertTrue(fileExecutor.isShutdown());
+    }
 }
 
diff -r eda9dc44feda -r af7bd0b8070f storage/mongo/src/main/java/com/redhat/thermostat/storage/mongodb/internal/MongoStorage.java
--- a/storage/mongo/src/main/java/com/redhat/thermostat/storage/mongodb/internal/MongoStorage.java	Tue Feb 12 14:01:51 2013 -0500
+++ b/storage/mongo/src/main/java/com/redhat/thermostat/storage/mongodb/internal/MongoStorage.java	Thu Feb 14 14:16:10 2013 +0100
@@ -321,7 +321,15 @@
 
     @Override
     public void shutdown() {
-        // Nothing to do here.
+        try {
+            // Clean up any pending connections. mongo-java-driver issue 130
+            // suggests that Mongo.close() helps with this ThreadLocal business
+            // tomcat warns about. See also:
+            // IcedTea BZ#1315 and https://jira.mongodb.org/browse/JAVA-130
+            db.getMongo().close();
+        } catch (Exception e) {
+            // ignored
+        }
     }
 
 }
diff -r eda9dc44feda -r af7bd0b8070f storage/mongo/src/test/java/com/redhat/thermostat/storage/mongodb/internal/MongoStorageTest.java
--- a/storage/mongo/src/test/java/com/redhat/thermostat/storage/mongodb/internal/MongoStorageTest.java	Tue Feb 12 14:01:51 2013 -0500
+++ b/storage/mongo/src/test/java/com/redhat/thermostat/storage/mongodb/internal/MongoStorageTest.java	Thu Feb 14 14:16:10 2013 +0100
@@ -52,6 +52,7 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
+import java.lang.reflect.Field;
 import java.util.UUID;
 
 import org.junit.After;
@@ -60,6 +61,7 @@
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
 import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PowerMockIgnore;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
@@ -86,6 +88,12 @@
 import com.redhat.thermostat.storage.core.Update;
 import com.redhat.thermostat.storage.model.BasePojo;
 
+//There is a bug (resolved as wontfix) in powermock which results in
+//java.lang.LinkageError if javax.management.* classes aren't ignored by
+//Powermock. More here: http://code.google.com/p/powermock/issues/detail?id=277
+//SSL tests need this and having that annotation on method level doesn't seem
+//to solve the issue.
+@PowerMockIgnore( {"javax.management.*"})
 @RunWith(PowerMockRunner.class)
 @PrepareForTest({ DBCollection.class, DB.class, Mongo.class, MongoStorage.class, MongoConnection.class })
 public class MongoStorageTest {
@@ -205,7 +213,6 @@
 
     @Test
     public void verifyFindAllReturnsCursor() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         Query query = storage.createQuery(testCategory);
         Cursor<TestClass> cursor = query.execute();
@@ -214,7 +221,6 @@
 
     @Test
     public void verifyFindAllCallsDBCollectionFind() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         Query query = storage.createQuery(testCategory);
         query.where(key1, Criteria.EQUALS, "fluff");
@@ -224,7 +230,6 @@
 
     @Test
     public void verifyFindAllCallsDBCollectionFindWithCorrectQuery() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
 
         MongoQuery query = mock(MongoQuery.class);
@@ -240,7 +245,6 @@
 
     @Test
     public void verifyFindAllReturnsCorrectCursor() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         // TODO find a way to test this that isn't just testing MongoCursor
         // Because we mock the DBCollection, the contents of this query don't actually determine the result.
@@ -252,7 +256,6 @@
 
     @Test
     public void verifyFindAllWithSortAndLimit() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         // TODO find a way to test this that isn't just testing MongoCursor
         // Because we mock the DBCollection, the contents of this query don't actually determine the result.
@@ -272,7 +275,6 @@
 
     @Test
     public void verifyFindAllFromCategoryCallsDBCollectionFindAll() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         Query query = storage.createQuery(testCategory);
         query.execute();
@@ -281,7 +283,6 @@
 
     @Test
     public void verifyFindAllFromCategoryReturnsCorrectCursor() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         Query query = storage.createQuery(testCategory);
         Cursor<TestClass> cursor = query.execute();
@@ -291,7 +292,6 @@
 
     @Test
     public void verifyGetCount() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         long count = storage.getCount(testCategory);
         assertEquals(2, count);
@@ -299,7 +299,6 @@
 
     @Test
     public void verifyGetCountForEmptyCategory() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         long count = storage.getCount(emptyTestCategory);
         assertEquals(0, count);
@@ -307,9 +306,8 @@
 
     @Test
     public void verifyGetCountForNonexistentCategory() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
-        storage.getConnection().connect();
+        setDbFieldInStorage(storage);
         long count = storage.getCount(new Category("NonExistent", TestClass.class));
         assertEquals(0, count);
     }
@@ -346,7 +344,6 @@
 
     @Test
     public void verifyPutChunkUsesCorrectChunkAgent() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         TestClass pojo = new TestClass();
         pojo.setAgentId("123");
@@ -361,7 +358,6 @@
 
     @Test
     public void verifyPutChunkUsesCorrectGlobalAgent() throws Exception {
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
         storage.setAgentId(new UUID(1, 2));
         TestClass pojo = new TestClass();
@@ -382,7 +378,6 @@
         GridFS gridFS = mock(GridFS.class);
         when(gridFS.findOne("test")).thenReturn(file);
         PowerMockito.whenNew(GridFS.class).withArguments(any()).thenReturn(gridFS);
-        PowerMockito.whenNew(Mongo.class).withParameterTypes(MongoURI.class).withArguments(any(MongoURI.class)).thenReturn(m);
         MongoStorage storage = makeStorage();
 
         InputStream actual = storage.loadFile("test");
@@ -476,5 +471,22 @@
         assertEquals("test5", value.get("key5"));
         assertEquals("123", value.get("agentId"));
     }
+    
+    @Test
+    public void verifyMongoCloseOnShutdown() throws Exception {
+        Mongo mockMongo = mock(Mongo.class);
+        when(db.getMongo()).thenReturn(mockMongo);
+        MongoStorage storage = new MongoStorage(conf);
+        setDbFieldInStorage(storage);
+        storage.shutdown();
+        verify(mockMongo).close();
+    }
+
+    private void setDbFieldInStorage(MongoStorage storage) throws Exception {
+        // use a bit of reflection to set the db field
+        Field dbField = storage.getClass().getDeclaredField("db");
+        dbField.setAccessible(true);
+        dbField.set(storage, db);
+    }
 }
 
diff -r eda9dc44feda -r af7bd0b8070f web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Tue Feb 12 14:01:51 2013 -0500
+++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java	Thu Feb 14 14:16:10 2013 +0100
@@ -69,6 +69,7 @@
 import com.redhat.thermostat.common.utils.LoggingUtils;
 import com.redhat.thermostat.storage.core.AbstractQuery.Sort;
 import com.redhat.thermostat.storage.core.Category;
+import com.redhat.thermostat.storage.core.Connection;
 import com.redhat.thermostat.storage.core.Cursor;
 import com.redhat.thermostat.storage.core.Key;
 import com.redhat.thermostat.storage.core.Put;
@@ -135,6 +136,22 @@
         }
         getServletContext().setAttribute(TOKEN_MANAGER_KEY, tokenManager);
     }
+    
+    @Override
+    public void destroy() {
+        logger.log(Level.INFO, "Going to shut down web service");
+        if (storage != null) {
+            // See IcedTea BZ#1315. Shut down storage in order
+            // to avoid further memory leaks.
+            Connection connection = storage.getConnection();
+            try {
+                connection.disconnect();
+            } finally {
+                storage.shutdown();
+            }
+        }
+        logger.log(Level.INFO, "Web service shut down finished");
+    }
 
     @Override
     protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException, ServletException {