From ccb71a52809b4fdb0f8a6453c49fbe3a8b3a745d Mon Sep 17 00:00:00 2001 From: Mario Salazar de Torres Date: Wed, 4 Aug 2021 10:37:19 +0200 Subject: [PATCH 1/2] GEODE-9478: Fix status --dir to use file controller - Previously when you only specified --dir option the PID was read from the member workDir and the status request was attempted to solved by using the attachment API, and after that JMX interface. But given only --dir was specified the controller resolving the request should be FileProcessController instead. - Logic has been changed for both servers and locators to always use FileProcessConroller whenever only --dir flag is specified. - Added an UT to verify new code. - Modified several ITs to verify the new behaviour. - Deleted the following ITs which no longer apply with the new logic: * statusWithEmptyPidFileThrowsIllegalArgumentException * statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails * statusWithStalePidFileReturnsNotResponding --- .../LocatorLauncherLocalIntegrationTest.java | 62 +++---------------- .../LocatorLauncherRemoteIntegrationTest.java | 61 +++--------------- .../ServerLauncherLocalIntegrationTest.java | 62 +++---------------- .../ServerLauncherRemoteIntegrationTest.java | 59 +++--------------- ...ocessControllerFactoryIntegrationTest.java | 14 +++++ .../geode/distributed/LocatorLauncher.java | 35 ++++------- .../geode/distributed/ServerLauncher.java | 30 +++------ .../process/FileProcessController.java | 31 ++++++++++ .../process/ProcessControllerFactory.java | 9 +++ .../process/FileProcessControllerTest.java | 9 +++ 10 files changed, 116 insertions(+), 256 deletions(-) diff --git a/geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java b/geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java index 8f07f1af0128..c676ab432314 100755 --- a/geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherLocalIntegrationTest.java @@ -14,8 +14,6 @@ */ package org.apache.geode.distributed; -import static java.lang.management.ManagementFactory.getRuntimeMXBean; -import static org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING; import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE; import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED; import static org.apache.geode.distributed.ConfigurationProperties.DISABLE_AUTO_RECONNECT; @@ -217,6 +215,13 @@ public void statusWithPidReturnsOnlineWithDetails() throws Exception { public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws Exception { givenRunningLocator(); + // Keep it as we are going to overwrite it + int pid = readPidFile(); + + // Sets a fake PID to ensure that the request is going through FileProcessController, + // because if not, the request would fail due to the PID not existing + givenPidFile(fakePid); + LocatorState locatorState = new Builder() .setWorkingDirectory(getWorkingDirectoryPath()) .build() @@ -230,62 +235,11 @@ public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws Exceptio assertThat(locatorState.getJvmArguments()).isEqualTo(getJvmArguments()); assertThat(locatorState.getLogFile()).isEqualTo(getLogFilePath()); assertThat(locatorState.getMemberName()).isEqualTo(getUniqueName()); - assertThat(locatorState.getPid().intValue()).isEqualTo(readPidFile()); + assertThat(locatorState.getPid().intValue()).isEqualTo(pid); assertThat(locatorState.getUptime()).isGreaterThan(0); assertThat(locatorState.getWorkingDirectory()).isEqualTo(new File(".").getCanonicalPath()); } - @Test - public void statusWithEmptyPidFileThrowsIllegalArgumentException() { - givenEmptyPidFile(); - LocatorLauncher launcher = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build(); - - Throwable thrown = catchThrowable(launcher::status); - - assertThat(thrown) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Invalid pid 'null' found in"); - } - - @Test - public void statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails() throws Exception { - givenEmptyWorkingDirectory(); - - LocatorState locatorState = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build() - .status(); - - assertThat(locatorState.getStatus()).isEqualTo(NOT_RESPONDING); - assertThat(locatorState.getClasspath()).isNull(); - assertThat(locatorState.getGemFireVersion()).isEqualTo(GemFireVersion.getGemFireVersion()); - assertThat(locatorState.getHost()).isEqualTo(getLocalHost().getCanonicalHostName()); - assertThat(locatorState.getJavaVersion()).isEqualTo(System.getProperty("java.version")); - assertThat(locatorState.getJvmArguments()).isEqualTo(getRuntimeMXBean().getInputArguments()); - assertThat(locatorState.getLogFile()).isNull(); - assertThat(locatorState.getMemberName()).isNull(); - assertThat(locatorState.getPid()).isNull(); - assertThat(locatorState.getUptime().intValue()).isEqualTo(0); - assertThat(locatorState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath()); - } - - /** - * This test takes > 1 minute to run in {@link LocatorLauncherLocalFileIntegrationTest}. - */ - @Test - public void statusWithStalePidFileReturnsNotResponding() { - givenPidFile(fakePid); - - LocatorState locatorState = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build() - .status(); - - assertThat(locatorState.getStatus()).isEqualTo(NOT_RESPONDING); - } - @Test public void stopWithPidReturnsStopped() { givenRunningLocator(); diff --git a/geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java b/geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java index cc195055a58b..65c16511fc5b 100755 --- a/geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/distributed/LocatorLauncherRemoteIntegrationTest.java @@ -14,12 +14,10 @@ */ package org.apache.geode.distributed; -import static org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING; import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE; import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED; import static org.apache.geode.internal.inet.LocalHostUtil.getLocalHost; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.catchThrowable; import java.io.File; import java.net.BindException; @@ -149,6 +147,13 @@ public void statusWithPidReturnsOnlineWithDetails() throws Exception { public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws Exception { givenRunningLocator(withPort(0)); + // Keep it as we are going to overwrite it + int pid = readPidFile(); + + // Sets a fake PID to ensure that the request is going through FileProcessController, + // because if not, the request would fail due to the PID not existing + givenPidFile(fakePid); + LocatorState locatorState = new Builder() .setWorkingDirectory(getWorkingDirectoryPath()) .build() @@ -161,62 +166,12 @@ public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws Exceptio assertThat(locatorState.getJvmArguments()).isEqualTo(getJvmArguments()); assertThat(locatorState.getLogFile()).isEqualTo(getLogFile().getCanonicalPath()); assertThat(locatorState.getMemberName()).isEqualTo(getUniqueName()); - assertThat(locatorState.getPid().intValue()).isEqualTo(readPidFile()); + assertThat(locatorState.getPid().intValue()).isEqualTo(pid); assertThat(locatorState.getStatus()).isEqualTo(ONLINE); assertThat(locatorState.getUptime()).isGreaterThan(0); assertThat(locatorState.getWorkingDirectory()).isEqualToIgnoringCase(getWorkingDirectoryPath()); } - @Test - public void statusWithEmptyPidFileThrowsIllegalArgumentException() { - givenEmptyPidFile(); - LocatorLauncher launcher = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build(); - - Throwable thrown = catchThrowable(launcher::status); - - assertThat(thrown) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Invalid pid 'null' found in"); - } - - @Test - public void statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails() throws Exception { - givenEmptyWorkingDirectory(); - - LocatorState locatorState = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build() - .status(); - - assertThat(locatorState.getClasspath()).isNull(); - assertThat(locatorState.getGemFireVersion()).isEqualTo(GemFireVersion.getGemFireVersion()); - assertThat(locatorState.getHost()).isEqualTo(getLocalHost().getCanonicalHostName()); - assertThat(locatorState.getJavaVersion()).isEqualTo(System.getProperty("java.version")); - assertThat(locatorState.getLogFile()).isNull(); - assertThat(locatorState.getMemberName()).isNull(); - assertThat(locatorState.getPid()).isNull(); - assertThat(locatorState.getStatus()).isEqualTo(NOT_RESPONDING); - assertThat(locatorState.getUptime().intValue()).isEqualTo(0); - assertThat(locatorState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath()); - } - - /** - * This test takes > 1 minute to run in {@link LocatorLauncherRemoteFileIntegrationTest}. - */ - @Test - public void statusWithStalePidFileReturnsNotResponding() { - givenPidFile(fakePid); - - LocatorState locatorState = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build() - .status(); - - assertThat(locatorState.getStatus()).isEqualTo(NOT_RESPONDING); - } - @Test public void stopWithPidReturnsStopped() { givenRunningLocator(withPort(0)); diff --git a/geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java b/geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java index dd953e9f3745..0d0c759f8c51 100755 --- a/geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerLauncherLocalIntegrationTest.java @@ -14,7 +14,6 @@ */ package org.apache.geode.distributed; -import static org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING; import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE; import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED; import static org.apache.geode.distributed.ConfigurationProperties.DISABLE_AUTO_RECONNECT; @@ -308,6 +307,13 @@ public void statusWithPidReturnsOnlineWithDetails() throws UnknownHostException public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws UnknownHostException { givenRunningServer(); + // Keep it as we are going to overwrite it + int pid = readPidFile(); + + // Sets a fake PID to ensure that the request is going through FileProcessController, + // because if not, the request would fail due to the PID not existing + givenPidFile(fakePid); + ServerState serverState = new Builder() .setWorkingDirectory(getWorkingDirectoryPath()) .build() @@ -320,64 +326,12 @@ public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws UnknownH assertThat(serverState.getJvmArguments()).isEqualTo(getJvmArguments()); assertThat(serverState.getLogFile()).isEqualTo(getLogFilePath()); assertThat(serverState.getMemberName()).isEqualTo(getUniqueName()); - assertThat(serverState.getPid().intValue()).isEqualTo(readPidFile()); + assertThat(serverState.getPid().intValue()).isEqualTo(pid); assertThat(serverState.getStatus()).isEqualTo(ONLINE); assertThat(serverState.getUptime()).isGreaterThan(0); assertThat(serverState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath()); } - @Test - public void statusWithEmptyPidFileThrowsIllegalArgumentException() { - givenEmptyPidFile(); - ServerLauncher launcher = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build(); - - Throwable thrown = catchThrowable(launcher::status); - - assertThat(thrown) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Invalid pid 'null' found in"); - } - - @Test - public void statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails() - throws UnknownHostException { - givenEmptyWorkingDirectory(); - - ServerState serverState = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build() - .status(); - - assertThat(serverState.getClasspath()).isNull(); - assertThat(serverState.getGemFireVersion()).isEqualTo(GemFireVersion.getGemFireVersion()); - assertThat(serverState.getHost()).isEqualTo(getLocalHost().getCanonicalHostName()); - assertThat(serverState.getJavaVersion()).isEqualTo(System.getProperty("java.version")); - assertThat(serverState.getJvmArguments()).isEqualTo(getJvmArguments()); - assertThat(serverState.getLogFile()).isNull(); - assertThat(serverState.getMemberName()).isNull(); - assertThat(serverState.getPid()).isNull(); - assertThat(serverState.getStatus()).isEqualTo(NOT_RESPONDING); - assertThat(serverState.getUptime().intValue()).isEqualTo(0); - assertThat(serverState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath()); - } - - /** - * This test takes > 1 minute to run in {@link ServerLauncherLocalFileIntegrationTest}. - */ - @Test - public void statusWithStalePidFileReturnsNotResponding() { - givenPidFile(fakePid); - - ServerState serverState = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build() - .status(); - - assertThat(serverState.getStatus()).isEqualTo(NOT_RESPONDING); - } - @Test public void stopWithPidReturnsStopped() { givenRunningServer(); diff --git a/geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java b/geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java index 7456c8de4e4e..c0d42f297148 100755 --- a/geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/distributed/ServerLauncherRemoteIntegrationTest.java @@ -14,14 +14,12 @@ */ package org.apache.geode.distributed; -import static org.apache.geode.distributed.AbstractLauncher.Status.NOT_RESPONDING; import static org.apache.geode.distributed.AbstractLauncher.Status.ONLINE; import static org.apache.geode.distributed.AbstractLauncher.Status.STOPPED; import static org.apache.geode.distributed.ConfigurationProperties.CACHE_XML_FILE; import static org.apache.geode.internal.inet.LocalHostUtil.getLocalHost; import static org.apache.geode.util.internal.GeodeGlossary.GEMFIRE_PREFIX; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.catchThrowable; import java.io.File; import java.io.IOException; @@ -210,13 +208,20 @@ public void statusWithPidReturnsOnlineWithDetails() throws IOException { public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws IOException { givenRunningServer(); + // Keep it as we are going to overwrite it + int pid = readPidFile(); + + // Sets a fake PID to ensure that the request is going through FileProcessController, + // because if not, the request would fail due to the PID not existing + givenPidFile(fakePid); + ServerState serverState = new Builder() .setWorkingDirectory(getWorkingDirectoryPath()) .build() .status(); assertThat(serverState.getStatus()).isEqualTo(ONLINE); - assertThat(serverState.getPid().intValue()).isEqualTo(readPidFile()); + assertThat(serverState.getPid().intValue()).isEqualTo(pid); assertThat(serverState.getUptime()).isGreaterThan(0); assertThat(serverState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath()); assertThat(serverState.getJvmArguments()).isEqualTo(getJvmArguments()); @@ -228,54 +233,6 @@ public void statusWithWorkingDirectoryReturnsOnlineWithDetails() throws IOExcept assertThat(serverState.getMemberName()).isEqualTo(getUniqueName()); } - @Test - public void statusWithEmptyPidFileThrowsIllegalArgumentException() { - givenEmptyPidFile(); - - ServerLauncher launcher = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build(); - - Throwable thrown = catchThrowable(launcher::status); - - assertThat(thrown) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Invalid pid 'null' found in"); - } - - @Test - public void statusWithEmptyWorkingDirectoryReturnsNotRespondingWithDetails() throws IOException { - givenEmptyWorkingDirectory(); - - ServerState serverState = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build() - .status(); - - assertThat(serverState.getStatus()).isEqualTo(NOT_RESPONDING); - assertThat(serverState.getPid()).isNull(); - assertThat(serverState.getUptime().intValue()).isEqualTo(0); - assertThat(serverState.getWorkingDirectory()).isEqualTo(getWorkingDirectoryPath()); - assertThat(serverState.getClasspath()).isNull(); - assertThat(serverState.getGemFireVersion()).isEqualTo(GemFireVersion.getGemFireVersion()); - assertThat(serverState.getJavaVersion()).isEqualTo(System.getProperty("java.version")); - assertThat(serverState.getLogFile()).isNull(); - assertThat(serverState.getHost()).isEqualTo(getLocalHost().getCanonicalHostName()); - assertThat(serverState.getMemberName()).isNull(); - } - - @Test - public void statusWithStalePidFileReturnsNotResponding() { - givenPidFile(fakePid); - - ServerState serverState = new Builder() - .setWorkingDirectory(getWorkingDirectoryPath()) - .build() - .status(); - - assertThat(serverState.getStatus()).isEqualTo(NOT_RESPONDING); - } - @Test public void stopWithPidReturnsStopped() { givenRunningServer(); diff --git a/geode-core/src/integrationTest/java/org/apache/geode/internal/process/ProcessControllerFactoryIntegrationTest.java b/geode-core/src/integrationTest/java/org/apache/geode/internal/process/ProcessControllerFactoryIntegrationTest.java index 9faf65176a0b..ff3a2182c8de 100644 --- a/geode-core/src/integrationTest/java/org/apache/geode/internal/process/ProcessControllerFactoryIntegrationTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/internal/process/ProcessControllerFactoryIntegrationTest.java @@ -19,6 +19,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.io.File; import java.io.FileNotFoundException; @@ -83,6 +84,19 @@ public void createProcessController_returnsMBeanProcessController() throws Excep assertThat(controller).isInstanceOf(MBeanOrFileProcessController.class); } + @Test + public void createProcessController_returnsFileProcessController() throws Exception { + // arrange + when(parameters.getDirectory()).thenReturn(new File("./")); + + // act + ProcessController controller = + factory.createProcessController(parameters); + + // assert + assertThat(controller).isInstanceOf(FileProcessController.class); + } + @Test public void createProcessController_withoutAttachAPI_returnsFileProcessController() throws Exception { diff --git a/geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java b/geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java index 68a22cf88200..1a127d54c8fe 100644 --- a/geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/LocatorLauncher.java @@ -81,6 +81,7 @@ import org.apache.geode.internal.process.FileAlreadyExistsException; import org.apache.geode.internal.process.FileControllableProcess; import org.apache.geode.internal.process.MBeanInvocationFailedException; +import org.apache.geode.internal.process.PidFile; import org.apache.geode.internal.process.PidUnavailableException; import org.apache.geode.internal.process.ProcessController; import org.apache.geode.internal.process.ProcessControllerFactory; @@ -1073,44 +1074,30 @@ private LocatorState statusWithPort() { } private LocatorState statusWithWorkingDirectory() { - int parsedPid = 0; try { - final ProcessController controller = - new ProcessControllerFactory().createProcessController(controllerParameters, - new File(getWorkingDirectory()), ProcessType.LOCATOR.getPidFileName()); - parsedPid = controller.getProcessId(); + new PidFile(new File(getWorkingDirectory()), ProcessType.LOCATOR.getPidFileName()); - // note: in-process request will go infinite loop unless we do the following - if (parsedPid == ProcessUtils.identifyPid()) { - LocatorLauncher runningLauncher = getInstance(); - if (runningLauncher != null) { - return runningLauncher.status(); - } - } + final ProcessController controller = + new ProcessControllerFactory().createProcessController(this.controllerParameters); final String statusJson = controller.status(); return LocatorState.fromJson(statusJson); - } catch (ConnectionFailedException handled) { - // failed to attach to locator JVM - return createNoResponseState(handled, - "Failed to connect to locator with process id " + parsedPid); } catch (FileNotFoundException handled) { // could not find pid file return createNoResponseState(handled, "Failed to find process file " + ProcessType.LOCATOR.getPidFileName() + " in " + getWorkingDirectory()); - } catch (IOException | MBeanInvocationFailedException | UnableToControlProcessException - | TimeoutException handled) { + } catch (IOException | UnableToControlProcessException | TimeoutException handled) { return createNoResponseState(handled, - "Failed to communicate with locator with process id " + parsedPid); - } catch (PidUnavailableException e) { - // couldn't determine pid from within locator JVM - return createNoResponseState(e, "Failed to find usable process id within file " - + ProcessType.LOCATOR.getPidFileName() + " in " + getWorkingDirectory()); + "Failed to communicate with locator"); } catch (InterruptedException handled) { Thread.currentThread().interrupt(); return createNoResponseState(handled, - "Interrupted while trying to communicate with locator with process id " + parsedPid); + "Interrupted while trying to communicate with locator"); + } catch (MBeanInvocationFailedException | ConnectionFailedException e) { + // This would never happen as controller will always be FileProcessController } + + return null; } /** diff --git a/geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java b/geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java index 257ad60953d6..a541c5df4a54 100755 --- a/geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/ServerLauncher.java @@ -88,6 +88,7 @@ import org.apache.geode.internal.process.FileAlreadyExistsException; import org.apache.geode.internal.process.FileControllableProcess; import org.apache.geode.internal.process.MBeanInvocationFailedException; +import org.apache.geode.internal.process.PidFile; import org.apache.geode.internal.process.PidUnavailableException; import org.apache.geode.internal.process.ProcessController; import org.apache.geode.internal.process.ProcessControllerFactory; @@ -1194,30 +1195,19 @@ private ServerState statusWithPid() { private ServerState statusWithWorkingDirectory() { int parsedPid = 0; try { - final ProcessController controller = - new ProcessControllerFactory().createProcessController(controllerParameters, - new File(getWorkingDirectory()), ProcessType.SERVER.getPidFileName()); - parsedPid = controller.getProcessId(); + // This will ensure that if folder is not correct it will fail ASAP + new PidFile(new File(getWorkingDirectory()), ProcessType.SERVER.getPidFileName()); - // note: in-process request will go infinite loop unless we do the following - if (parsedPid == identifyPid()) { - final ServerLauncher runningLauncher = getInstance(); - if (runningLauncher != null) { - return runningLauncher.statusInProcess(); - } - } + final ProcessController controller = + new ProcessControllerFactory().createProcessController(this.controllerParameters); final String statusJson = controller.status(); return ServerState.fromJson(statusJson); - } catch (ConnectionFailedException handled) { - // failed to attach to server JVM - return createNoResponseState(handled, - "Failed to connect to server with process id " + parsedPid); } catch (FileNotFoundException handled) { // could not find pid file return createNoResponseState(handled, "Failed to find process file " + ProcessType.SERVER.getPidFileName() + " in " + getWorkingDirectory()); - } catch (IOException | MBeanInvocationFailedException | UnableToControlProcessException + } catch (IOException | UnableToControlProcessException | TimeoutException handled) { return createNoResponseState(handled, "Failed to communicate with server with process id " + parsedPid); @@ -1225,11 +1215,11 @@ private ServerState statusWithWorkingDirectory() { Thread.currentThread().interrupt(); return createNoResponseState(handled, "Interrupted while trying to communicate with server with process id " + parsedPid); - } catch (PidUnavailableException handled) { - // couldn't determine pid from within server JVM - return createNoResponseState(handled, "Failed to find usable process id within file " - + ProcessType.SERVER.getPidFileName() + " in " + getWorkingDirectory()); + } catch (MBeanInvocationFailedException | ConnectionFailedException e) { + // This would never happen as controller will always be FileProcessController } + + return null; } /** diff --git a/geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java b/geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java index b0d976958300..8497dbd099f0 100755 --- a/geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java +++ b/geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java @@ -77,6 +77,37 @@ class FileProcessController implements ProcessController { statusTimeoutMillis = units.toMillis(timeout); } + /** + * Constructs an instance for controlling a local process. + * + * @param parameters details about the controllable process + * + * @throws IllegalArgumentException if pid is not a positive integer + */ + FileProcessController(final FileControllerParameters parameters) { + this(parameters, DEFAULT_STATUS_TIMEOUT_MILLIS, MILLISECONDS); + } + + /** + * Constructs an instance for controlling a local process. + * + * @param parameters details about the controllable process + * @param timeout the timeout that operations must complete within + * @param units the units of the timeout + * + * @throws IllegalArgumentException if pid is not a positive integer + */ + FileProcessController(final FileControllerParameters parameters, + final long timeout, final TimeUnit units) { + notNull(parameters, "Invalid parameters '" + parameters + "' specified"); + isTrue(timeout >= 0, "Invalid timeout '" + timeout + "' specified"); + notNull(units, "Invalid units '" + units + "' specified"); + + this.pid = 0; + this.parameters = parameters; + this.statusTimeoutMillis = units.toMillis(timeout); + } + @Override public int getProcessId() { return pid; diff --git a/geode-core/src/main/java/org/apache/geode/internal/process/ProcessControllerFactory.java b/geode-core/src/main/java/org/apache/geode/internal/process/ProcessControllerFactory.java index f6647abfda35..211e333e46d5 100755 --- a/geode-core/src/main/java/org/apache/geode/internal/process/ProcessControllerFactory.java +++ b/geode-core/src/main/java/org/apache/geode/internal/process/ProcessControllerFactory.java @@ -59,6 +59,15 @@ public ProcessController createProcessController(final ProcessControllerParamete return new MBeanOrFileProcessController(parameters, pid); } + public ProcessController createProcessController(final ProcessControllerParameters parameters) + throws IOException, InterruptedException, TimeoutException { + notNull(parameters, "Invalid parameters '" + parameters + "' specified"); + notNull(parameters.getDirectory(), + "Invalid workDir '" + parameters.getDirectory() + "' specified"); + + return new FileProcessController(parameters); + } + public ProcessController createProcessController(final ProcessControllerParameters parameters, final File directory, final String pidFileName) throws IOException, InterruptedException, TimeoutException { diff --git a/geode-core/src/test/java/org/apache/geode/internal/process/FileProcessControllerTest.java b/geode-core/src/test/java/org/apache/geode/internal/process/FileProcessControllerTest.java index adaa02db2a99..da7af1960a69 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/process/FileProcessControllerTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/process/FileProcessControllerTest.java @@ -49,6 +49,15 @@ public void before() throws Exception { controller = new FileProcessController(mockParameters, pid, timeout, units); } + @Test + public void noParamsSpecified_throwsNullPointerException() throws Exception { + pid = 0; + + assertThatThrownBy(() -> new FileProcessController(null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("Invalid parameters 'null' specified"); + } + @Test public void pidLessThanOne_throwsIllegalArgumentException() throws Exception { pid = 0; From 1ae64a50ea065175fde2737d48ee5fe2ad806d69 Mon Sep 17 00:00:00 2001 From: Mario Salazar de Torres Date: Tue, 24 Aug 2021 10:26:17 +0200 Subject: [PATCH 2/2] GEODE-9478: Revision 1 - Removed throws in javadoc given new constructors doesn't have any PID --- .../apache/geode/internal/process/FileProcessController.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java b/geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java index 8497dbd099f0..9a6bb9cb2a42 100755 --- a/geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java +++ b/geode-core/src/main/java/org/apache/geode/internal/process/FileProcessController.java @@ -81,8 +81,6 @@ class FileProcessController implements ProcessController { * Constructs an instance for controlling a local process. * * @param parameters details about the controllable process - * - * @throws IllegalArgumentException if pid is not a positive integer */ FileProcessController(final FileControllerParameters parameters) { this(parameters, DEFAULT_STATUS_TIMEOUT_MILLIS, MILLISECONDS); @@ -94,8 +92,6 @@ class FileProcessController implements ProcessController { * @param parameters details about the controllable process * @param timeout the timeout that operations must complete within * @param units the units of the timeout - * - * @throws IllegalArgumentException if pid is not a positive integer */ FileProcessController(final FileControllerParameters parameters, final long timeout, final TimeUnit units) {