Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI: Close subcommands in MultiCommand #28954

Merged
merged 16 commits into from
Mar 15, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions server/cli/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@

package org.elasticsearch.cli;

import java.io.Closeable;
import java.io.IOException;
import java.util.Arrays;
import java.util.LinkedHashMap;
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.
*/
Expand Down Expand Up @@ -74,4 +78,10 @@ protected void execute(Terminal terminal, OptionSet options) throws Exception {
}
subcommand.mainWithoutErrorHandling(Arrays.copyOfRange(args, 1, args.length), terminal);
}

@Override
public void close() throws IOException {
IOUtils.close(subcommands.values());
}

}
74 changes: 72 additions & 2 deletions server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,57 @@
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a personal preference but given that we're adding to this class, I find it hard to read without empty lines between methods/constructor/member fields. Can you please add them?

public void close() throws IOException {
super.close();
if (this.closed.compareAndSet(false, true) == false) {
throw new IOException("DummyMultiCommand already closed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is not really an issue with IO so IOException doesn't make much sense; how about we change this to an IllegalStateException?

}
}
}

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 IOException("DummySubCommand already closed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing here, how about using IllegalStateException

}
if (throwsExceptionOnClose) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have this condition+throw separate from setting closed (maybe rename closed to closeCalled) then you don't need the difficult to read xor logic in the last test.

throw new IOException("Error occurred while closing DummySubCommand");
}
}
}

DummyMultiCommand multiCommand;
Expand Down Expand Up @@ -102,4 +137,39 @@ public void testSubcommandArguments() throws Exception {
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());
}
}