8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error. #4235

alexmenkov · 2021-05-27T23:00:56Z

nsk test framework for testing jdi/jdwp assumes that the test launch debuggee first and then creates IOPipe to communicate with debuggee (debugger listens, debugee connects to the IOPipe socket).
This makes possible failure when debuggee tries to connect before debugger starts listening.
The fix changes logic of the tests to start listening on IOPipe socket before launch debuggee.
Other tests which use IOPipe/SocketIOPipe and have the same issue (there are a lot of them) will be fixes as separate issues.

Couple details about the fix:

debugee.getPipeServerSocket() just calls binder.getPipeServerSocket(), returned ServerSocket can be changed only by DebugeeBinder.prepareForPipeConnection which is called by some tests;
connectingProcess logic is broken. The only place it's used is BasicSocketConnection.checkConnectingProcess, which is called from SocketConnection.continueAttach. But continueAttach is called from debuggee code (listening == false) while connectingProcess is set only from debugger code (listening == true). So it didn't work and I dropped it.

Testing: all tests which use IOPipe/SocketIOPipe classes:

test/hotspot/jtreg/serviceability/dcmd/framework
test/hotspot/jtreg/vmTestbase/nsk/jdi
test/hotspot/jtreg/vmTestbase/nsk/jdwp

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error.

Reviewers

Chris Plummer (@plummercj - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4235/head:pull/4235
$ git checkout pull/4235

Update a local copy of the PR:
$ git checkout pull/4235
$ git pull https://git.openjdk.java.net/jdk pull/4235/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4235

View PR using the GUI difftool:
$ git pr show -t 4235

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4235.diff

bridgekeeper · 2021-05-27T23:01:50Z

👋 Welcome back amenkov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-05-27T23:05:11Z

@alexmenkov The following label will be automatically applied to this pull request:

serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-05-27T23:09:24Z

Webrevs

00: Full (ae4723e3)

plummercj · 2021-06-01T17:56:10Z

test/hotspot/jtreg/serviceability/dcmd/framework/process/TestJavaProcess.java

@@ -48,7 +48,7 @@ public static void main(String argv[]) {
        log("Waiting for the quit command from the test ...");
        String cmd = pipe.readln();
        int exitCode = PASSED;
-        if (cmd.equals("quit")) {
+        if ("quit".equals(cmd)) {


I don't see how this is an improvement.

Working on the fix I got null cmd here.
I think "Invalid command received null" failure is clearer than NPE

openjdk · 2021-06-03T20:33:42Z

@alexmenkov 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:

8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error.

Reviewed-by: cjplummer, sspitsyn

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 103 new commits pushed to the master branch:

460ce55: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
b955865: 8267995: Add reference to JVMS class file format in Lookup::defineHiddenClass
9f05c41: 8265783: Create a separate library for x86 Intel SVML assembly intrinsics
e27c4d4: 8268185: Update GitHub Actions for jtreg 6
68ac871: 8268189: ProblemList compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java in -Xcomp mode
af3df63: 8267598: jpackage removes system libraries from java.library.path
52d8215: 8268131: 2 java/foreign tests timed out
5405f98: 8268077: java.util.List missing from Collections Framework Overview
3aa7062: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"
5982cfc: 8266317: Vector API enhancements
... and 93 more: https://git.openjdk.java.net/jdk/compare/8a31c07598cd5ea1305a9706d80b0251fd3a1e6d...master

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 master branch, type /integrate in a new comment.

sspitsyn

Hi Alex,
This is nice refactoring. It looks good to me.
The fix has a potential to introduce new regressions.
I hope you run all the impacted tests on all platforms.
Thanks,
Serguei

sspitsyn · 2021-06-03T21:42:10Z

test/hotspot/jtreg/vmTestbase/nsk/share/jpda/SocketIOPipe.java

+            if (listenerThread == null) {
+                // start listening and accept connection on the current thread
+                listenerThread = new ListenerThread();
+                listenerThread.run();


It is not clear, why listenerThread.run() is used instead of listenerThread.start() as at the line 257.

This is part of the workaround for non-updated tests - the tests should work the same way they did.
start() would start new thread, so listening starts a bit later and we have more chances to get "connection refused" error.

sspitsyn · 2021-06-03T21:44:32Z

test/hotspot/jtreg/vmTestbase/nsk/share/jpda/SocketIOPipe.java

-            } else {
-                connection.setServerSocket(serverSocket);
+            // listenerThread == null means the test is not updated yet
+            // to start IOPipe listening before launching debuggee.


I like this workaround for non-updated tests. This spot can be simplified after all tests got updated.

Yes, the plan is to put assert here later.

alexmenkov · 2021-06-08T17:03:47Z

/integrate

openjdk · 2021-06-08T17:05:52Z

@alexmenkov Since your change was applied there have been 174 commits pushed to the master branch:

c21cc93: 8248877: Document API contract for MetaspaceObj subtypes
159cb6f: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac
8158b82: 8268327: Upstream: 8268169: The system lookup can not find stdio functions such as printf on Windows 10
6843576: 8268227: java/foreign/TestUpcall.java still times out
2717fcb: 8232948: javac -h should mangle the overload argument signature
89da202: 8266159: macOS ARM + Metal pipeline shows artifacts on Swing Menu with Java L&F
61ab4b9: 8267564: JDK-8252971 causes SPECjbb2015 socket exceptions on Windows when MKS is installed
00c88f7: 8266918: merge_stack in check_code.c add NULL check
8105478: 8268165: AsyncLogging will crash if rotate() fails
fd91b2a: 8265440: IGV: make node selection more visible
... and 164 more: https://git.openjdk.java.net/jdk/compare/8a31c07598cd5ea1305a9706d80b0251fd3a1e6d...master

Your commit was automatically rebased without conflicts.

Pushed as commit b568e87.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

FIx for serviceability/dcmd/framework tests

ae4723e

openjdk bot added the rfr Pull request is ready for review label May 27, 2021

openjdk bot added the serviceability serviceability-dev@openjdk.org label May 27, 2021

plummercj reviewed Jun 1, 2021

View reviewed changes

plummercj approved these changes Jun 3, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 3, 2021

sspitsyn approved these changes Jun 3, 2021

View reviewed changes

openjdk bot closed this Jun 8, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 8, 2021

alexmenkov deleted the nskPipe branch June 8, 2021 17:46

8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error. #4235

8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error. #4235

alexmenkov commented May 27, 2021 •

edited by openjdk bot

bridgekeeper bot commented May 27, 2021

openjdk bot commented May 27, 2021

mlbridge bot commented May 27, 2021

plummercj Jun 1, 2021

alexmenkov Jun 1, 2021

plummercj Jun 3, 2021

openjdk bot commented Jun 3, 2021 •

edited

sspitsyn left a comment •

edited

sspitsyn Jun 3, 2021

alexmenkov Jun 3, 2021

sspitsyn Jun 3, 2021

alexmenkov Jun 3, 2021 •

edited

alexmenkov commented Jun 8, 2021

openjdk bot commented Jun 8, 2021

8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error. #4235

8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error. #4235

Conversation

alexmenkov commented May 27, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 27, 2021

openjdk bot commented May 27, 2021

mlbridge bot commented May 27, 2021

Webrevs

plummercj Jun 1, 2021

Choose a reason for hiding this comment

alexmenkov Jun 1, 2021

Choose a reason for hiding this comment

plummercj Jun 3, 2021

Choose a reason for hiding this comment

openjdk bot commented Jun 3, 2021 • edited

sspitsyn left a comment • edited

Choose a reason for hiding this comment

sspitsyn Jun 3, 2021

Choose a reason for hiding this comment

alexmenkov Jun 3, 2021

Choose a reason for hiding this comment

sspitsyn Jun 3, 2021

Choose a reason for hiding this comment

alexmenkov Jun 3, 2021 • edited

Choose a reason for hiding this comment

alexmenkov commented Jun 8, 2021

openjdk bot commented Jun 8, 2021

alexmenkov commented May 27, 2021 •

edited by openjdk bot

openjdk bot commented Jun 3, 2021 •

edited

sspitsyn left a comment •

edited

alexmenkov Jun 3, 2021 •

edited