8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java #6883

jaikiran · 2021-12-17T14:52:53Z

Can I please get a review for this test only change which proposes to enable debug logs from the test that failed intermittently? This change addresses https://bugs.openjdk.java.net/browse/JDK-8278961.

The change passes the (test specific) -d option to enable logs from that test by default. While I was at it, I even added a few more debug logs hoping it might provide some hints if/when it fails next.

For reference, a (successful) run of this test will now print something like:

----------System.out:(18/930)----------
running on OS that supports ICMP port unreachable
Testing with class java.net.DatagramSocket
tests will be run against destination address localhost/127.0.0.1 port 52682
Checking send to connected address ...
socket is locally bound to address /127.0.0.1 port 52681
received data from address /127.0.0.1 port 52681
received data from address /127.0.0.1 port 52681
received data from address /127.0.0.1 port 52681
received data from address /127.0.0.1 port 52681
received data from address /127.0.0.1 port 52681
Checking send to non-connected address ...
received data from address /127.0.0.1 port 52681
Checking send to invalid address ...
Got expected exception: java.net.PortUnreachableException
Got expected exception: java.net.PortUnreachableException
Got expected exception: java.net.PortUnreachableException
Got expected exception: java.net.PortUnreachableException
Got expected exception: java.net.PortUnreachableException

Progress

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

Issue

JDK-8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java

Reviewers

@djelinski (no known github.com user name / role) ⚠️ Review applies to 721cd07
Daniel Fuchs (@dfuch - Reviewer)
Mark Sheppard (@msheppar - Reviewer) ⚠️ Review applies to 721cd07

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6883

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

Using diff file

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

…ToBadAddress.java

bridgekeeper · 2021-12-17T14:54:15Z

👋 Welcome back jpai! 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-12-17T14:56:20Z

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

net

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-12-17T15:01:39Z

Webrevs

01: Full - Incremental (387dc5d1)
00: Full (721cd07d)

djelinski · 2021-12-17T16:35:18Z

Does the OSsupportsFeature method still make sense? It mentions Windows 2000; I'm pretty sure every Windows version supports this today.

Also, I took a peek at JDK-8278858. The logs mention

elapsed time (seconds): 20.069

which is consistent with 5 (tries) *4seconds (timeout), which suggests that either the host firewalls ICMP "destination unreachable" packets, or our serversock.close (); call is somehow broken on that machine.

djelinski · 2021-12-17T16:21:12Z

test/jdk/java/net/DatagramSocket/SendDatagramToBadAddress.java

@@ -170,6 +175,8 @@ private void test(DatagramSocket sock) throws Exception {
                sock.send(p);
                p = new DatagramPacket(buf, buf.length, addr, port);
                sock.receive (p);
+                print("(unexpectedly) received data from address " + p.getAddress()
+                        + " port " + p.getPort() + " on attempt " + i);
            } catch (InterruptedIOException ex) {
                print ("socket timeout");
            } catch (Exception ex) {


Can we catch PortUnreachableException here instead?

Here too I didn't want to change the current behaviour/code of the test. It's not just this catch block but even the one a few lines above which catches InterruptedIOException. Neither the send() nor the receive() APIs of DatagramSocket specify that they throw this specific exception, but the test seems to catch it and consider it a socket timeout. So I'm guessing this test and the catch block was written with some specific context in mind and I didn't want to change that part as part of this PR which only aims to enable logging by default. Plus like Mark notes, the spec says PortUnreachableException "may" be thrown and isn't guaranteed.

Right. Looks like OSsupportsFeature was used to filter out systems that don't send "port unreachable"; we could probably use it here if the failure turns out to be OS-specific.

Additional logs look fine.

as I indicated below, we have tracked and investigated this issue and it is not purely a macosx-aarch64 issue. It may also be a test env issue. As such using the OS filtering in the test, which is designed to primarily exclued AIX, wouldn't be the appropraite approach. If we wish to exclude the test on macosx-aarch64, then use of Problemlist is more appropriate.

Hello Mark,

I just noticed this comment. My understanding of the comment is that you don't expect any other changes to this test other than what has already been done in this PR to enable the debug logs? Is that correct?

yes, the addition of the diagnostics is useful

djelinski · 2021-12-17T16:21:59Z

test/jdk/java/net/DatagramSocket/SendDatagramToBadAddress.java

@@ -170,6 +175,8 @@ private void test(DatagramSocket sock) throws Exception {
                sock.send(p);
                p = new DatagramPacket(buf, buf.length, addr, port);
                sock.receive (p);
+                print("(unexpectedly) received data from address " + p.getAddress()


We should immediately fail the test here

Hello Daniel,
I had considered that when doing this change. But this test itself appears to be very old one and I don't have any historical context of why it is doing these checks in a loop. Furthermore, even after it receives an exception (either a socket timeout one or any general exception), after setting a flag in case of a general exception, the test continues to run the next iteration of the loop. So I didn't want to change that logic, at least not this PR whose aim is to just enable debug logging.
However, if others too think we should fail here immediately, I'll go ahead do that change.

msheppar · 2021-12-17T18:04:17Z

it is a good idea to turn on the diagnostics -- we have used it in private test runs.

the current crop of failures are on macos-aarch64, it appears to be an OS issue, in that an ICMP destination unreachabale is not being generated for the UDP sends to an non existent socket end point. BUT, there is no obligation to do so -- it is not mandaotory in the spec, something along the lines:

"If, in the destination host, the IP module cannot deliver the datagram because the indicated protocol module or process port is not active, the destination host may send a destination unreachable message to the source host."

The question is: "is it a bug or feature in the OS?" Or it could be a host confoguration issue. There may be some configuration that has disabled ICMP, maybe a firewall policy or security setting in the kerenal, not to generate ICMP DST Unreachable responses?
We haven't found one yet.

dfuch

Hi Jaikiran,

What you have seems appropriate to me. Hopefully the added debug information can help us better diagnose the issue (validate/invalidate some hypothesis) next time the test fails.

dfuch · 2022-01-12T14:21:20Z

test/jdk/java/net/DatagramSocket/SendDatagramToBadAddress.java

@@ -24,11 +24,11 @@
 /*
 * @test
 *
- * @bug 4204320
+ * @bug 4204320 8278961


Ah - can you remove 8278961 from the bug list before integrating?

@bug is supposed to be used to list the bugs that the test is verifying...

Hello Daniel,
I removed that bug id and also updated the copyright year to the new year and updated the PR. No other changes.

openjdk · 2022-01-12T14:21:37Z

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

8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java

Reviewed-by: dfuchs, msheppar

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

b61a4af: 8259774: Deprecate -XX:FlightRecorderOptions:samplethreads
6933934: 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java
4851948: 8279903: Redundant modulo operation in ECDHKeyAgreement
67e3d51: Merge
5aecb37: 8206181: ExceptionInInitializerError: improve handling of exceptions in user-provided taglets
86d0abb: 8279695: [TESTBUG] modify compiler/loopopts/TestSkeletonPredicateNegation.java to run on C1 also
6d7db4b: 8279356: Method linking fails with guarantee(mh->adapter() != NULL) failed: Adapter blob must already exist!
92307e5: 8278489: Preserve result in native wrapper with +UseHeavyMonitors
bbc1ddb: 8278267: ARM32: several vector test failures for ASHR
cb25029: 8279800: isAssignableFrom checks in AlgorithmParametersSpi.engineGetParameterSpec appear to be backwards
... and 193 more: https://git.openjdk.java.net/jdk/compare/3607a5cdd9a3013851c8faefd346f04934f897e6...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.

dfuch

LGTM. Please integrate :-)

jaikiran · 2022-01-13T13:22:44Z

Thank you Daniel, Mark and @djelinski for the reviews.

jaikiran · 2022-01-13T13:22:51Z

/integrate

openjdk · 2022-01-13T13:24:06Z

Going to push as commit c17a012.
Since your change was applied there have been 203 commits pushed to the master branch:

b61a4af: 8259774: Deprecate -XX:FlightRecorderOptions:samplethreads
6933934: 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java
4851948: 8279903: Redundant modulo operation in ECDHKeyAgreement
67e3d51: Merge
5aecb37: 8206181: ExceptionInInitializerError: improve handling of exceptions in user-provided taglets
86d0abb: 8279695: [TESTBUG] modify compiler/loopopts/TestSkeletonPredicateNegation.java to run on C1 also
6d7db4b: 8279356: Method linking fails with guarantee(mh->adapter() != NULL) failed: Adapter blob must already exist!
92307e5: 8278489: Preserve result in native wrapper with +UseHeavyMonitors
bbc1ddb: 8278267: ARM32: several vector test failures for ASHR
cb25029: 8279800: isAssignableFrom checks in AlgorithmParametersSpi.engineGetParameterSpec appear to be backwards
... and 193 more: https://git.openjdk.java.net/jdk/compare/3607a5cdd9a3013851c8faefd346f04934f897e6...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-13T13:24:23Z

@jaikiran Pushed as commit c17a012.

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

8278961: Enable debug logging in java/net/DatagramSocket/SendDatagram…

721cd07

…ToBadAddress.java

openjdk bot added the rfr Pull request is ready for review label Dec 17, 2021

openjdk bot added the net net-dev@openjdk.org label Dec 17, 2021

djelinski reviewed Dec 17, 2021

View reviewed changes

djelinski approved these changes Dec 20, 2021

View reviewed changes

dfuch approved these changes Jan 12, 2022

View reviewed changes

dfuch reviewed Jan 12, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 12, 2022

msheppar approved these changes Jan 12, 2022

View reviewed changes

fix @bug in test definition and fix copyright year

387dc5d

dfuch approved these changes Jan 13, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 13, 2022

openjdk bot closed this Jan 13, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 13, 2022

jaikiran deleted the 8278961 branch January 13, 2022 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java #6883

8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java #6883

jaikiran commented Dec 17, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 17, 2021

openjdk bot commented Dec 17, 2021

mlbridge bot commented Dec 17, 2021 •

edited

Loading

djelinski commented Dec 17, 2021

djelinski Dec 17, 2021

jaikiran Dec 18, 2021 •

edited

Loading

djelinski Dec 20, 2021

msheppar Dec 20, 2021

jaikiran Jan 12, 2022

msheppar Jan 12, 2022

djelinski Dec 17, 2021

jaikiran Dec 18, 2021

msheppar commented Dec 17, 2021

dfuch left a comment

dfuch Jan 12, 2022 •

edited

Loading

dfuch Jan 12, 2022

jaikiran Jan 13, 2022

openjdk bot commented Jan 12, 2022 •

edited

Loading

dfuch left a comment

jaikiran commented Jan 13, 2022

jaikiran commented Jan 13, 2022

openjdk bot commented Jan 13, 2022

openjdk bot commented Jan 13, 2022

8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java #6883

8278961: Enable debug logging in java/net/DatagramSocket/SendDatagramToBadAddress.java #6883

Conversation

jaikiran commented Dec 17, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 17, 2021

openjdk bot commented Dec 17, 2021

mlbridge bot commented Dec 17, 2021 • edited Loading

Webrevs

djelinski commented Dec 17, 2021

Choose a reason for hiding this comment

jaikiran Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msheppar commented Dec 17, 2021

dfuch left a comment

Choose a reason for hiding this comment

dfuch Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jan 12, 2022 • edited Loading

dfuch left a comment

Choose a reason for hiding this comment

jaikiran commented Jan 13, 2022

jaikiran commented Jan 13, 2022

openjdk bot commented Jan 13, 2022

openjdk bot commented Jan 13, 2022

jaikiran commented Dec 17, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 17, 2021 •

edited

Loading

jaikiran Dec 18, 2021 •

edited

Loading

dfuch Jan 12, 2022 •

edited

Loading

openjdk bot commented Jan 12, 2022 •

edited

Loading