8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket

[RealCLanger]

During analysing a customer case I figured out that we have an inconsistency between documentation and actual behavior in class com.sun.jndi.ldap.Connection. The method documentation of com.sun.jndi.ldap.Connection::createSocket states: "If a timeout is supplied but unconnected sockets are not supported then the timeout is ignored and a connected socket is created."

This, however does not happen. If a SocketFactory would not support unconnected sockets, it would likely throw a SocketException in SocketFactory::createSocket(). And since the code does not check for this behavior, a connection with timeout value through a SocketFactory that does not support unconnected sockets would simply fail with an IOException.

So we should either make the code adhere to what is documented or adapt the documentation to the actual behavior.

I hereby try to fix the connect coding. Alternatively, we could also adapt the description - I have no strong opinion. What do the experts suggest?

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8326482 to be approved

Issues

• JDK-8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket (Bug - P3)
• JDK-8326482: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket (CSR)

Reviewers

• Daniel Fuchs (@dfuch - Reviewer)
• Aleksei Efimov (@AlekseiEfimov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17797/head:pull/17797
$ git checkout pull/17797

Update a local copy of the PR:
$ git checkout pull/17797
$ git pull https://git.openjdk.org/jdk.git pull/17797/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17797

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17797.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back clanger! 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]

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

• core-libs

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]

Webrevs

• 08: Full - Incremental (8fdc039c)
• 07: Full - Incremental (10271159)
• 06: Full - Incremental (97299970)
• 05: Full - Incremental (ec6579d2)
• 04: Full - Incremental (d8d1d6db)
• 03: Full - Incremental (88ae0b27)
• 02: Full (c0dee34c)
• 01: Full - Incremental (48318eb2)
• 00: Full (419f96f5)

[AlekseiEfimov]

Hi Christoph,

I think the proposed change is good, and it solves the problem we've also seen before with custom socket factories specified in the "java.naming.ldap.factory.socket" JNDI environment property not implementing javax.net.SocketFactory::createSocket() method - custom implementations are not required to implement this method, hence SocketException can be thrown by the default implementation.
The change proposed by you should help to address such scenarios.

It would also be great to update the com.sun.jndi.ldap.connect.timeout env property documentation in the java.naming module-info with the code comment mentioned above.
To fully clarify the "unconnected sockets are not supported" statement the "java.naming.ldap.factory.socket" environment property might need to have documentation added.

I've launched JNDI/LDAP regression tests with your patch and no failures were observed.
As a good addition to the proposed fix, it would be great to have a test for scenarios when a custom socket factory does/doesn't override the createSocket method. There are a few test examples that can be used as a bootstrap - for example, test/jdk/com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java.

[RealCLanger]

Hi Aleksei,

thanks for taking a look. Yes, I'm working on extending the test. Stay tuned. 😄

Cheers
Christoph

[openjdk]

@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[openjdk]

@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[RealCLanger]

So, I've updated the PR. Sorry for force-pushing, hope you didn't review the code in detail yet.

The new version updates module-info as requested. I also enhanced the test LdapSSLHandshakeFailureTest.java and added variants that exercise a Socket Factory which does not support unconnected sockets. Two of the test variants would fail with the current JDK.

[AlekseiEfimov]

Currently, it is hard to distinguish what part of the test responsible for JDK-8314063 testing, and which part is for JDK-8325579.
I would prefer to add a new test for the current fix instead: that could be done as additional test mode, OR even better to add a completely new test.
As Daniel stated before, it is hard to review the change made in this PR because of the test name change. Previous suggestion might help to address that too.

[RealCLanger]

I made some updates and addressed the review comments. I reverted the renaming of the test for the sake of reviewing. While I still think LdapSSLHandshakeTest is a better, more generic name for the test's purpose, we could change its name in a separate change.

[RealCLanger]

Currently, it is hard to distinguish what part of the test responsible for JDK-8314063 testing, and which part is for JDK-8325579. I would prefer to add a new test for the current fix instead: that could be done as additional test mode, OR even better to add a completely new test.

Hm, I think the test was overpurposed already. Creating another test file with duplicating code does not sound too good IMHO. Maybe it is acceptable without the renaming?

Another question: Do we need a CSR/Release note as there is some behavioral change involved (although it should always have been like with this change)?

[AlekseiEfimov]

/csr needed

[openjdk]

@AlekseiEfimov has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@RealCLanger please create a CSR request for issue JDK-8325579 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[AlekseiEfimov]

Hm, I think the test was overpurposed already.

This test was added by JDK-8314063 fix, and I do not think it was change after that.

Creating another test file with duplicating code does not sound too good IMHO. Maybe it is acceptable without the renaming?

I think it is acceptable. Currently, it is hard to separate the test cases for a) the original JDK-8314063 fix (the opened socket is not closed properly when connection timeout occurs) and b) the current fix JDK-8325579 (unconnected sockets are not supported by SocketFactory). It would be great to have this distinction in the modified test.

[RealCLanger]

I drafted a CSR. @AlekseiEfimov, would be nice if you could review it.

As for the test, I had a closer look now and I find it hard to separate testing of JDK-8314063 from JDK-8325579. Furthermore, most of the entries test things that hadn't been addressed by any of these two bugs at all.

So, JDK-8314063 is only tested in lines 72, 73, 76 and 77
The original problem of this issue JDK-8325579 is touched in line 71 and 73.

That means, most of the other test invocations test some generic behavior which was never erroneous so far.

I could, however, give each line its own test id and annotate the bugs accordingly. Do you think that makes sense?

[AlekseiEfimov]

As for the test, I had a closer look now and I find it hard to separate testing of JDK-8314063 from JDK-8325579. Furthermore, most of the entries test things that hadn't been addressed by any of these two bugs at all.

So, JDK-8314063 is only tested in lines 72, 73, 76 and 77 The original problem of this issue JDK-8325579 is touched in line 71 and 73.

That means, most of the other test invocations test some generic behavior which was never erroneous so far.

Thanks for exploring the possibility of improving tracebility of test invocations to reported bugs.

I could, however, give each line its own test id and annotate the bugs accordingly. Do you think that makes sense?

It does make sense, but I'm not sure how such annotations will look like and if it will be easy to use them for debugging failures. I will leave the final decision to you here. Your last message with linkage of test invocations to bug id is already a good information to have.

I drafted a CSR. @AlekseiEfimov, would be nice if you could review it.

Thanks for drafting a CSR. I will review it in comming days.

[AlekseiEfimov]

Thanks for exploring the possibility of improving tracebility of test invocations to reported bugs.

I've given this test change a second thought, maybe you can try to separate the test into two separate test classes? One possibility to avoid duplicating code and have a separate test for each bug is to introduce a base test class that will contain the common functionality for JDK-8314063 and JDK-8325579 tests.

[RealCLanger]

Thanks for exploring the possibility of improving tracebility of test invocations to reported bugs.

I've given this test change a second thought, maybe you can try to separate the test into two separate test classes? One possibility to avoid duplicating code and have a separate test for each bug is to introduce a base test class that will contain the common functionality for JDK-8314063 and JDK-8325579 tests.

I will have a look.

[openjdk]

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

8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket

Reviewed-by: dfuchs, aefimov

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

• cc5cda5: 8328005: Convert java/awt/im/JTextFieldTest.java applet test to main
• ef2bd57: 8328558: Convert javax/swing/JCheckBox/8032667/bug8032667.java applet test to main
• e66788c: 8325179: Race in BasicDirectoryModel.validateFileCache
• 1496b5d: 8327924: Simplify TrayIconScalingTest.java
• 16ed191: 8328101: Parallel: Obsolete ParallelOldDeadWoodLimiterMean and ParallelOldDeadWoodLimiterStdDev
• 581b1e2: 8328402: Implement pausing functionality for the PassFailJFrame
• 75195aa: 8328299: Convert DnDFileGroupDescriptor.html applet test to main
• 725d87b: 8328570: Convert closed JViewport manual applet tests to main
• e41bc42: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync
• 4308017: 8328631: Convert java/awt/InputMethods/InputMethodsTest/InputMethodsTest.java applet test to manual
• ... and 14 more: https://git.openjdk.org/jdk/compare/000f4d8d156a48939fd29f8b4dd84b3dfd7d9d95...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.

[RealCLanger]

Thanks for exploring the possibility of improving tracebility of test invocations to reported bugs.

I've given this test change a second thought, maybe you can try to separate the test into two separate test classes? One possibility to avoid duplicating code and have a separate test for each bug is to introduce a base test class that will contain the common functionality for JDK-8314063 and JDK-8325579 tests.

I will have a look.

I stared at the test once more but it still doesn't make sense to me to split it into multiple tests because the purposes of each test run intertwine, as stated above. The only thing that would really make sense is to rename from LdapSSLHandshakeFailureTest to LdapSSLHandshakeTest since it in fact tests various variations of the SSL handshaking.

[dfuch]

LGTM. I haven't looked at the updated test too closely.

[AlekseiEfimov]

The latest version looks good to me with a couple minor comments and suggestions.

[dfuch]

Latest changes LGTM

[AlekseiEfimov]

The latest changes look good to me. Thank you for addressing all the comments.
I've tested the proposed change with JNDI regression tests, including the modified one, and there are no new issues discovered.

[RealCLanger]

Thanks @AlekseiEfimov and @dfuch for the reviews. Submitting this now.

/integrate

[openjdk]

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

• 70a15c7: 8328825: Google CAInterop test failures
• a1fbbff: 8328717: Convert javax/swing/JColorChooser/8065098/bug8065098.java applet test to main
• 26b2ec4: 8328824: Clean up java.base native compilation
• af15c68: 8321278: C2: Partial peeling fails with assert "last_peel <- first_not_peeled"
• 142c311: 8328744: Parallel: Parallel GC throws OOM before heap is fully expanded
• cb2a671: 8324121: SIGFPE in PhaseIdealLoop::extract_long_range_checks
• 0c1b254: 8326438: C2: assert(ld->in(1)->Opcode() == Op_LoadN) failed: Assumption invalid: input to DecodeN is not LoadN
• 29ba4b7: 8328705: GHA: Cross-compilation jobs do not require build JDK
• f67ec19: 8079786: [macosx] Test java/awt/Frame/DisposeParentGC/DisposeParentGC.java fails for Mac only
• 93579c2: 8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment
• ... and 70 more: https://git.openjdk.org/jdk/compare/000f4d8d156a48939fd29f8b4dd84b3dfd7d9d95...master

Your commit was automatically rebased without conflicts.

[openjdk]

@RealCLanger Pushed as commit 907e30f.

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