8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors #15143

weibxiao · 2023-08-03T17:32:43Z

com.sun.jndi.ldap.Connection::leanup does not close the underlying socket if the is an IOException generation when the output stream was flushing the buffer.

Please refer to the bug https://bugs.openjdk.org/browse/JDK-8313657.

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

Issue

JDK-8313657: com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors (Bug - P3)

Reviewers

Vyom Tewari (@vyommani - Committer) ⚠️ Review applies to e3e98226
Mark Sheppard (@msheppar - Reviewer)
Aleksei Efimov (@AlekseiEfimov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15143

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

Using diff file

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

Webrev

Link to Webrev Comment

…ions on SocketTimeoutErrors

bridgekeeper · 2023-08-03T17:33:28Z

👋 Welcome back weibxiao! 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 · 2023-08-03T17:34:46Z

@weibxiao 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.

AlanBateman · 2023-08-03T17:59:10Z

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

+                            sock.close();
+                            unpauseReader();
+                            if (debug) {
+                                if (sock.isClosed()) {


How is it possible for sock.close() to succeed but isClosed to return false?

It is an unreachable code and got removed.

mlbridge · 2023-08-04T13:07:01Z

Webrevs

msheppar · 2023-08-04T15:20:32Z

why not restructure the finally block a little ... refactor extract methods
flushOutputStream,
closeConnectionSocket
... should the outputStream be closed in this finally block, also?

            } finally {
                flushOutputStream(outStream);
                closeConnectionSocket(sock);
                try {
                    unpauseReader();
                } catch (IOException ie) {
                    if (debug)
                        System.err.println("Connection.cleanup: problem with unpuaseReader " + ie);
                }

...

private void flushOutputStream (OutputStream outputStream) {
    try {
        outStream.flush();
        outStream.close();
    } catch (IOException ioEx) {
        if (debug)
            System.err.println("Connection.flushOutputStream: OutputStream flush or close problem " + ioEx);
    }
}

private void closeConnectionSocket (Socket sock) {
    //bug 8313657, socket not closed util GC running
    try {
        sock.close();
    } catch (IOException ioEx) {
        if (debug)
            System.err.println("ConnectioncloseConnectioSocket: problem closing socket: " + ioEx);
    }
}

AlekseiEfimov · 2023-08-04T15:31:58Z

test/jdk/javax/naming/InitialContext/SocketCloseTest.java

+import javax.naming.directory.DirContext;
+import javax.naming.directory.InitialDirContext;
+import javax.net.SocketFactory;
+import java.io.*;


Can we please use a set of class imports instead of package import here:

import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream;

AlekseiEfimov · 2023-08-04T15:42:19Z

test/jdk/javax/naming/InitialContext/SocketCloseTest.java

+ * @library /test/lib
+ */
+
+public class SocketCloseTest {


Can we change the test location from test/javax/naming/InitialContext to an LDAP-implementation specific folder - test/jdk/com/sun/jndi/ldap

AlekseiEfimov · 2023-08-04T15:45:41Z

test/jdk/javax/naming/InitialContext/SocketCloseTest.java

+            48, 12, 2, 1, 1, 97, 7, 10, 1, 0, 4, 0, 4, 0
+    };
+    private static final int SEARCH_SIZE = 87;
+    private static final byte[] SEARCH_RESPONSE = new byte[]{


SEARCH_RESPONSE, SEARCH_SIZE and BIND_SIZE are not used in the test. If they're not needed they can be removed.

AlekseiEfimov · 2023-08-04T15:49:59Z

test/jdk/javax/naming/InitialContext/SocketCloseTest.java

+        Hashtable<String, Object> props = new Hashtable<>();
+
+        props.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
+        props.put(Context.PROVIDER_URL, "ldap://localhost:1389/o=example");


For future test maintainers - can we please clarify that the hostname and port do not matter here because the provided custom socket factory doesn't establish a connection to the specified provider URL

AlekseiEfimov · 2023-08-04T16:28:48Z

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

@@ -650,6 +650,19 @@ void cleanup(Control[] reqCtls, boolean notifyParent) {
                    } catch (IOException ie) {
                        if (debug)
                            System.err.println("Connection: problem closing socket: " + ie);


How about combining the debug statement here with the one below and just print the socket isClosed state, something like

if (debug) { System.err.println("Connection: problem cleaning-up the connection: " + ie); System.err.println("Socket isClosed: " + sock.isClosed()); }

The text was changed here since the exception can also be thrown by unpauseReader

AlekseiEfimov · 2023-08-04T16:30:18Z

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

+                            }
+                        } catch (IOException ioe) {
+                            if (debug)
+                                System.err.println("Connection::cleanup problem: " + ioe);


Maybe change the message here to highlight the fact that it's the 2nd attemp to clean-up the connection

weibxiao · 2023-08-04T19:05:08Z

Extracted the methods for finally black of Connection::cleanUP
Move the test class to LDAP test folder and updated the test comments.
Removed the redundant code and update the import statement.

vyommani · 2023-08-07T06:40:49Z

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

@@ -684,6 +682,43 @@ void cleanup(Control[] reqCtls, boolean notifyParent) {
        }
    }

+    // flush and close output stream
+    private void flushCloseOutputStream() {


'flushAndCloseOutputStream' will be more meaningful name.

vyommani · 2023-08-07T06:42:54Z

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

+    }
+
+    // close socket
+    private void closeConnectionSocket() {


'closeSocketConnection' will be more meaningful.

the suggested name was to convey the closing of the Connection's socket, not a socket connection per se.

msheppar · 2023-08-07T09:52:16Z

I ran the test multiple times in mach5 and it executes fine. However, while examining the log output I noticed the "The socket was not closed. " output. This is a little confusing. While looking at the test and exploring the origins of this outout, we see that the test is essentially an othervm execution. As such, I restructured the test to othervm and it executes with some informative output and eliminates the discombobulating output.

I have left a comment in the JBS bug item with some further details and changes for othervm. This gives some imformative output as the test executes.

The origins of the output is the main try block executing in the agentvm and the throwing of a ClassNotFoundException.

If you don't wish to do otherrvm and to avoid the "The socket was not closed. " output, and if wish to retain the same structure, then consider adding a run command to the test with an arg, and then a conditional on the core test logic e.g.

@run main SocketCloseTest launcher

and in the main method

public static void main(String[] args) throws Exception {
    Hashtable<String, Object> props = new Hashtable<>();

if (args.length == 0) { // only executed in the launched JVM
props.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
props.put(Context.PROVIDER_URL, "ldap://localhost:1389/o=example");
props.put("java.naming.ldap.factory.socket", CustomSocketFactory.class.getName());
try {
final DirContext ctx = new InitialDirContext(props);
} catch (Exception e) {
if (CustomSocketFactory.customSocket.closeMethodCalledCount() > 0) {
System.out.println(SOCKET_CLOSED_MSG);
} else {
System.out.println(SOCKET_NOT_CLOSED_MSG);
}
}
}

    OutputAnalyzer outputAnalyzer = ProcessTools.executeTestJvm("SocketCloseTest_II");
    outputAnalyzer.stdoutShouldContain(SOCKET_CLOSED_MSG);
    outputAnalyzer.stdoutShouldNotContain(SOCKET_NOT_CLOSED_MSG);
    outputAnalyzer.stdoutShouldContain(BAD_FLUSH);
}

msheppar · 2023-08-07T10:18:31Z

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

+        props.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
+        props.put(Context.PROVIDER_URL, "ldap://localhost:1389/o=example");
+        props.put("java.naming.ldap.factory.socket", CustomSocketFactory.class.getName());
+        try {


this will execute twice: once in the agentvm main and also, as desired, in the test JVM main.
The agentvm main sees a ClassNotFoundException being thrown and then the output of "The socket was not closed. " in the log c.f. other comments and bug comments for further details to avoid execution in agentvm or use of othervm test mode execution

weibxiao · 2023-08-09T00:17:24Z

Update the name of the methods
Update the test file.

vyommani · 2023-08-09T04:06:57Z

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

+    public static class LdapInputStream extends InputStream {
+        private LdapOutputStream los;
+        private ByteArrayInputStream bos;
+        int pos = 0;


variable 'pos' is not used can be removed.

vyommani · 2023-08-09T04:08:20Z

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

+        @Override
+        public int read() throws IOException {
+            bos = new ByteArrayInputStream(BIND_RESPONSE);
+            int next = bos.read();


can be simplified as follows return bos.read();

vyommani · 2023-08-09T04:59:56Z

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

+    }
+
+    public static class CustomSocketFactory extends SocketFactory {
+        public static CustomSocket customSocket = new CustomSocket();


you can make 'customSocket' to private.

weibxiao · 2023-08-09T12:24:45Z

Cannot make CustomSocket be private. The test failed. I believe JDK is using the reflection to load the class.
The rest of the changes are based on the comments.

vyommani

Latest code looks OK to me.

AlekseiEfimov · 2023-08-11T18:46:47Z

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

+    }
+
+    private static class LdapInputStream extends InputStream {
+        private LdapOutputStream los;


The LdapOutputStream reference is not used by LdapInputStream, therefore the los field can be removed:

private static class LdapInputStream extends InputStream { - private LdapOutputStream los; private ByteArrayInputStream bos; - public LdapInputStream(LdapOutputStream los) { - this.los = los; + public LdapInputStream() { } @Override @@ -144,7 +142,7 @@ public class SocketCloseTest { private static class CustomSocket extends Socket { private int closeMethodCalled = 0; private LdapOutputStream output = new LdapOutputStream(); - private LdapInputStream input = new LdapInputStream(output); + private LdapInputStream input = new LdapInputStream(); public void connect(SocketAddress address, int timeout) { }

weibxiao · 2023-08-14T13:02:54Z

Updated LdapInputStream.java and remove the unused variable.

msheppar

thanks for incorporating the feedback
LGTM

openjdk · 2023-08-14T16:56:34Z

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

8313657: com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors

Reviewed-by: vtewari, msheppar, 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 107 new commits pushed to the master branch:

4b2703a: 8313678: SymbolTable can leak Symbols during cleanup
f41c267: 8314045: ArithmeticException in GaloisCounterMode
911d1db: 8314078: HotSpotConstantPool.lookupField() asserts due to field changes in ConstantPool.cpp
6574dd7: 8314025: Remove JUnit-based test in java/lang/invoke from problem list
207bd00: 8313756: [BACKOUT] 8308682: Enhance AES performance
823f5b9: 8308850: Change JVM options with small ranges that get -Wconversion warnings to 32 bits
5bfb82e: 8314119: G1: Fix -Wconversion warnings in G1CardSetInlinePtr::card_pos_for
06aa3c5: 8314118: Update JMH devkit to 1.37
4164693: 8313372: [JVMCI] Export vmIntrinsics::is_intrinsic_available results to JVMCI compilers.
049b55f: 8314019: Add gc logging to jdk/jfr/event/gc/detailed/TestZAllocationStallEvent.java
... and 97 more: https://git.openjdk.org/jdk/compare/ab1c212ac1097ae6e1122ef1aba47ca51eca11f2...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vyommani, @msheppar, @AlekseiEfimov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

weibxiao · 2023-08-14T17:10:44Z

/integrate

openjdk · 2023-08-14T17:12:13Z

@weibxiao
Your change (at version b7d0c25) is now ready to be sponsored by a Committer.

AlekseiEfimov · 2023-08-14T17:38:00Z

/sponsor

openjdk · 2023-08-14T17:38:56Z

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

4b2703a: 8313678: SymbolTable can leak Symbols during cleanup
f41c267: 8314045: ArithmeticException in GaloisCounterMode
911d1db: 8314078: HotSpotConstantPool.lookupField() asserts due to field changes in ConstantPool.cpp
6574dd7: 8314025: Remove JUnit-based test in java/lang/invoke from problem list
207bd00: 8313756: [BACKOUT] 8308682: Enhance AES performance
823f5b9: 8308850: Change JVM options with small ranges that get -Wconversion warnings to 32 bits
5bfb82e: 8314119: G1: Fix -Wconversion warnings in G1CardSetInlinePtr::card_pos_for
06aa3c5: 8314118: Update JMH devkit to 1.37
4164693: 8313372: [JVMCI] Export vmIntrinsics::is_intrinsic_available results to JVMCI compilers.
049b55f: 8314019: Add gc logging to jdk/jfr/event/gc/detailed/TestZAllocationStallEvent.java
... and 97 more: https://git.openjdk.org/jdk/compare/ab1c212ac1097ae6e1122ef1aba47ca51eca11f2...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-14T17:39:03Z

@AlekseiEfimov @weibxiao Pushed as commit e56d3bc.

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

8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connect…

03ae313

…ions on SocketTimeoutErrors

openjdk bot added the core-libs core-libs-dev@openjdk.org label Aug 3, 2023

AlanBateman reviewed Aug 3, 2023

View reviewed changes

weibxiao changed the title ~~8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connect…~~ 8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors Aug 3, 2023

weibxiao added 2 commits August 3, 2023 15:31

remove unreachable code and reformatting the code

da30787

reformat the code

c7feb93

openjdk bot added the rfr Pull request is ready for review label Aug 4, 2023

AlekseiEfimov reviewed Aug 4, 2023

View reviewed changes

weibxiao added 2 commits August 4, 2023 14:17

updated the code according the review comments

abf8126

update error message

f33a565

vyommani reviewed Aug 7, 2023

View reviewed changes

msheppar reviewed Aug 7, 2023

View reviewed changes

update method name and test case

b46537b

vyommani reviewed Aug 9, 2023

View reviewed changes

class access modifier

e3e9822

vyommani approved these changes Aug 9, 2023

View reviewed changes

AlekseiEfimov reviewed Aug 11, 2023

View reviewed changes

removed the unused variable

b7d0c25

msheppar approved these changes Aug 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 14, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 14, 2023

AlekseiEfimov approved these changes Aug 14, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 14, 2023

openjdk bot closed this Aug 14, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors #15143

8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors #15143

weibxiao commented Aug 3, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 3, 2023

openjdk bot commented Aug 3, 2023

AlanBateman Aug 3, 2023

weibxiao Aug 3, 2023

mlbridge bot commented Aug 4, 2023 •

edited

Loading

msheppar commented Aug 4, 2023

AlekseiEfimov Aug 4, 2023

AlekseiEfimov Aug 4, 2023

AlekseiEfimov Aug 4, 2023

AlekseiEfimov Aug 4, 2023

AlekseiEfimov Aug 4, 2023

AlekseiEfimov Aug 4, 2023

weibxiao commented Aug 4, 2023

vyommani Aug 7, 2023

vyommani Aug 7, 2023

msheppar Aug 7, 2023

msheppar commented Aug 7, 2023

msheppar Aug 7, 2023

weibxiao commented Aug 9, 2023

vyommani Aug 9, 2023

vyommani Aug 9, 2023

vyommani Aug 9, 2023

weibxiao commented Aug 9, 2023

vyommani left a comment

AlekseiEfimov Aug 11, 2023

weibxiao commented Aug 14, 2023

msheppar left a comment

openjdk bot commented Aug 14, 2023 •

edited

Loading

weibxiao commented Aug 14, 2023

openjdk bot commented Aug 14, 2023

AlekseiEfimov commented Aug 14, 2023

openjdk bot commented Aug 14, 2023

openjdk bot commented Aug 14, 2023

8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors #15143

8313657 : com.sun.jndi.ldap.Connection.cleanup does not close connections on SocketTimeoutErrors #15143

Conversation

weibxiao commented Aug 3, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 3, 2023

openjdk bot commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Aug 4, 2023 • edited Loading

Webrevs

msheppar commented Aug 4, 2023

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

weibxiao commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msheppar commented Aug 7, 2023

Choose a reason for hiding this comment

weibxiao commented Aug 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weibxiao commented Aug 9, 2023

vyommani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weibxiao commented Aug 14, 2023

msheppar left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 14, 2023 • edited Loading

weibxiao commented Aug 14, 2023

openjdk bot commented Aug 14, 2023

AlekseiEfimov commented Aug 14, 2023

openjdk bot commented Aug 14, 2023

openjdk bot commented Aug 14, 2023

weibxiao commented Aug 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 4, 2023 •

edited

Loading

openjdk bot commented Aug 14, 2023 •

edited

Loading