8319651: Several network tests ignore vm flags when start java process #17787
Conversation
|
👋 Welcome back dclarke! A progress list of the required criteria for merging this PR into |
|
@DarraghClarke The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| new ProcessBuilder( | ||
| java, "-cp", ".", | ||
| ProcessTools.createTestJavaProcessBuilder( | ||
| "-cp", ".", |
There was a problem hiding this comment.
If I'm not mistaken, this will result in the -cp option being passed twice, with different values, to the java sub process. I believe that ProcessTools.createTestJavaProcessBuilder will end up passing "-cp <java.class.path>" to the subprocess, unless -Dtest.noclasspath=false is defined in the parent process.
Tough I could not find what would happen if -cp is passed twice in man java - it appears that the long standing behaviour is that the last one wins - so I guess that's OK, as I doubt this behaviour (last one wins) could be changed...
An alternative could be to create a new method in ProcessTools that would allow to pass a flag to prevent the addition of -cp <java.class.path> when that's not desiravle, and call that here - and in all other places where -cp is passed to ProcessTools.createTestJavaProcessBuilder.
There was a problem hiding this comment.
Would it be possible to change the createJavaProcessBuilder method in ProcessTools to include something to check if -cp is already an argument being passed?
maybe something like this?
if (!noCP && !args.contains("-cp")) {
args.add("-cp");
args.add(System.getProperty("java.class.path"));
}
There was a problem hiding this comment.
That's not a bad idea. What I would recommend in that case would be to not do anything here - but rather log another issue against ProcessTools.createTestJavaProcessBuilder.
Then proceed with this PR without changing anything...
There was a problem hiding this comment.
Hello Darragh, Daniel,
As for introducing a new method on ProcessTools class, I don't know if it's needed - I feel that the ProcessTools class is already getting too complex because of the various similarly named methods. I think it might be better to reuse some of those existing /test/lib utility methods to achieve the same, something like:
final List<String> command = new ArrayList<>();
command.add(JDKToolFinder.getJDKTool("java"));
command.addAll(jdk.test.lib.Utils.prependTestJavaOpts("-cp", classpath, className, appArgs));
final ProcessBuilder pb = new ProcessBuilder(command);
final OutputAnalyzer outputAnalyzer = ProcessTools.executeCommand(pb);I haven't tried out this snippet to be sure this works as expected.
I am not suggesting we do this change in this current PR.
There was a problem hiding this comment.
Hi Jaikiran,
What Darragh suggested was to modify the current method in ProcessTools to not add -cp <java.class.path> if the argument it is given already contains -cp. Since the first -cp added by the ProcessTools should be ignored anyway by the compiler (last one wins) - then I think it's a good idea, but to follow up outside of the PR.
There was a problem hiding this comment.
Sorry Daniel and Darragh, I misunderstood the discussion and thought it was a proposal to add a new method in that class. What you suggest about changing the implementation of that existing method, in a separate PR, sounds fine to me.
|
@DarraghClarke This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 123 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>
|
Thanks for the suggestions and feedback, I think I've addressed everything now so will leave some time in case there is anything else and to rerun tests but will merge later. |
|
/integrate |
|
Going to push as commit 9538f5d.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@DarraghClarke Pushed as commit 9538f5d. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Currently these tests ignore vm flags, In most cases I've updated them to use
ProcessTools.createTestJavaProcessBuilderthis usually required some cleanup also.test/jdk/java/net/ServerSocket/AcceptCauseFileDescriptorLeak.javaandtest/jdk/java/net/URLConnection/6212146/TestDriver.javahave been set to use@require vm.flaglessbecause they both useshcommands.I've ran these changes against tiers 1-3 and everything seems stable
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17787/head:pull/17787$ git checkout pull/17787Update a local copy of the PR:
$ git checkout pull/17787$ git pull https://git.openjdk.org/jdk.git pull/17787/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17787View PR using the GUI difftool:
$ git pr show -t 17787Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17787.diff
Webrev
Link to Webrev Comment