Mercurial > hg > release > thermostat-1.2
changeset 1633:7163ecc4e8ed
Register timers WebStorageEndpoint uses so that they can be stopped.
Reviewed-by: vanaltj
Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2014-December/012238.html
line wrap: on
line diff
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/CursorManager.java Thu Dec 11 18:11:05 2014 +0100 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/CursorManager.java Thu Dec 11 13:49:11 2014 +0100 @@ -54,27 +54,23 @@ public static final int CURSOR_NOT_STORED = -1; private final Map<Integer, CursorHolder> cursors; + private final TimerRegistry registry; private int cursorIdCounter; - private final Timer sweeperTimer; - - CursorManager() { - this(new Timer()); - } // test-only - CursorManager(Map<Integer, CursorHolder> cursors, Timer timer) { - this.sweeperTimer = timer; + CursorManager(TimerRegistry registry, Map<Integer, CursorHolder> cursors) { + this.registry = registry; this.cursors = cursors; } - CursorManager(Timer timer) { + CursorManager(TimerRegistry registry) { this.cursors = new HashMap<>(); - this.sweeperTimer = timer; + this.registry = registry; } // test-only CursorManager(int cursorIdCounter) { - this(); + this(null); this.cursorIdCounter = cursorIdCounter; } @@ -139,12 +135,9 @@ } void startSweeperTimer() { - startSweeperTimer(new CursorSweeper(this)); - } - - // This is here in order to facilitate better testing. - void startSweeperTimer(final TimerTask task) { - sweeperTimer.scheduleAtFixedRate(task, 0, CursorHolder.TIMEOUT); + CursorTimer sweeperTimer = new CursorTimer(new CursorSweeper(this)); + sweeperTimer.scheduleTask(); + registry.registerTimer(sweeperTimer); } /** @@ -208,4 +201,33 @@ } + static class CursorTimer implements StoppableTimer { + + private static final String NAME = NAME_PREFIX + "cursor-manager"; + private final Timer timer; + private final TimerTask task; + boolean taskScheduled = false; + + CursorTimer(TimerTask task) { + this(task, new Timer(NAME)); + } + + // for testing + CursorTimer(TimerTask task, Timer timer) { + this.timer = timer; + this.task = task; + } + + void scheduleTask() { + timer.scheduleAtFixedRate(task, 0, CursorHolder.TIMEOUT); + taskScheduled = true; + } + + @Override + public void stop() { + timer.cancel(); + } + + } + }
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/StoppableTimer.java Thu Dec 11 13:49:11 2014 +0100 @@ -0,0 +1,54 @@ +/* + * Copyright 2012-2014 Red Hat, Inc. + * + * This file is part of Thermostat. + * + * Thermostat is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published + * by the Free Software Foundation; either version 2, or (at your + * option) any later version. + * + * Thermostat is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Thermostat; see the file COPYING. If not see + * <http://www.gnu.org/licenses/>. + * + * Linking this code with other modules is making a combined work + * based on this code. Thus, the terms and conditions of the GNU + * General Public License cover the whole combination. + * + * As a special exception, the copyright holders of this code give + * you permission to link this code with independent modules to + * produce an executable, regardless of the license terms of these + * independent modules, and to copy and distribute the resulting + * executable under terms of your choice, provided that you also + * meet, for each linked independent module, the terms and conditions + * of the license of that module. An independent module is a module + * which is not derived from or based on this code. If you modify + * this code, you may extend this exception to your version of the + * library, but you are not obligated to do so. If you do not wish + * to do so, delete this exception statement from your version. + */ + +package com.redhat.thermostat.web.server; + +/** + * Interface for stoppable timers the {@link WebStorageEndPoint} uses. That is + * all timers it uses <strong>must</strong> implement this interface and be + * sure to register the timer via {@link TimerRegistry}. + * + */ +interface StoppableTimer { + + public static final String NAME_PREFIX = "thermostat-timer-"; + + /** + * Stops this timer and the associated thread. + */ + void stop(); + +}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/TimerRegistry.java Thu Dec 11 13:49:11 2014 +0100 @@ -0,0 +1,69 @@ +/* + * Copyright 2012-2014 Red Hat, Inc. + * + * This file is part of Thermostat. + * + * Thermostat is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published + * by the Free Software Foundation; either version 2, or (at your + * option) any later version. + * + * Thermostat is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Thermostat; see the file COPYING. If not see + * <http://www.gnu.org/licenses/>. + * + * Linking this code with other modules is making a combined work + * based on this code. Thus, the terms and conditions of the GNU + * General Public License cover the whole combination. + * + * As a special exception, the copyright holders of this code give + * you permission to link this code with independent modules to + * produce an executable, regardless of the license terms of these + * independent modules, and to copy and distribute the resulting + * executable under terms of your choice, provided that you also + * meet, for each linked independent module, the terms and conditions + * of the license of that module. An independent module is a module + * which is not derived from or based on this code. If you modify + * this code, you may extend this exception to your version of the + * library, but you are not obligated to do so. If you do not wish + * to do so, delete this exception statement from your version. + */ + +package com.redhat.thermostat.web.server; + +import java.util.ArrayList; +import java.util.List; + +/** + * Registers {@link StoppableTimer}s and shuts them down when no longer + * needed. + * + */ +class TimerRegistry { + + private final List<StoppableTimer> registeredTimers; + + TimerRegistry() { + this(new ArrayList<StoppableTimer>()); + } + + // for testing + TimerRegistry(List<StoppableTimer> timers) { + this.registeredTimers = timers; + } + + void registerTimer(StoppableTimer timer) { + registeredTimers.add(timer); + } + + void shutDown() { + for (StoppableTimer timer: registeredTimers) { + timer.stop(); + } + } +}
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/TokenManager.java Thu Dec 11 18:11:05 2014 +0100 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/TokenManager.java Thu Dec 11 13:49:11 2014 +0100 @@ -53,15 +53,24 @@ private static final int TOKEN_LENGTH = 256; - private SecureRandom random = new SecureRandom(); + private final SecureRandom random = new SecureRandom(); - private Map<String,byte[]> tokens = Collections.synchronizedMap(new HashMap<String,byte[]>()); + private final Map<String,byte[]> tokens = Collections.synchronizedMap(new HashMap<String,byte[]>()); - // Maybe use a ScheduledExecutorService if this turns out to not scale well enough. - private Timer timer = new Timer(); + private final TokenManagerTimer timer; private int timeout = 30 * 1000; + TokenManager(TimerRegistry registry) { + this(registry, new TokenManagerTimer()); + } + + // for testing + TokenManager(TimerRegistry registry, TokenManagerTimer timer) { + this.timer = timer; + registry.registerTimer(timer); + } + void setTimeout(int timeout) { this.timeout = timeout; } @@ -142,5 +151,29 @@ return tokens.get(sha256); } + static class TokenManagerTimer implements StoppableTimer { + + private static final String NAME = NAME_PREFIX + "token-manager"; + private final Timer timer; + + // for testing + TokenManagerTimer(Timer timer) { + this.timer = timer; + } + + TokenManagerTimer() { + this(new Timer(NAME)); + } + + @Override + public void stop() { + timer.cancel(); + } + + void schedule(TimerTask task, int timeout) { + timer.schedule(task, timeout); + } + + } }
--- a/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java Thu Dec 11 18:11:05 2014 +0100 +++ b/web/server/src/main/java/com/redhat/thermostat/web/server/WebStorageEndPoint.java Thu Dec 11 13:49:11 2014 +0100 @@ -158,6 +158,18 @@ private Set<String> knownCategoryNames; // the principal callback used for retrieving the JAAS user principal private PrincipalCallback principalCallback; + // a registry for timers which starts/stops registered timers on init() + // destroy() + private TimerRegistry timerRegistry; + + public WebStorageEndPoint() { + // default constructor + } + + // Package private for testing + WebStorageEndPoint(TimerRegistry timerRegistry) { + this.timerRegistry = timerRegistry; + } @Override public void init(ServletConfig config) throws ServletException { @@ -193,7 +205,8 @@ PrincipalCallbackFactory cbFactory = new PrincipalCallbackFactory(info); principalCallback = Objects.requireNonNull(cbFactory.getCallback()); - TokenManager tokenManager = new TokenManager(); + timerRegistry = new TimerRegistry(); + TokenManager tokenManager = new TokenManager(timerRegistry); String timeoutParam = getInitParameter(TOKEN_MANAGER_TIMEOUT_PARAM); if (timeoutParam != null) { tokenManager.setTimeout(Integer.parseInt(timeoutParam)); @@ -210,6 +223,7 @@ @Override public void destroy() { + timerRegistry.shutDown(); logger.log(Level.INFO, "Going to shut down web service"); if (storage != null) { // See IcedTea BZ#1315. Shut down storage in order @@ -653,7 +667,7 @@ // Not yet set for this user, create a new cursor manager // and start the sweeper timer so as to prevent memory // leaks due to cursors kept as a reference in cursor manager - cursorManager = new CursorManager(); + cursorManager = new CursorManager(timerRegistry); cursorManager.startSweeperTimer(); userSession.setAttribute(CURSOR_MANAGER_KEY, cursorManager); }
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/CursorManagerTest.java Thu Dec 11 18:11:05 2014 +0100 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/CursorManagerTest.java Thu Dec 11 13:49:11 2014 +0100 @@ -52,10 +52,12 @@ import java.util.TimerTask; import org.junit.Test; +import org.mockito.ArgumentCaptor; import com.redhat.thermostat.storage.core.experimental.BatchCursor; import com.redhat.thermostat.web.server.CursorManager.CursorHolder; import com.redhat.thermostat.web.server.CursorManager.CursorSweeper; +import com.redhat.thermostat.web.server.CursorManager.CursorTimer; public class CursorManagerTest { @@ -65,7 +67,7 @@ */ @Test public void testPutBasicCursorHasMore() { - CursorManager manager = new CursorManager(); + CursorManager manager = new CursorManager(mock(TimerRegistry.class)); int id = manager.put(getHasMoreBatchCursor()); assertTrue(id >= 0); assertEquals(0, id); @@ -103,7 +105,7 @@ */ @Test public void testPutNoHasMoreCursor() { - CursorManager manager = new CursorManager(); + CursorManager manager = new CursorManager(mock(TimerRegistry.class)); int id = manager.put(mock(BatchCursor.class)); assertEquals(CursorManager.CURSOR_NOT_STORED, id); } @@ -116,7 +118,7 @@ @Test public void testGetInvalidId() { - CursorManager manager = new CursorManager(); + CursorManager manager = new CursorManager(mock(TimerRegistry.class)); BatchCursor<?> c = manager.get(CursorManager.CURSOR_NOT_STORED); assertNull(c); } @@ -128,7 +130,7 @@ */ @Test public void testGetHasMore() { - CursorManager manager = new CursorManager(); + CursorManager manager = new CursorManager(mock(TimerRegistry.class)); int num = (int)(Math.random() * 300); addCursors(num, manager); BatchCursor<?> cursor = getHasMoreBatchCursor(); @@ -148,7 +150,7 @@ cursors.put(4, new CursorHolder(mock(BatchCursor.class), expiredTime)); cursors.put(5, new CursorHolder(mock(BatchCursor.class), notExpiredTime)); cursors.put(7, new CursorHolder(mock(BatchCursor.class), expiredTime)); - CursorManager manager = new CursorManager(cursors, mock(Timer.class)); + CursorManager manager = new CursorManager(mock(TimerRegistry.class), cursors); manager.expireCursors(); // should remove old cursors assertEquals(1, cursors.keySet().size()); assertNotNull(cursors.get(5)); @@ -160,7 +162,7 @@ cursors.put(3, new CursorHolder(mock(BatchCursor.class), 3)); cursors.put(4, new CursorHolder(mock(BatchCursor.class), 4)); cursors.put(5, new CursorHolder(mock(BatchCursor.class), 5)); - CursorManager manager = new CursorManager(cursors, mock(Timer.class)); + CursorManager manager = new CursorManager(mock(TimerRegistry.class), cursors); manager.removeCursor(3); assertEquals(2, cursors.keySet().size()); assertNotNull(cursors.get(4)); @@ -176,7 +178,7 @@ cursors.put(4, new CursorHolder(mock(BatchCursor.class), expiredTime)); cursors.put(5, new CursorHolder(mock(BatchCursor.class), notExpiredTime)); cursors.put(7, new CursorHolder(mock(BatchCursor.class), expiredTime)); - CursorManager manager = new CursorManager(cursors, mock(Timer.class)); + CursorManager manager = new CursorManager(mock(TimerRegistry.class), cursors); // refresh 4's timestamp so that it's now no longer expired manager.updateCursorTimeStamp(4); manager.expireCursors(); @@ -187,12 +189,14 @@ @Test public void canStartSweeperTimerViaManager() { - Timer mockTimer = mock(Timer.class); - CursorManager manager = new CursorManager(mockTimer); - TimerTask timerTask = mock(TimerTask.class); - manager.startSweeperTimer(timerTask); - long threeMinutes = 3 * 60 * 1000; - verify(mockTimer).scheduleAtFixedRate(timerTask, 0, threeMinutes); + TimerRegistry registry = mock(TimerRegistry.class); + CursorManager manager = new CursorManager(registry); + manager.startSweeperTimer(); + ArgumentCaptor<CursorTimer> timerCaptor = ArgumentCaptor.forClass(CursorTimer.class); + verify(registry).registerTimer(timerCaptor.capture()); + CursorTimer timer = timerCaptor.getValue(); + assertNotNull("expected non-null timer => timer thread started", timer); + assertTrue("startSweeperTimer() expected to schedule task", timer.taskScheduled); } private void addCursors(int num, CursorManager manager) { @@ -201,6 +205,27 @@ } } + // CursorTimer tests + + @Test + public void testCursorTimerScheduleTask() { + TimerTask timerTask = mock(TimerTask.class); + Timer timer = mock(Timer.class); + CursorTimer cursorTimer = new CursorTimer(timerTask, timer); + long threeMinutes = 3 * 60 * 1000; + cursorTimer.scheduleTask(); + verify(timer).scheduleAtFixedRate(timerTask, 0, threeMinutes); + } + + @Test + public void testCursorTimerStop() { + TimerTask timerTask = mock(TimerTask.class); + Timer timer = mock(Timer.class); + CursorTimer cursorTimer = new CursorTimer(timerTask, timer); + cursorTimer.stop(); + verify(timer).cancel(); + } + // CursorHolder tests @Test
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/TimerRegistryTest.java Thu Dec 11 13:49:11 2014 +0100 @@ -0,0 +1,73 @@ +/* + * Copyright 2012-2014 Red Hat, Inc. + * + * This file is part of Thermostat. + * + * Thermostat is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published + * by the Free Software Foundation; either version 2, or (at your + * option) any later version. + * + * Thermostat is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Thermostat; see the file COPYING. If not see + * <http://www.gnu.org/licenses/>. + * + * Linking this code with other modules is making a combined work + * based on this code. Thus, the terms and conditions of the GNU + * General Public License cover the whole combination. + * + * As a special exception, the copyright holders of this code give + * you permission to link this code with independent modules to + * produce an executable, regardless of the license terms of these + * independent modules, and to copy and distribute the resulting + * executable under terms of your choice, provided that you also + * meet, for each linked independent module, the terms and conditions + * of the license of that module. An independent module is a module + * which is not derived from or based on this code. If you modify + * this code, you may extend this exception to your version of the + * library, but you are not obligated to do so. If you do not wish + * to do so, delete this exception statement from your version. + */ + +package com.redhat.thermostat.web.server; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; + +import java.util.List; + +import org.junit.Test; + +public class TimerRegistryTest { + + @Test + public void canRegisterTimers() { + @SuppressWarnings("unchecked") + List<StoppableTimer> timers = mock(List.class); + TimerRegistry reg = new TimerRegistry(timers); + StoppableTimer mockTimer = mock(StoppableTimer.class); + reg.registerTimer(mockTimer); + verify(timers).add(mockTimer); + verifyNoMoreInteractions(timers); + } + + @Test + public void shutdownStopsTimers() { + TimerRegistry registry = new TimerRegistry(); + StoppableTimer mockTimer = mock(StoppableTimer.class); + StoppableTimer timer2 = mock(StoppableTimer.class); + registry.registerTimer(timer2); + registry.registerTimer(mockTimer); + registry.shutDown(); + verify(timer2).stop(); + verify(mockTimer).stop(); + verifyNoMoreInteractions(mockTimer); + verifyNoMoreInteractions(timer2); + } +}
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/TokenManagerTest.java Thu Dec 11 18:11:05 2014 +0100 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/TokenManagerTest.java Thu Dec 11 13:49:11 2014 +0100 @@ -40,12 +40,19 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Matchers.isA; import java.io.UnsupportedEncodingException; import java.util.Arrays; +import java.util.Timer; +import java.util.TimerTask; import org.junit.Test; +import com.redhat.thermostat.web.server.TokenManager.TokenManagerTimer; + public class TokenManagerTest { /** @@ -62,7 +69,7 @@ */ @Test public void generateTokenTest() { - TokenManager tokenManager = new TokenManager(); + TokenManager tokenManager = new TokenManager(mock(TimerRegistry.class)); String clientToken = "something"; String action = "myAction"; byte[] token = tokenManager.generateToken(clientToken.getBytes(), action); @@ -77,7 +84,7 @@ byte[] expected = new byte[] { (byte)0xff, 0x6f, 0x6d, 0x65, 0x53, 0x74, 0x72, 0x69, 0x6e, 0x67, 0x57, 0x65, 0x48, 0x61, 0x73, 0x68 }; - TokenManager tokenManager = new TokenManager(); + TokenManager tokenManager = new TokenManager(mock(TimerRegistry.class)); String actual = tokenManager.convertBytesToHexString(expected); assertEquals(expected.length * 2, actual.length()); assertEquals("ff6f6d65537472696e67576548617368", actual); @@ -85,7 +92,7 @@ @Test public void generateAndVerifyTokenTest() { - TokenManager tokenManager = new TokenManager(); + TokenManager tokenManager = new TokenManager(mock(TimerRegistry.class)); String clientToken = "something"; String action = "myAction"; byte[] token = tokenManager.generateToken(clientToken.getBytes(), action); @@ -94,5 +101,36 @@ String wrongAction = "someAction"; assertFalse(tokenManager.verifyToken(clientToken.getBytes(), token, wrongAction)); } + + @Test + public void instantiationRegistersTimer() { + TimerRegistry registry = mock(TimerRegistry.class); + new TokenManager(registry); + verify(registry).registerTimer(isA(TokenManagerTimer.class)); + registry = mock(TimerRegistry.class); + TokenManagerTimer timer = mock(TokenManagerTimer.class); + new TokenManager(registry, timer); + verify(registry).registerTimer(timer); + } + + // TokenManagerTimer tests + + @Test + public void tokenManagerTimerCanScheduleTasks() { + Timer mockTimer = mock(Timer.class); + TokenManagerTimer timer = new TokenManagerTimer(mockTimer); + TimerTask task = mock(TimerTask.class); + int timeout = -100; + timer.schedule(task, timeout); + verify(mockTimer).schedule(task, -100); + } + + @Test + public void tokenManagerTimerCanStopTimer() { + Timer mockTimer = mock(Timer.class); + TokenManagerTimer timer = new TokenManagerTimer(mockTimer); + timer.stop(); + verify(mockTimer).cancel(); + } }
--- a/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java Thu Dec 11 18:11:05 2014 +0100 +++ b/web/server/src/test/java/com/redhat/thermostat/web/server/WebStorageEndPointUnitTest.java Thu Dec 11 13:49:11 2014 +0100 @@ -242,6 +242,14 @@ assertNotNull(serverTokenCaptor.getValue()); } + @Test + public void testShutDownCancelsTimers() { + TimerRegistry registry = mock(TimerRegistry.class); + WebStorageEndPoint endpoint = new WebStorageEndPoint(registry); + endpoint.destroy(); + verify(registry).shutDown(); + } + private ThCreatorResult creatWorkingThermostatHome() throws IOException { Path testThermostatHome = Files.createTempDirectory( "foo-thermostat-home-", new FileAttribute[] {});