Mercurial > hg > release > thermostat-1.6
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);