On Thu, 27 May 2021 23:00:56 GMT, Alex Menkov <[email protected]> wrote:
> 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
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
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/SocketIOPipe.java line 275:
> 273: if (listening) {
> 274: // listenerThread == null means the test is not updated yet
> 275: // 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.
test/hotspot/jtreg/vmTestbase/nsk/share/jpda/SocketIOPipe.java line 279:
> 277: // start listening and accept connection on the current
> thread
> 278: listenerThread = new ListenerThread();
> 279: listenerThread.run();
It is not clear, why `listenerThread.run()` is used instead of
`listenerThread.start()` as at the line 257.
-------------
Marked as reviewed by sspitsyn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4235
