changeset 1959:3784a15aef43

Avoid potential IOOB exception after dump-heap command completion Reviewed-by: jerboaa Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2016-May/018753.html Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2016-June/019752.html PR3042
author Andrew Azores <aazores@redhat.com>
date Thu, 12 May 2016 11:17:00 -0400
parents bd656ada63c5
children 7e9e2f81b86c
files vm-heap-analysis/command/src/main/java/com/redhat/thermostat/vm/heap/analysis/command/internal/DumpHeapCommand.java vm-heap-analysis/command/src/main/java/com/redhat/thermostat/vm/heap/analysis/command/locale/LocaleResources.java vm-heap-analysis/command/src/main/resources/com/redhat/thermostat/vm/heap/analysis/command/locale/strings.properties vm-heap-analysis/command/src/test/java/com/redhat/thermostat/vm/heap/analysis/command/internal/DumpHeapCommandTest.java
diffstat 4 files changed, 68 insertions(+), 3 deletions(-) [+]
line wrap: on
line diff
--- a/vm-heap-analysis/command/src/main/java/com/redhat/thermostat/vm/heap/analysis/command/internal/DumpHeapCommand.java	Thu Mar 03 13:02:57 2016 -0500
+++ b/vm-heap-analysis/command/src/main/java/com/redhat/thermostat/vm/heap/analysis/command/internal/DumpHeapCommand.java	Thu May 12 11:17:00 2016 -0400
@@ -102,8 +102,13 @@
             @Override
             public void run() {
                 String latestHeapId = getLatestHeapId(heapDAO, args.getVM());
-                ctx.getConsole().getOutput().println(translator.localize(LocaleResources.COMMAND_HEAP_DUMP_DONE,
-                        latestHeapId).getContents());
+                // latestHeapId may be null if last heap dump is actually not yet available in storage
+                if (latestHeapId != null) {
+                    ctx.getConsole().getOutput().println(translator.localize(LocaleResources.COMMAND_HEAP_DUMP_DONE,
+                            latestHeapId).getContents());
+                } else {
+                    ctx.getConsole().getOutput().println(translator.localize(LocaleResources.COMMAND_HEAP_DUMP_DONE_NOID).getContents());
+                }
                 s.release();
             }
         };
@@ -133,8 +138,16 @@
         }
     }
 
+    // FIXME: storage may actually return us outdated results which do not contain the latest
+    // heap dump(s). This can result in an empty list being returned (which we signal here by
+    // returning null), or can result in a second dump-heap command incorrectly echoing the same
+    // heap dump ID as the immediately prior dump-heap command.
+    // See discussion here: http://icedtea.classpath.org/pipermail/thermostat/2016-May/018753.html
     static String getLatestHeapId(HeapDAO heapDao, VmRef vmRef) {
         Collection<HeapInfo> heapInfos = heapDao.getAllHeapInfo(vmRef);
+        if (heapInfos.isEmpty()) {
+            return null;
+        }
         List<HeapInfo> sortedByLatest = new ArrayList<>(heapInfos);
         Collections.sort(sortedByLatest, new Comparator<HeapInfo>() {
             @Override
--- a/vm-heap-analysis/command/src/main/java/com/redhat/thermostat/vm/heap/analysis/command/locale/LocaleResources.java	Thu Mar 03 13:02:57 2016 -0500
+++ b/vm-heap-analysis/command/src/main/java/com/redhat/thermostat/vm/heap/analysis/command/locale/LocaleResources.java	Thu May 12 11:17:00 2016 -0400
@@ -61,6 +61,7 @@
     HEAP_DUMP_ERROR,
     
     COMMAND_HEAP_DUMP_DONE,
+    COMMAND_HEAP_DUMP_DONE_NOID,
 
     COMMAND_FIND_ROOT_NO_ROOT_FOUND,
 
--- a/vm-heap-analysis/command/src/main/resources/com/redhat/thermostat/vm/heap/analysis/command/locale/strings.properties	Thu Mar 03 13:02:57 2016 -0500
+++ b/vm-heap-analysis/command/src/main/resources/com/redhat/thermostat/vm/heap/analysis/command/locale/strings.properties	Thu May 12 11:17:00 2016 -0400
@@ -20,6 +20,7 @@
 HEAP_DUMP_ERROR = Error dumping heap (agent: {0}, vm: {1})
 
 COMMAND_HEAP_DUMP_DONE = Heap dump ID: {0}
+COMMAND_HEAP_DUMP_DONE_NOID = Heap dump completed
 
 COMMAND_FIND_ROOT_NO_ROOT_FOUND = No root found for: {0}
 
--- a/vm-heap-analysis/command/src/test/java/com/redhat/thermostat/vm/heap/analysis/command/internal/DumpHeapCommandTest.java	Thu Mar 03 13:02:57 2016 -0500
+++ b/vm-heap-analysis/command/src/test/java/com/redhat/thermostat/vm/heap/analysis/command/internal/DumpHeapCommandTest.java	Thu May 12 11:17:00 2016 -0400
@@ -36,6 +36,7 @@
 
 package com.redhat.thermostat.vm.heap.analysis.command.internal;
 
+import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
@@ -105,6 +106,7 @@
 
         HeapInfo heapInfo = new HeapInfo();
         heapInfo.setHeapDumpId("0001");
+        heapInfo.setHeapId("0001-0002");
         when(heapDAO.getAllHeapInfo(any(VmRef.class))).thenReturn(Collections.singleton(heapInfo));
 
         DumpHeapCommand command = new DumpHeapCommand(context, impl);
@@ -119,10 +121,58 @@
 
         verify(impl).execute(eq(vmInfoDao), eq(agentInfoDao), isA(VmRef.class), eq(queue),
                 any(Runnable.class), any(Runnable.class));
-        assertEquals("Done\n", factory.getOutput());
+        assertEquals("Heap dump ID: 0001-0002\n", factory.getOutput());
     }
 
     @Test
+    public void verifyEchosNoIdMessageWhenNoResultFromStorageAfterDumpCompletes() throws CommandException {
+        VmInfoDAO vmInfoDao = mock(VmInfoDAO.class);
+        AgentInfoDAO agentInfoDao = mock(AgentInfoDAO.class);
+        HeapDAO heapDao = mock(HeapDAO.class);
+        RequestQueue queue = mock(RequestQueue.class);
+        HostRef hostRef = new HostRef("myAgent", "dummy");
+        VmInfo vmInfo = new VmInfo("myAgent", "foo", 123, 0, 0, null, null, null, null, null, null, null, null, null,
+                null, null,0, "myUsername");
+        when(vmInfoDao.getVmInfo(new VmRef(hostRef, vmInfo))).thenReturn(vmInfo);
+
+        when(heapDao.getAllHeapInfo(any(VmRef.class))).thenReturn(Collections.<HeapInfo>emptyList());
+
+        DumpHeapHelper impl = mock(DumpHeapHelper.class);
+        final ArgumentCaptor<Runnable> successHandler = ArgumentCaptor
+                .forClass(Runnable.class);
+        doAnswer(new Answer<Void>() {
+
+            @Override
+            public Void answer(InvocationOnMock invocation) throws Throwable {
+                successHandler.getValue().run();
+                return null;
+            }
+        }).when(impl).execute(eq(vmInfoDao), eq(agentInfoDao), any(VmRef.class), eq(queue),
+                successHandler.capture(), any(Runnable.class));
+
+        StubBundleContext context = new StubBundleContext();
+        context.registerService(VmInfoDAO.class, vmInfoDao, null);
+        context.registerService(AgentInfoDAO.class, agentInfoDao, null);
+        context.registerService(RequestQueue.class, queue, null);
+        context.registerService(HeapDAO.class, heapDao, null);
+
+        DumpHeapCommand command = new DumpHeapCommand(context, impl);
+
+        TestCommandContextFactory factory = new TestCommandContextFactory();
+
+        SimpleArguments args = new SimpleArguments();
+        args.addArgument("hostId", "dummy");
+        args.addArgument("vmId", "foo");
+
+        command.run(factory.createContext(args));
+
+        verify(impl).execute(eq(vmInfoDao), eq(agentInfoDao), isA(VmRef.class), eq(queue),
+                any(Runnable.class), any(Runnable.class));
+        assertThat(factory.getOutput(), is(equalTo("Heap dump completed\n")));
+    }
+
+
+    @Test
     public void verifyNeedsHostAndVmId() throws CommandException {
         VmInfoDAO vmInfoDao = mock(VmInfoDAO.class);
         AgentInfoDAO agentInfoDao = mock(AgentInfoDAO.class);