JDK-8277795: ldap connection timeout not honoured under contention #6568

robm-openjdk · 2021-11-25T23:54:18Z

This fix attemps to resolve an issue where threads can stack up on each other while waiting to get a connection from the ldap pool to an unreachable server. It does this by having each thread start a countdown prior to holding the pools' lock. (which has been changed to a ReentrantLock) Once the lock has been grabbed, the timeout is adjusted to take the waiting time into account and the process of getting a connection from the pool or creating a new one commences.

Note: this fix also changes the meaning of the connection pools initSize somewhat. In a situation where we have a large initSize and a small timeout the first thread could actually exhaust the timeout before creating all of its initial connections. Instead this fix simply creates a single connection per pool-connection-request. It continues to do so for subsequent requests regardless of whether an existing unused connection is available in the pool until initSize is exhausted. As such it may require a CSR.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-8277795: ldap connection timeout not honoured under contention
JDK-8280829: ldap connection timeout not honoured under contention (CSR)

Reviewers

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6568

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

Using diff file

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

bridgekeeper · 2021-11-25T23:55:33Z

👋 Welcome back robm! 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-11-25T23:57:04Z

@robm-openjdk 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 · 2021-11-26T00:15:22Z

Webrevs

dfuch

What testing is there for this fix?

dfuch · 2021-11-26T10:20:11Z

src/java.naming/share/classes/com/sun/jndi/ldap/LdapClientFactory.java

+    public PooledConnection createPooledConnection(PoolCallback pcb, long timeout)
+        throws NamingException {
+        return new LdapClient(host, port, socketFactory,
+                (int)timeout, readTimeout, trace, pcb);


A blunt cast from long to int is a bit worrying as it could lead to positive values becoming negative, unless you have checks in place in the calling code that will ensure that the long value is never > Integer.MAX_VALUE? And it could also result in a large value becoming a small positive value.
I'd suggest to remove the inconsistency one way or the other - or add an explicit check to make it obvious that this case cannot happen.

Good point. I've added a check to the case. (this actually comes from LdapPoolManager.getLdapClient which takes an int for the connection timeout parameter, but it makes sense to be careful)

dfuch · 2021-11-26T10:48:51Z

src/java.naming/share/classes/com/sun/jndi/ldap/pool/Pool.java

+        remaining = checkRemaining(start, remaining);
+
+        if (!conns.grabLock(remaining)) {
+            throw new NamingException("Timed out waiting for lock");


Is this the appropriate exception? I see in checkRemaining:

throw new CommunicationException( "Timeout exceeded while waiting for a connection: " + timeout + "ms");

I've changed this to a CommuncationException.

robm-openjdk · 2021-11-27T00:00:19Z

What testing is there for this fix?

I've just added a test modelled on LdapTimeoutTest.java. (with some whitespace issues which I'm about to fix!)

dfuch · 2022-01-11T15:24:06Z

test/jdk/com/sun/jndi/ldap/LdapPoolTimeoutTest.java

+        env.put("com.sun.jndi.ldap.read.timeout", String.valueOf(READ_MILLIS));
+        env.put("com.sun.jndi.ldap.connect.timeout", String.valueOf(CONNECT_MILLIS));
+        env.put("com.sun.jndi.ldap.connect.pool", "true");
+        env.put(Context.PROVIDER_URL, "ldap://example.com:1234");


I assume this makes the assumption that requests to "example.com:1234" will fail in timeout?
If so wouldn't it be safer to create a ServerSocket that never accepts connections?

Otherwise looks OK to me.

openjdk · 2022-01-11T18:11:37Z

@robm-openjdk 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:

8277795: ldap connection timeout not honoured under contention

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

a1d1e47: 8280823: Remove NULL check in DumpTimeClassInfo::is_excluded
094db1a: 8277948: AArch64: Print the correct native stack if -XX:+PreserveFramePointer when crash
7857405: 8280744: Allow SuppressWarnings to be used in all declaration contexts
40a2ce2: 8270476: Make floating-point test infrastructure more lambda and method reference friendly
6d242e4: 8280835: jdk/javadoc/tool/CheckManPageOptions.java depends on source hierarchy
ece89c6: 8280366: (fs) Restore Files.createTempFile javadoc
b94ebaa: 8280686: Remove Compile::print_method_impl
a3a0dcd: 8280353: -XX:ArchiveClassesAtExit should print warning if base archive failed to load
cab5905: 8280583: Always build NMT
7f68759: 8280719: G1: Remove outdated comment in RemoveSelfForwardPtrObjClosure::apply
... and 72 more: https://git.openjdk.java.net/jdk/compare/ab2c8d3c9baf1080f436287785e4e02fd79953a7...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.

AlekseiEfimov

The changes look good to me.
Few minor comments:
The last modification year in copyright headers can be updated.

AlekseiEfimov · 2022-01-12T13:16:06Z

test/jdk/com/sun/jndi/ldap/LdapPoolTimeoutTest.java

+ * @library /test/lib
+ *          lib/
+ * @run testng/othervm LdapPoolTimeoutTest
+ * @bug JDK-8277795


I believe the JDK- prefix is not needed in @ bug tag value.
Tags can be also reordered to follow the recommendations here.

msheppar · 2022-01-13T01:02:38Z

src/java.naming/share/classes/com/sun/jndi/ldap/pool/PooledConnectionFactory.java

+     * connection from the pool.
+     * @param timeout the connection timeout
+     */
+    public abstract PooledConnection createPooledConnection(PoolCallback pcb, long timeout)


why not use int timeout to be consistent with existing code ?
You've been required to "squash" it into an int in the factory ?

IIRC this was a request from an earlier review. (long being the standard throughout other new public apis) I'm happy with either, but int does avoid the trouble of casting.

Well I guess the request was "why not use long everywhere to avoid casting to int" ;-)
But I'm happy with either too - as long as the place where you have a long (e.g obtained by substracting two nano times) and call a method that takes an int has the proper guards in place, and either assert/throws/floor or ceil if the assumptions are not met - provided that a comment explains why that particular alternative is selected.

Good point. I think its better to deal with the casts at the edges since the timeout handling will use long by default.

yes a redeclaration of timeout with a type long across the component would be a consistent approach, also

so just to clarify, we're not taking the approach to homogenise the timeout declarations, throughout the component, to be of type long?

which would see LdapClientFactory constructor take a long timeout and timeout member varaiables be redefined as long

Not in this issue. I plan to file a follow up bug to make a slight change to the test. I would like to get this issue fixed ASAP and would appreciate the time to take a good look at the transition to a long timeout. (i.e. I'll handle it in that follow up issue)

msheppar · 2022-01-13T01:07:52Z

src/java.naming/share/classes/com/sun/jndi/ldap/LdapClientFactory.java

@@ -65,6 +65,13 @@ public PooledConnection createPooledConnection(PoolCallback pcb)
                connTimeout, readTimeout, trace, pcb);
    }

+    public PooledConnection createPooledConnection(PoolCallback pcb, long timeout)
+        throws NamingException {
+        return new LdapClient(host, port, socketFactory,


any need to perform sanity check against erroneous negative values on the timeout supplied here and in other parts of the solution

Hmmm... Good point. I had looked into this yesterday when I reviewed - and AFAIU a value <= 0 would be interpreted as no timeout (that is, infinite timeout) - and that seems consistent throughout. It's non obvious - but I convinced myself that passing a negative value here would not necessarily be an error, and would work as expected. However the narrowing down of a negative long to an int doesn't necessarily preserve the sign.
@robm-openjdk the conversion from long to int probably needs to also take care of values that are < Integer.MIN_VALUE.

jshell> long l = Integer.MIN_VALUE * 2L l ==> -4294967296 jshell> int x = (int)l x ==> 0 jshell> long l = Integer.MIN_VALUE * 2L + 1 l ==> -4294967295 jshell> int x = (int)l x ==> 1

(Though I don't think it can happen - but maybe I'm mistaken)
In any case it's safer to sanitize the input.

robm-openjdk · 2022-02-04T13:06:12Z

/integrate

openjdk · 2022-02-04T13:07:15Z

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

51b53a8: 8280913: Create a regression test for JRootPane.setDefaultButton() method
46c6c6f: 8281043: Intrinsify recursive ObjectMonitor locking for PPC64
c936e70: 8280593: [PPC64, S390] redundant allocation of MacroAssembler in StubGenerator ctor
63e11cf: 8280970: Cleanup dead code in java.security.Provider
e44dc63: 8271055: Crash during deoptimization with "assert(bb->is_reachable()) failed: getting result from unreachable basicblock" with -XX:+VerifyStack
b6935df: 8251505: Use of types in compiler shared code should be consistent.
130cf46: 4750574: (se spec) Selector spec should clarify calculation of select return value
cda9c30: 8278753: Runtime crashes with access violation during JNI_CreateJavaVM call
86c24b3: 8240908: RetransformClass does not know about MethodParameters attribute
1f92660: 8281057: Fix doc references to overriding in JLS
... and 153 more: https://git.openjdk.java.net/jdk/compare/ab2c8d3c9baf1080f436287785e4e02fd79953a7...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-04T13:07:28Z

@robm-openjdk Pushed as commit 3d926dd.

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

robm-openjdk · 2022-02-07T18:10:49Z

/backport jdk18u

openjdk · 2022-02-07T18:11:47Z

@robm-openjdk Unknown command backport - for a list of valid commands use /help.

openjdk bot added the core-libs core-libs-dev@openjdk.org label Nov 25, 2021

robm-openjdk marked this pull request as ready for review November 26, 2021 00:10

openjdk bot added the rfr Pull request is ready for review label Nov 26, 2021

dfuch reviewed Nov 26, 2021

View reviewed changes

openjdk bot removed the rfr Pull request is ready for review label Nov 26, 2021

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

dfuch reviewed Jan 11, 2022

View reviewed changes

dfuch approved these changes Jan 11, 2022

View reviewed changes

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

AlekseiEfimov reviewed Jan 12, 2022

View reviewed changes

msheppar reviewed Jan 13, 2022

View reviewed changes

robm-openjdk added 7 commits January 21, 2022 12:59

JDK-8277795: ldap connection timeout not honoured under contention

e9da70f

JDK-8277795: whitespace cleanup

903266a

JDK-8277795: formatting

459e889

Add test

e0aae93

JDK-8277795: whitespace

9e9f747

Allow the test to pass on MacOSX

b5ca4d2

Add method to guard long to int cast

6af94f3

robm-openjdk force-pushed the JDK-8277795 branch from 7c27762 to 6af94f3 Compare January 21, 2022 13:00

dfuch approved these changes Jan 21, 2022

View reviewed changes

AlekseiEfimov approved these changes Jan 22, 2022

View reviewed changes

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration labels Jan 27, 2022

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

openjdk bot added the integrated Pull request has been integrated label Feb 4, 2022

openjdk bot closed this Feb 4, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8277795: ldap connection timeout not honoured under contention #6568

JDK-8277795: ldap connection timeout not honoured under contention #6568

robm-openjdk commented Nov 25, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 25, 2021

openjdk bot commented Nov 25, 2021

mlbridge bot commented Nov 26, 2021 •

edited

Loading

dfuch left a comment

dfuch Nov 26, 2021

robm-openjdk Nov 26, 2021

dfuch Nov 26, 2021

robm-openjdk Nov 26, 2021

robm-openjdk commented Nov 27, 2021

dfuch Jan 11, 2022

openjdk bot commented Jan 11, 2022 •

edited

Loading

AlekseiEfimov left a comment

AlekseiEfimov Jan 12, 2022

msheppar Jan 13, 2022

robm-openjdk Jan 19, 2022 •

edited

Loading

dfuch Jan 19, 2022

robm-openjdk Jan 21, 2022

msheppar Jan 21, 2022

msheppar Jan 25, 2022

robm-openjdk Jan 27, 2022 •

edited

Loading

msheppar Jan 13, 2022

dfuch Jan 13, 2022

dfuch Jan 13, 2022

robm-openjdk commented Feb 4, 2022

openjdk bot commented Feb 4, 2022

openjdk bot commented Feb 4, 2022

robm-openjdk commented Feb 7, 2022

openjdk bot commented Feb 7, 2022

JDK-8277795: ldap connection timeout not honoured under contention #6568

JDK-8277795: ldap connection timeout not honoured under contention #6568

Conversation

robm-openjdk commented Nov 25, 2021 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Nov 25, 2021

openjdk bot commented Nov 25, 2021

mlbridge bot commented Nov 26, 2021 • edited Loading

Webrevs

dfuch left a 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

robm-openjdk commented Nov 27, 2021

Choose a reason for hiding this comment

openjdk bot commented Jan 11, 2022 • edited Loading

AlekseiEfimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robm-openjdk Jan 19, 2022 • 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

robm-openjdk Jan 27, 2022 • 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

robm-openjdk commented Feb 4, 2022

openjdk bot commented Feb 4, 2022

openjdk bot commented Feb 4, 2022

robm-openjdk commented Feb 7, 2022

openjdk bot commented Feb 7, 2022

robm-openjdk commented Nov 25, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 26, 2021 •

edited

Loading

openjdk bot commented Jan 11, 2022 •

edited

Loading

robm-openjdk Jan 19, 2022 •

edited

Loading

robm-openjdk Jan 27, 2022 •

edited

Loading