8251188: Update LDAP tests not to use wildcard addresses #252
Conversation
|
|
|
@AlekseiEfimov The following label will be automatically applied to this pull request: 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 |
Webrevs
|
| try (var socket = serverSock.accept()) { | ||
| System.err.println("Accepted connection:" + socket); | ||
| // Give LDAP client time to fully establish the connection. | ||
| // When client is done - close the accepted socket | ||
| testDone.await(); | ||
| } catch (Exception e) { | ||
| System.err.println("Server socket. Failure to accept connection:"); | ||
| e.printStackTrace(); | ||
| } |
There was a problem hiding this comment.
I wonder if removing the while (true) loop will make the test more susceptible of failing in timeout if the server ever receives a connection request from an unexpected client (we've seen that happening in the past with networking tests). Is there anyway the server could attempt to verify that the accepted socket is from the expected client, and close it and go back to accepting if it's not? Maybe by looking at the accepted socket remote address & port?
There was a problem hiding this comment.
Thanks for the good suggestion Daniel. I will modify it to look at the remote socket's address.
There was a problem hiding this comment.
Hi Daniel,
Could you please take a look at the new version of DeadSSLLdapTimeoutTest.java with the following modifications:
DeadServerTimeoutSSLTest test was modified to use custom SocketFactory (DeadSSLSocketFactory) which tracks the first opened socket on LDAP client side. This socket is later used on server side to check if the connection initiated by test's LDAP client. If the connection was not established by LDAP client then new accept attempt is performed.
The test was never seen to fail during 200+ concurrent LDAP tests runs.
| int iteration = 0; | ||
| // Wait for socket to get opened by DeadSSLSocketFactory and connected to the test server | ||
| while (iteration++ < 20) { | ||
| if (DeadSSLSocketFactory.firstCreatedSocket.get() != null && | ||
| DeadSSLSocketFactory.firstCreatedSocket.get().isConnected()) { | ||
| break; | ||
| } | ||
| try { | ||
| TimeUnit.MILLISECONDS.sleep(50); | ||
| } catch (InterruptedException ie) { | ||
| } | ||
| } | ||
| Socket clientSideSocket = DeadSSLSocketFactory.firstCreatedSocket.get(); | ||
| System.err.printf("Got SSLSocketFactory connection after %d iterations: %s%n", | ||
| iteration, clientSideSocket); | ||
|
|
||
| if (clientSideSocket == null || !clientSideSocket.isConnected()) { | ||
| // If after 1000 ms client side connection is not opened - probably other local process | ||
| // tried to connect to the test server socket. Close current connection and retry accept. | ||
| continue; | ||
| } else { | ||
| // Check if accepted socket is connected to the LDAP client | ||
| if (acceptedSocket.getLocalPort() == clientSideSocket.getPort() && | ||
| acceptedSocket.getPort() == clientSideSocket.getLocalPort() && | ||
| acceptedSocket.getInetAddress().equals(clientSideSocket.getLocalAddress())) { |
There was a problem hiding this comment.
Oh - sorry about that - I didn't realize checking the client socket would be so complex. This is not obvious, but of course the socket on the server side could be accepted before the connect() call from the client side returns. And anyway the LDAP stack can/will create an unconnected socket and connect it later, so placing a trigger (CountDownLatch) in the custom socket factory might not work as it might trigger too early. Hence the waiting loop :-( ...
Good job here - I wish there had been a simpler way to do this!
|
@AlekseiEfimov This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: 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 90 new commits pushed to the
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.
|
|
/integrate |
openjdk
bot
added
integrated
|
@AlekseiEfimov Since your change was applied there have been 90 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit a75edc2. |
Hi,
Please help to review JDK-8251188 fix which helps to improve LDAP tests stability. The list of changes:
LDAPTestUtils.initEnvmethod that takes LDAP provider URL as a parameter.DeadServerTimeoutSSLTest.javawas also updated to fix the intermittent failures reported by JDK-8152654 and JDK-8169942.Before the fix the failure rate was 1 out of 4 runs. After the fix it was executed 400+ times alongside to other LDAP tests, and showed no failures, and therefore removed from the problem list.
Thank you,
Aleksei
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/252/head:pull/252$ git checkout pull/252