RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts

Stefan Karlsson stefan.karlsson at oracle.com
Thu Apr 30 09:07:54 UTC 2020


Hi all,

Please review this patch to make it less likely that we accidentally add 
or fail to add test.java.opts and test.vm.opts to our spawned test JVMs.

https://cr.openjdk.java.net/~stefank/8244078/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8244078

ProcessTools.createJavaProcessBuilder(cmd) creates a ProcessBuilder 
*without* test.java.opts and test.vm.opts. There is a 
(addTestVmAndJavaOptions, cmd) overload that allows the caller to opt-in 
to the addition of these flags. The created ProcessBuilder is then used 
to start the JVM, and almost always fed into an OutputAnalyzer.

There's another function executeTestJvm, that both creates a 
ProcessBuilder and then feeds it into an OutputAnalyzer (plus a bit 
more). This function uses createJavaProcessBuilder(true, cmd), and 
thereby adds the test.java.opts and test.vm.opts flags.

This means that one has to know about this difference when reading code 
using createJavaProcessBuilder(cmd) and executeTestJvm(cmd), and when 
creating (copying) code using these functions.

It has been suggested that createJavaProcessBuilder is intended to be a 
lower-level, building block API and that it's not confusing that they 
have different behavior. I don't really agree, but I'm buying into the 
notion of lower-level vs higher-level APIs here. So, my proposal is to 
remove the addTestVmAndJavaOptions feature from 
createJavaProcessBuilder, and instead create a new function called 
createTestJvm that adds test.java.opts and test.vm.opts. The name is 
intentionally similar to executeTestJvm, and in fact, the executeTestJvm 
implementation will use createTestJvm:

      public static OutputAnalyzer executeTestJvm(String... cmds) throws Exception {
- ProcessBuilder pb = createJavaProcessBuilder(true, cmds);
+ ProcessBuilder pb = createTestJvm(cmds);
          return executeProcess(pb);
      }

The rest of the patch is mainly:
- leaving createJavaProcessBuilder(cmd) as is
- replacing createJavaProcessBuilder(false, cmd) with 
createJavaProcessBuilder(cmd)
- replacing createJavaProcessBuilder(true, cmd) with createTestJvm(cmd)

There was one odd thing in jdi that requires extra scrutiny:
https://cr.openjdk.java.net/~stefank/8244078/webrev.01/test/jdk/com/sun/jdi/lib/jdb/Debuggee.java.udiff.html

I've run this through tier1-3, and are currently running this through 
higher tiers.

Thanks,
StefanK


More information about the serviceability-dev mailing list