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

8272600: (test) Use native "sleep" in Basic.java #5239

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -27,13 +27,13 @@
* 5026830 5023243 5070673 4052517 4811767 6192449 6397034 6413313
* 6464154 6523983 6206031 4960438 6631352 6631966 6850957 6850958
* 4947220 7018606 7034570 4244896 5049299 8003488 8054494 8058464
* 8067796 8224905 8263729 8265173
* 8067796 8224905 8263729 8265173 8272600 8231297
Copy link
Member

@iklam iklam Aug 24, 2021

Choose a reason for hiding this comment

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

The test should also be modified to use @run main/othervm/native/timeout=300 so that this test will be flagged by jtreg if -nativepath: is not specified.

Copy link
Contributor Author

@RogerRiggs RogerRiggs Aug 24, 2021

Choose a reason for hiding this comment

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

It should be possible to run this test as a main, without the overhead of building the native image.
The use of a Java child greatly reduces the complexity of the test and improves its maintainability.
Requiring a native special built program raises the overhead considerably.
And all because the VM can't or won't allow its output to be managed.

Copy link
Member

@dholmes-ora dholmes-ora Aug 31, 2021

Choose a reason for hiding this comment

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

In the same way the test uses:

private static final String[] winEnvCommand = {"cmd.exe", "/c", "set"};

you could also have:

private static final String[] winSleepCommand = {"cmd.exe", "/c", "timeout", "/T", "60", "/NOBREAK"};

* @key intermittent
* @summary Basic tests for Process and Environment Variable code
* @modules java.base/java.lang:open
* @library /test/lib
* @run main/othervm/timeout=300 -Djava.security.manager=allow Basic
* @run main/othervm/timeout=300 -Djava.security.manager=allow -Djdk.lang.Process.launchMechanism=fork Basic
* @run main/othervm/native/timeout=300 -Djava.security.manager=allow Basic
* @run main/othervm/native/timeout=300 -Djava.security.manager=allow -Djdk.lang.Process.launchMechanism=fork Basic
* @author Martin Buchholz
*/

@@ -50,8 +50,8 @@
import static java.lang.ProcessBuilder.Redirect.*;

import java.io.*;
import java.lang.reflect.Field;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardCopyOption;
import java.util.*;
@@ -85,7 +85,7 @@ public class Basic {
/**
* Returns the number of milliseconds since time given by
* startNanoTime, which must have been previously returned from a
* call to {@link System.nanoTime()}.
* call to {@link System#nanoTime()}.
*/
private static long millisElapsedSince(long startNanoTime) {
return TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanoTime);
@@ -2137,34 +2137,8 @@ public void doIt(Map<String,String> environ) {
final int cases = 4;
for (int i = 0; i < cases; i++) {
final int action = i;
List<String> childArgs = new ArrayList<>(javaChildArgs);
List<String> childArgs = getSleepArgs();
final ProcessBuilder pb = new ProcessBuilder(childArgs);
{
// Redirect any child VM error output away from the stream being tested
// and to the log file. For background see:
// 8231297: java/lang/ProcessBuilder/Basic.java test fails intermittently
// Destroying the process may, depending on the timing, cause some output
// from the child VM.
// This test requires the thread reading from the subprocess be blocked
// in the read from the subprocess; there should be no bytes to read.
// Modify the argument list shared with ProcessBuilder to redirect VM output.
assert (childArgs.get(1).equals("-XX:+DisplayVMOutputToStderr")) : "Expected arg 1 to be \"-XX:+DisplayVMOutputToStderr\"";
switch (action & 0x1) {
case 0:
childArgs.set(1, "-XX:+DisplayVMOutputToStderr");
childArgs.add(2, "-Xlog:all=warning:stderr");
pb.redirectError(INHERIT);
break;
case 1:
childArgs.set(1, "-XX:+DisplayVMOutputToStdout");
childArgs.add(2, "-Xlog:all=warning:stdout");
pb.redirectOutput(INHERIT);
break;
default:
throw new Error();
}
}
childArgs.add("sleep");
final byte[] bytes = new byte[10];
final Process p = pb.start();
final CountDownLatch latch = new CountDownLatch(1);
@@ -2441,8 +2415,7 @@ public void run() {
// Process.waitFor(0, TimeUnit.MILLISECONDS) work as expected.
//----------------------------------------------------------------
try {
List<String> childArgs = new ArrayList<String>(javaChildArgs);
childArgs.add("sleep");
List<String> childArgs = getSleepArgs();
final Process p = new ProcessBuilder(childArgs).start();
long start = System.nanoTime();
if (!p.isAlive() || p.waitFor(0, TimeUnit.MILLISECONDS)) {
@@ -2471,12 +2444,13 @@ public void run() {
// works as expected.
//----------------------------------------------------------------
try {
List<String> childArgs = new ArrayList<String>(javaChildArgs);
childArgs.add("sleep");
List<String> childArgs = getSleepArgs();
final Process p = new ProcessBuilder(childArgs).start();
long start = System.nanoTime();

p.waitFor(10, TimeUnit.MILLISECONDS);
if (p.waitFor(10, TimeUnit.MILLISECONDS)) {
System.out.println("WaitFor didn't wait long enough: " + (System.nanoTime() - start));
Copy link
Member

@dholmes-ora dholmes-ora Sep 15, 2021

Choose a reason for hiding this comment

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

Either the condition or the message seems wrong here. If waitFor returns true then the process has exited and we obviously did wait long enough.

Copy link
Contributor Author

@RogerRiggs RogerRiggs Sep 15, 2021

Choose a reason for hiding this comment

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

This code is diagnostic.
After switching to native sleep, I had intermittent failures claiming it did not sleep long enough.
I was unable to find a specific cause for those failures.
Many of the tests fail to check if the sleep processes terminate prematurely and if the executable is not found, it never launched.

Copy link
Member

@dholmes-ora dholmes-ora Sep 15, 2021

Choose a reason for hiding this comment

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

Okay but my comment still stands.

};

long end = System.nanoTime();
if ((end - start) < TimeUnit.MILLISECONDS.toNanos(10))
@@ -2490,8 +2464,7 @@ public void run() {
// interrupt works as expected, if interrupted while waiting.
//----------------------------------------------------------------
try {
List<String> childArgs = new ArrayList<String>(javaChildArgs);
childArgs.add("sleep");
List<String> childArgs = getSleepArgs();
final Process p = new ProcessBuilder(childArgs).start();
final long start = System.nanoTime();
final CountDownLatch aboutToWaitFor = new CountDownLatch(1);
@@ -2522,8 +2495,7 @@ public void run() {
// interrupt works as expected, if interrupted while waiting.
//----------------------------------------------------------------
try {
List<String> childArgs = new ArrayList<String>(javaChildArgs);
childArgs.add("sleep");
List<String> childArgs = getSleepArgs();
final Process p = new ProcessBuilder(childArgs).start();
final long start = System.nanoTime();
final CountDownLatch aboutToWaitFor = new CountDownLatch(1);
@@ -2554,8 +2526,7 @@ public void run() {
// interrupt works as expected, if interrupted before waiting.
//----------------------------------------------------------------
try {
List<String> childArgs = new ArrayList<String>(javaChildArgs);
childArgs.add("sleep");
List<String> childArgs = getSleepArgs();
final Process p = new ProcessBuilder(childArgs).start();
final long start = System.nanoTime();
final CountDownLatch threadStarted = new CountDownLatch(1);
@@ -2586,8 +2557,7 @@ public void run() {
// Check that Process.waitFor(timeout, null) throws NPE.
//----------------------------------------------------------------
try {
List<String> childArgs = new ArrayList<String>(javaChildArgs);
childArgs.add("sleep");
List<String> childArgs = getSleepArgs();
final Process p = new ProcessBuilder(childArgs).start();
THROWS(NullPointerException.class,
() -> p.waitFor(10L, null));
@@ -2610,8 +2580,7 @@ public void run() {
// Check that default implementation of Process.waitFor(timeout, null) throws NPE.
//----------------------------------------------------------------
try {
List<String> childArgs = new ArrayList<String>(javaChildArgs);
childArgs.add("sleep");
List<String> childArgs = getSleepArgs();
final Process proc = new ProcessBuilder(childArgs).start();
final DelegatingProcess p = new DelegatingProcess(proc);

@@ -2637,24 +2606,78 @@ public void run() {
// Process.waitFor(long, TimeUnit)
//----------------------------------------------------------------
try {
List<String> childArgs = new ArrayList<String>(javaChildArgs);
childArgs.add("sleep");
List<String> childArgs = getSleepArgs();
final Process proc = new ProcessBuilder(childArgs).start();
DelegatingProcess p = new DelegatingProcess(proc);
long start = System.nanoTime();

p.waitFor(1000, TimeUnit.MILLISECONDS);
if (p.waitFor(1000, TimeUnit.MILLISECONDS)) {
// Unexpected early exit
long early = System.nanoTime();
System.out.println("Process exited too soon (" + (early - start) + "ns)" +
", exitValue: " + p.exitValue());
fail("Process exited too soon, exitValue: " + p.exitValue());
}

long end = System.nanoTime();
if ((end - start) < 500000000)
fail("Test failed: waitFor didn't take long enough");
fail("Test failed: waitFor didn't take long enough (" + (end - start) + "ns)");

p.destroy();

p.waitFor(1000, TimeUnit.MILLISECONDS);
} catch (Throwable t) { unexpected(t); }
}

// Path to native executables, if any
private static final String TEST_NATIVEPATH = System.getProperty("test.nativepath");

// Path where "sleep" program may be found" or null
private static final Path SLEEP_PATH = initSleepPath();

/**
* Compute the Path to a sleep executable.
* @return a Path to sleep or BasicSleep(.exe) or null if none
*/
private static Path initSleepPath() {
if (Windows.is() && TEST_NATIVEPATH != null) {
// exeBasicSleep is equivalent to sleep on Unix
Path exePath = Path.of(TEST_NATIVEPATH).resolve("BasicSleep.exe");
if (Files.isExecutable(exePath)) {
return exePath;
}
}

List<String> binPaths = List.of("/bin", "/usr/bin");
for (String dir : binPaths) {
Path exePath = Path.of(dir).resolve("sleep");
if (Files.isExecutable(exePath)) {
return exePath;
}
}
return null;
}

/**
* Return the list of process arguments for a child to sleep 10 minutes (600 seconds).
*
* @return A list of process arguments to sleep 10 minutes.
*/
private static List<String> getSleepArgs() {
List<String> childArgs = null;
if (SLEEP_PATH != null) {
childArgs = List.of(SLEEP_PATH.toString(), "600");
} else {
// Fallback to the JavaChild ; its 'sleep' command is for 10 minutes.
// The fallback the Java$Child is used if the test is run without building
// the BasicSleep native executable (for Windows).
childArgs = new ArrayList<>(javaChildArgs);
childArgs.add("sleep");
System.out.println("Sleep not found, fallback to JavaChild: " + childArgs);
}
return childArgs;
}

static void closeStreams(Process p) {
try {
p.getOutputStream().close();
@@ -0,0 +1,54 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
#include <stdio.h>
#include <stdlib.h>

#ifdef _WIN32
#include <windows.h>
#else
#include <unistd.h>
#endif
/**
* Command line program to sleep at least given number of seconds.
* The behavior should equivalent to the Unix sleep command.
* Actual time sleeping may vary if interrupted, the remaining time
* returned from sleep has limited accuracy.
*
* Note: the file name prefix "exe" identifies the source should be built into BasicSleep(.exe).
*/
int main(int argc, char** argv) {
int seconds;

if (argc < 2 || (seconds = atoi(argv[1])) < 0) {
fprintf(stderr, "usage: BasicSleep <non-negative seconds>\n");
exit(1);
}

#ifdef _WIN32
Sleep(seconds * 1000);
#else
while ((seconds = sleep(seconds)) > 0) {
// until no more to sleep
}
#endif
}