Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v6]
On Fri, 3 Nov 2023 07:22:26 GMT, Johannes Bechberger wrote: >> Fix race condition in debugger port selection, introduced with >> [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920). >> >> Tested on my Mac M1, but it doesn't contain platform-dependent code. > > Johannes Bechberger has updated the pull request incrementally with one > additional commit since the last revision: > > Update test/jdk/com/sun/jdi/lib/jdb/Debuggee.java > > Co-authored-by: Alex Menkov <69548902+alexmen...@users.noreply.github.com> I forgot that I wanted it to backport from 17u-dev. - PR Comment: https://git.openjdk.org/jdk/pull/16358#issuecomment-1884891560
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v6]
> Fix race condition in debugger port selection, introduced with > [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920). > > Tested on my Mac M1, but it doesn't contain platform-dependent code. Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/com/sun/jdi/lib/jdb/Debuggee.java Co-authored-by: Alex Menkov <69548902+alexmen...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/16358/files - new: https://git.openjdk.org/jdk/pull/16358/files/8a9b7133..8168a8ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16358=05 - incr: https://webrevs.openjdk.org/?repo=jdk=16358=04-05 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16358.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16358/head:pull/16358 PR: https://git.openjdk.org/jdk/pull/16358
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v5]
On Thu, 2 Nov 2023 11:35:29 GMT, Johannes Bechberger wrote: >> Fix race condition in debugger port selection, introduced with >> [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920). >> >> Tested on my Mac M1, but it doesn't contain platform-dependent code. > > Johannes Bechberger has updated the pull request incrementally with one > additional commit since the last revision: > > Refactor as requested Marked as reviewed by amenkov (Reviewer). test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 132: > 130: onthrow.isEmpty() ? > 131: JDWP::parseListenAddress : > 132: Launcher::parseLaunchEchoListenAddress Please increase indent for conditional operator Suggestion: onthrow.isEmpty() ? JDWP::parseListenAddress : Launcher::parseLaunchEchoListenAddress or Suggestion: onthrow.isEmpty() ? JDWP::parseListenAddress : Launcher::parseLaunchEchoListenAddress - PR Review: https://git.openjdk.org/jdk/pull/16358#pullrequestreview-1711398441 PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1380882396
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v5]
> Fix race condition in debugger port selection, introduced with > [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920). > > Tested on my Mac M1, but it doesn't contain platform-dependent code. Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Refactor as requested - Changes: - all: https://git.openjdk.org/jdk/pull/16358/files - new: https://git.openjdk.org/jdk/pull/16358/files/8788131e..8a9b7133 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16358=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16358=03-04 Stats: 36 lines in 1 file changed: 14 ins; 16 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16358.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16358/head:pull/16358 PR: https://git.openjdk.org/jdk/pull/16358
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v4]
> Fix race condition in debugger port selection, introduced with > [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920). > > Tested on my Mac M1, but it doesn't contain platform-dependent code. Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/com/sun/jdi/lib/jdb/Debuggee.java Co-authored-by: Alex Menkov <69548902+alexmen...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/16358/files - new: https://git.openjdk.org/jdk/pull/16358/files/ff651a94..8788131e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16358=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16358=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16358.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16358/head:pull/16358 PR: https://git.openjdk.org/jdk/pull/16358
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v3]
On Thu, 26 Oct 2023 07:51:56 GMT, Johannes Bechberger
wrote:
>> Fix race condition in debugger port selection, introduced with
>> [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920).
>>
>> Tested on my Mac M1, but it doesn't contain platform-dependent code.
>
> Johannes Bechberger has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add suggested fixes
test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 212:
> 210: */
> 211: private static JDWP.ListenAddress
> parseLaunchEchoListenAddress(String debuggeeOutput) {
> 212: Pattern listenRegexp = Pattern.compile("Listen Args: \\b(.+)\\b
> \\b(.+)\\b");
"Listen Args:" should be a static final and also referenced above where the
"echo" command is used.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1373766455
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v3]
On Thu, 26 Oct 2023 07:51:56 GMT, Johannes Bechberger
wrote:
>> Fix race condition in debugger port selection, introduced with
>> [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920).
>>
>> Tested on my Mac M1, but it doesn't contain platform-dependent code.
>
> Johannes Bechberger has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add suggested fixes
test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 107:
> 105: }
> 106:
> 107: // required to pass non null port with address and emit string
> before the throw
The comment is obsolete
test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 129:
> 127:
> 128: public Debuggee launch(String name) {
> 129: return new Debuggee(prepare(), name, s -> {
Address detector functions can be passed without wrapping:
`
return new Debuggee(prepare(), name,
onthrow.isEmpty() ?
JDWP::parseListenAddress : Debuggee::parseLaunchEchoListenAddress)
`
But maybe current code is more readable, so it's up to you
test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 141:
> 139: }
> 140:
> 141: // starts the process, waits for "Listening for transport" output
> and detects transport/address
Suggestion:
// starts the process, waits until the provided addressDetector detects
transport/address from the process output
test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 211:
> 209: * Returns null if the string specified does not contain required
> info.
> 210: */
> 211: private static JDWP.ListenAddress
> parseLaunchEchoListenAddress(String debuggeeOutput) {
The logic belongs to Launcher, please move the method to Launcher class
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1373538027
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1373595837
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1373537417
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1373607149
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v3]
> Fix race condition in debugger port selection, introduced with > [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920). > > Tested on my Mac M1, but it doesn't contain platform-dependent code. Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision: Add suggested fixes - Changes: - all: https://git.openjdk.org/jdk/pull/16358/files - new: https://git.openjdk.org/jdk/pull/16358/files/561956a8..ff651a94 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16358=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16358=01-02 Stats: 46 lines in 3 files changed: 21 ins; 19 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16358.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16358/head:pull/16358 PR: https://git.openjdk.org/jdk/pull/16358
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use" [v2]
> Fix race condition in debugger port selection, introduced with > [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920). > > Tested on my Mac M1, but it doesn't contain platform-dependent code. Johannes Bechberger has updated the pull request incrementally with two additional commits since the last revision: - Update test/jdk/com/sun/jdi/lib/jdb/Debuggee.java Co-authored-by: Alex Menkov <69548902+alexmen...@users.noreply.github.com> - Update test/jdk/com/sun/jdi/lib/jdb/Debuggee.java Co-authored-by: Alex Menkov <69548902+alexmen...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/16358/files - new: https://git.openjdk.org/jdk/pull/16358/files/0a07f721..561956a8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16358=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16358=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16358.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16358/head:pull/16358 PR: https://git.openjdk.org/jdk/pull/16358
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
On Wed, 25 Oct 2023 11:27:18 GMT, Johannes Bechberger
wrote:
> Fix race condition in debugger port selection, introduced with
> [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920).
>
> Tested on my Mac M1, but it doesn't contain platform-dependent code.
test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 106:
> 104:
> 105: // required to pass non null port with address and emit string
> before the throw
> 106: public Launcher enableOnThrow(String value, String
> expectedOutputBeforeThrow) {
Suggestion:
public Launcher enableOnThrow(String exceptionClassName) {
expectedOutputBeforeThrow is not needed.
Comment for the method is obsolete
test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 136:
> 134:
> 135: // starts the process, waits for "Listening for transport" output
> and detects transport/address
> 136: private Debuggee(ProcessBuilder pb, String name, boolean hasOnThrow,
> String expectedOutputBeforeThrow) {
expectedOutputBeforeThrow is not used/not neded
test/jdk/com/sun/jdi/lib/jdb/Debuggee.java line 136:
> 134:
> 135: // starts the process, waits for "Listening for transport" output
> and detects transport/address
> 136: private Debuggee(ProcessBuilder pb, String name, boolean hasOnThrow,
> String expectedOutputBeforeThrow) {
Launcher prepares command line and address detection logic depends on on the
command line, so I think it would be better to make
Suggestion:
private Debuggee(ProcessBuilder pb, String name, Function addressDetector) {
then warm-up predicate in the ctor uses `listenAddress[0] =
addressDetector.apply(s)`
Launcher.launch() can pass JDWP::parseListenAddress for usual cases and
implement the detector for onthrow case
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1372415102
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1372403856
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1372413049
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
On Wed, 25 Oct 2023 17:23:34 GMT, Chris Plummer wrote:
>> `parseListenAddress()` looks for a fairly specific format:
>>
>> `listenRegexp = Pattern.compile("Listening for transport \\b(.+)\\b at
>> address: \\b(.+)\\b");`
>>
>> However, it does not assume the transport type, so maybe doing that here is
>> not such a good idea. I don't like adding shell scripts to tests. We've been
>> converting tests from shell scripts for years now because they tend to be
>> problematic.
>>
>> How about using something like launch="echo Listen Args: ". then you have a
>> more specific string you can pattern match on:
>>
>> `Listen Args: dt_socket 42305`
>
> Maybe you can use call `enableOnThrow()` as a trigger to use a specific echo
> command whose pattern is later detected. I'm not so sure
> `parseLaunchEchoListenAddress()` needs to be in JDWP since it will only be
> used by the Debuggee class. That spares the JDWP class from needing to know
> about this pattern.
Good to know that special shell scripts are to be avoided. I agree that your
proposal is better than just the simple ’echo’ and fix it accordingly.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1372106905
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
On Wed, 25 Oct 2023 17:09:09 GMT, Chris Plummer wrote:
>> Making too many not enough assumptions. The other method didn't make
>> assumptions on the format of address and connection type, so I thought that
>> I shouldn't too. I could create a temporary shell file that mimicks the
>> other output though.
>
> `parseListenAddress()` looks for a fairly specific format:
>
> `listenRegexp = Pattern.compile("Listening for transport \\b(.+)\\b at
> address: \\b(.+)\\b");`
>
> However, it does not assume the transport type, so maybe doing that here is
> not such a good idea. I don't like adding shell scripts to tests. We've been
> converting tests from shell scripts for years now because they tend to be
> problematic.
>
> How about using something like launch="echo Listen Args: ". then you have a
> more specific string you can pattern match on:
>
> `Listen Args: dt_socket 42305`
Maybe you can use call `enableOnThrow()` as a trigger to use a specific echo
command whose pattern is later detected. I'm not so sure
`parseLaunchEchoListenAddress()` needs to be in JDWP since it will only be used
by the Debuggee class. That spares the JDWP class from needing to know about
this pattern.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1372093847
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
On Wed, 25 Oct 2023 16:53:18 GMT, Johannes Bechberger
wrote:
>> test/lib/jdk/test/lib/JDWP.java line 60:
>>
>>> 58: if (parts.length != 2) {
>>> 59: return null;
>>> 60: }
>>
>> This is a bit presumptuous in that it assumes that the first 2 token line of
>> output is the line we are looking for. The reason `parseListenAddress()`
>> tries to match a specific pattern is because sometimes tests are run in a
>> way that causes the debuggee to produce some extra output (like logging
>> output) and this output needs to be skipped. At the very least you should
>> be checking for a pattern that includes "dt_socket".
>>
>> BTW, I tried using `printf` instead of `echo` so the debug agent could be
>> made to generate a "Listening for transport..." line, in which case this new
>> `parseLaunchEchoListenAddress()` would not be needed, but I could not get
>> the argument processing right. Something about exec() and printf just
>> doesn't work right.
>
> Making too many not enough assumptions. The other method didn't make
> assumptions on the format of address and connection type, so I thought that I
> shouldn't too. I could create a temporary shell file that mimicks the other
> output though.
`parseListenAddress()` looks for a fairly specific format:
`listenRegexp = Pattern.compile("Listening for transport \\b(.+)\\b at address:
\\b(.+)\\b");`
However, it does not assume the transport type, so maybe doing that here is not
such a good idea. I don't like adding shell scripts to tests. We've been
converting tests from shell scripts for years now because they tend to be
problematic.
How about using something like launch="echo Listen Args: ". then you have a
more specific string you can pattern match on:
`Listen Args: dt_socket 42305`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1372079078
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
On Wed, 25 Oct 2023 16:39:31 GMT, Chris Plummer wrote:
>> Fix race condition in debugger port selection, introduced with
>> [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920).
>>
>> Tested on my Mac M1, but it doesn't contain platform-dependent code.
>
> test/lib/jdk/test/lib/JDWP.java line 60:
>
>> 58: if (parts.length != 2) {
>> 59: return null;
>> 60: }
>
> This is a bit presumptuous in that it assumes that the first 2 token line of
> output is the line we are looking for. The reason `parseListenAddress()`
> tries to match a specific pattern is because sometimes tests are run in a way
> that causes the debuggee to produce some extra output (like logging output)
> and this output needs to be skipped. At the very least you should be
> checking for a pattern that includes "dt_socket".
>
> BTW, I tried using `printf` instead of `echo` so the debug agent could be
> made to generate a "Listening for transport..." line, in which case this new
> `parseLaunchEchoListenAddress()` would not be needed, but I could not get the
> argument processing right. Something about exec() and printf just doesn't
> work right.
Making too many not enough assumptions. The other method didn't make
assumptions on the format of address and connection type, so I thought that I
shouldn't too. I could create a temporary shell file that mimicks the other
output though.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1372061956
Re: RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
On Wed, 25 Oct 2023 11:27:18 GMT, Johannes Bechberger
wrote:
> Fix race condition in debugger port selection, introduced with
> [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920).
>
> Tested on my Mac M1, but it doesn't contain platform-dependent code.
test/lib/jdk/test/lib/JDWP.java line 60:
> 58: if (parts.length != 2) {
> 59: return null;
> 60: }
This is a bit presumptuous in that it assumes that the first 2 token line of
output is the line we are looking for. The reason `parseListenAddress()` tries
to match a specific pattern is because sometimes tests are run in a way that
causes the debuggee to produce some extra output (like logging output) and this
output needs to be skipped. At the very least you should be checking for a
pattern that includes "dt_socket".
BTW, I tried using `printf` instead of `echo` so the debug agent could be made
to generate a "Listening for transport..." line, in which case this new
`parseLaunchEchoListenAddress()` would not be needed, but I could not get the
argument processing right. Something about exec() and printf just doesn't work
right.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16358#discussion_r1372046981
RFR: JDK-8318736: com/sun/jdi/JdwpOnThrowTest.java failed with "transport error 202: bind failed: Address already in use"
Fix race condition in debugger port selection, introduced with [JDK-8317920](https://bugs.openjdk.org/browse/JDK-8317920). Tested on my Mac M1, but it doesn't contain platform-dependent code. - Commit messages: - Fix Debuggee creation Changes: https://git.openjdk.org/jdk/pull/16358/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16358=00 Issue: https://bugs.openjdk.org/browse/JDK-8318736 Stats: 46 lines in 3 files changed: 17 ins; 23 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16358.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16358/head:pull/16358 PR: https://git.openjdk.org/jdk/pull/16358
