From a685784ceaffe34e5277d03041cc402a5aba697c Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com> Date: Fri, 16 Mar 2018 09:59:23 +1100 Subject: [PATCH] CLI: Close subcommands in MultiCommand (#28954) * CLI Command: MultiCommand must close subcommands to release resources properly - Changes are done to override the close method and call close on subcommands using IOUtils#close - Unit Test Closes #28953 --- server/cli/build.gradle | 1 + .../org/elasticsearch/cli/MultiCommand.java | 10 +++ .../elasticsearch/cli/MultiCommandTests.java | 75 ++++++++++++++++++- 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/server/cli/build.gradle b/server/cli/build.gradle index c41c4d975b0..91fbca19eca 100644 --- a/server/cli/build.gradle +++ b/server/cli/build.gradle @@ -36,6 +36,7 @@ archivesBaseName = 'elasticsearch-cli' dependencies { compile 'net.sf.jopt-simple:jopt-simple:5.0.2' + compile "org.elasticsearch:elasticsearch-core:${version}" } test.enabled = false diff --git a/server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java b/server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java index ba6b447792a..054a29e78a6 100644 --- a/server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java +++ b/server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java @@ -19,6 +19,8 @@ package org.elasticsearch.cli; +import java.io.Closeable; +import java.io.IOException; import java.util.Arrays; import java.util.LinkedHashMap; import java.util.Map; @@ -26,6 +28,8 @@ import java.util.Map; import joptsimple.NonOptionArgumentSpec; import joptsimple.OptionSet; +import org.elasticsearch.core.internal.io.IOUtils; + /** * A cli tool which is made up of multiple subcommands. */ @@ -74,4 +78,10 @@ public class MultiCommand extends Command { } subcommand.mainWithoutErrorHandling(Arrays.copyOfRange(args, 1, args.length), terminal); } + + @Override + public void close() throws IOException { + IOUtils.close(subcommands.values()); + } + } diff --git a/server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java b/server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java index f4448bbedfe..41fe851ed25 100644 --- a/server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java +++ b/server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java @@ -22,22 +22,57 @@ package org.elasticsearch.cli; import joptsimple.OptionSet; import org.junit.Before; +import java.io.IOException; +import java.util.concurrent.atomic.AtomicBoolean; + public class MultiCommandTests extends CommandTestCase { static class DummyMultiCommand extends MultiCommand { + + final AtomicBoolean closed = new AtomicBoolean(); + DummyMultiCommand() { - super("A dummy multi command", () -> {}); + super("A dummy multi command", () -> { + }); + } + + @Override + public void close() throws IOException { + super.close(); + if (this.closed.compareAndSet(false, true) == false) { + throw new IllegalStateException("DummyMultiCommand already closed"); + } } } static class DummySubCommand extends Command { + final boolean throwsExceptionOnClose; + final AtomicBoolean closeCalled = new AtomicBoolean(); + DummySubCommand() { - super("A dummy subcommand", () -> {}); + this(false); } + + DummySubCommand(final boolean throwsExceptionOnClose) { + super("A dummy subcommand", () -> { + }); + this.throwsExceptionOnClose = throwsExceptionOnClose; + } + @Override protected void execute(Terminal terminal, OptionSet options) throws Exception { terminal.println("Arguments: " + options.nonOptionArguments().toString()); } + + @Override + public void close() throws IOException { + if (this.closeCalled.compareAndSet(false, true) == false) { + throw new IllegalStateException("DummySubCommand already closed"); + } + if (throwsExceptionOnClose) { + throw new IOException("Error occurred while closing DummySubCommand"); + } + } } DummyMultiCommand multiCommand; @@ -102,4 +137,40 @@ public class MultiCommandTests extends CommandTestCase { assertFalse(output, output.contains("command1")); assertTrue(output, output.contains("Arguments: [foo, bar]")); } + + public void testClose() throws Exception { + DummySubCommand subCommand1 = new DummySubCommand(); + DummySubCommand subCommand2 = new DummySubCommand(); + multiCommand.subcommands.put("command1", subCommand1); + multiCommand.subcommands.put("command2", subCommand2); + multiCommand.close(); + assertTrue("MultiCommand was not closed when close method is invoked", multiCommand.closed.get()); + assertTrue("SubCommand1 was not closed when close method is invoked", subCommand1.closeCalled.get()); + assertTrue("SubCommand2 was not closed when close method is invoked", subCommand2.closeCalled.get()); + } + + public void testCloseWhenSubCommandCloseThrowsException() throws Exception { + final boolean command1Throws = randomBoolean(); + final boolean command2Throws = randomBoolean(); + final DummySubCommand subCommand1 = new DummySubCommand(command1Throws); + final DummySubCommand subCommand2 = new DummySubCommand(command2Throws); + multiCommand.subcommands.put("command1", subCommand1); + multiCommand.subcommands.put("command2", subCommand2); + if (command1Throws || command2Throws) { + // verify exception is thrown, as well as other non failed sub-commands closed + // properly. + IOException ioe = expectThrows(IOException.class, multiCommand::close); + assertEquals("Error occurred while closing DummySubCommand", ioe.getMessage()); + if (command1Throws && command2Throws) { + assertEquals(1, ioe.getSuppressed().length); + assertTrue("Missing suppressed exceptions", ioe.getSuppressed()[0] instanceof IOException); + assertEquals("Error occurred while closing DummySubCommand", ioe.getSuppressed()[0].getMessage()); + } + } else { + multiCommand.close(); + } + assertTrue("SubCommand1 was not closed when close method is invoked", subCommand1.closeCalled.get()); + assertTrue("SubCommand2 was not closed when close method is invoked", subCommand2.closeCalled.get()); + } + }