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
Changes from 1 commit
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,7 +27,7 @@
* 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
@@ -52,6 +52,7 @@
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.*;
@@ -82,10 +83,13 @@ public class Basic {
static final String PERMISSION_DENIED_ERROR_MSG = "(Permission denied|error=13)";
static final String NO_SUCH_FILE_ERROR_MSG = "(No such file|error=2)";

/* Path to native executables; sleepmillis */
static final Path TEST_NATIVEPATH = Path.of(System.getProperty("test.nativepath", "."));

/**
* 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 +2141,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 +2419,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,8 +2448,7 @@ 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();

@@ -2490,8 +2466,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 +2497,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 +2528,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 +2559,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 +2582,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,8 +2608,7 @@ 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();
@@ -2647,14 +2617,31 @@ public void run() {

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); }
}

/**
* Return the list of process arguments for a child to sleep 10 minutes (600000 milliseconds).
* @return A list of process arguments to sleep 10 minutes.
*/
private static List<String> getSleepArgs() {
List<String> childArgs = null;
Path sleepExe = TEST_NATIVEPATH.resolve("sleepmillis");
if (sleepExe.toFile().canExecute()) {
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.

Why is the fallback necessary? Other test cases such as test/jdk/tools/launcher/JliLaunchTest.java do not have such a fallback.

Also, I noticed that JliLaunchTest does something like this:

        Path launcher = Paths.get(System.getProperty("test.nativepath"),
            "JliLaunchTest" + (Platform.isWindows() ? ".exe" : ""));

but test/jdk/java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java doesn't add ".exe", so it may not be necessary.

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.

See above, the java Child is more portable and lower maintenance.
Windows looks for xxx.exe if xxx is not found.

Copy link
Member

@iklam iklam Aug 25, 2021

Choose a reason for hiding this comment

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

How about adding a comment to explain why the fallback is necessary?

Also, if jtreg -nativepath:xxx is specified, but there's no xxx/sleepmillis, or xxx/sleepmillis is not executable, that should be a setup error (or a bug in the test itself). E.g., if we are testing inside mach5, we should always execute the native program, and should not silently fallback to the java Child program. Otherwise, setup problems in mach5 might bring us back to the mysterious intermittent failure.

(The current version of the code is buggy on Windows and will always silently fall back to Child because the executable is named "sleepmillis.exe", not "sleepmillis").

childArgs = List.of(sleepExe.toString(), "600000");
} else {
// Fallback to the JavaChild sleep does a sleep for 10 minutes.
childArgs = new ArrayList<>(javaChildArgs);
childArgs.add("sleep");
}
return childArgs;
}

static void closeStreams(Process p) {
try {
p.getOutputStream().close();
@@ -0,0 +1,49 @@
/*
* 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>
#include <time.h>

/**
* Command line program to sleep at least given number of seconds.
*
* Note: the file name prefix "exe" identifies the source should be built into SleepMillis(.exe).
*/
int main(int argc, char** argv) {
// Use higher resolution nanosleep to be able to retry accurately if interrupted
struct timespec sleeptime;
int millis;

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

sleeptime.tv_sec = millis / 1000;
sleeptime.tv_nsec = (millis % 1000) * 1000 * 1000;
int rc;
while ((rc = nanosleep(&sleeptime, &sleeptime)) > 0) {
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.

is nanosleep a portable call? I couldn't find documentation for it with google search of nanosleep site:docs.microsoft.com.

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.

Sadly, true.
Falling back to the portable 'sleep(seconds)' is necessary; but the timing will be less precise.
C++ supports a higher resolution 'sleep_for' but the JDK native test build does not support building
main programs using C++ and its not worth the complications added.

// Repeat until == 0 or negative (error)
}
exit(rc == 0 ? 0 : 1);
}