Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed" #2749

Closed
wants to merge 4 commits into from

Conversation

@evwhelan
Copy link
Contributor

@evwhelan evwhelan commented Feb 26, 2021

Hi all,

Please review my test fix relating to JDK-8262438

This patch introduces as Thread.sleep at the start of each iteration which creates a new test jvm.
This allows the server socket sufficient time to release the previous connection and allows the port to be used again.

I also refactored the behaviour for when the exitCode is not 0, allowing for an easier to read output.
An incorrect HttpsUrlConnection.disconnect() was also removed.

The test was ran 100 times on all platforms and no failures were seen.

Thanks,
Evan


Progress

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

Issue

  • JDK-8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed"

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2749/head:pull/2749
$ git checkout pull/2749

…led with "SocketException: Socket is closed"
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 26, 2021

👋 Welcome back ewhelan! 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 openjdk bot added the rfr label Feb 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 26, 2021

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

  • security

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.

@openjdk openjdk bot added the security label Feb 26, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 26, 2021

Webrevs

@dfuch
Copy link
Member

@dfuch dfuch commented Feb 26, 2021

Hi Evan - I am a bit skeptical that the proposed fix will solve the issue. AFAICS the exception is raised by the server side - and if I read it correctly it happens when the server finds that the socket is already closed when it tries to close it. How could sleeping between 2 process invocation solve this? The port used is an ephemeral port - so it should never be the same?
Does the log show that the same port was being reused?

@evwhelan
Copy link
Contributor Author

@evwhelan evwhelan commented Mar 2, 2021

Hi Daniel,

From speaking with you offline, I have ensured that the server-side and client-side socket input streams are emptied.

I have also added some debugging info should this test fail in the future.

I've removed the proposed Thread.sleep and the test does not fail in 400 runs

Regards,
Evan

out.writeBytes("HTTP/1.0 400 " + e.getMessage() + "\r\n");
out.writeBytes("Content-Type: text/html\r\n\r\n");
out.flush();
} finally {
socket.getInputStream().readAllBytes();
Copy link
Member

@dfuch dfuch Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause the server side to block until the client closes the socket. Is that what you really want to do? (It may be - but if the client is a regular HTTP client (HttpURLConnection / HttpClient) it will not close the connection until its keep-alive delay (may be up to 20mins) is expired.

Copy link
Contributor Author

@evwhelan evwhelan Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR by removing the custom server logic, more detail can be found in my latest comment.

Evan :)

@evwhelan
Copy link
Contributor Author

@evwhelan evwhelan commented Mar 4, 2021

Hi Daniel,

So in testing, I realised that adding my own custom logic to handle the client-server communication was a bit overkill.

Simply invoking the SSLSocketTemplate::run method is sufficient in making the basic connection and generating the desired debug info which the test checks for.

After 400 runs I saw no failures

Thanks,
Evan

Copy link
Member

@dfuch dfuch left a comment

I don't see any specific issue with what you are proposing from a networking perspective, but you will need someone from security-libs to review the proposed changes and acknowledge that the test is still testing what it intends to test.

@robm-openjdk
Copy link
Member

@robm-openjdk robm-openjdk commented Mar 5, 2021

The changes here don't affect the original fix given that the default impl of the client & server work for the purposes of the test.

@rhalade
Copy link
Member

@rhalade rhalade commented Mar 8, 2021

Can you please confirm that you saw all protocols ("TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3") negotiated with updated test run?

@evwhelan
Copy link
Contributor Author

@evwhelan evwhelan commented Mar 9, 2021

Yes, the updated test run successfully tested with all of the protocols listed.

rhalade
rhalade approved these changes Mar 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 9, 2021

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

8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed"

Reviewed-by: rhalade

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

  • fdd3941: 8263233: Update java.net and java.nio to use instanceof pattern variable
  • 3fe8a46: 8263170: ComboBoxModel documentation refers to a nonexistent type
  • d8a9c3c: 8263002: Remove CDS MiscCode region
  • 67ea3bd: 8263102: Expand documention of Method.isBridge
  • d0c1aec: 8263140: Japanese chars garble in console window in HSDB
  • 70342e8: 8262520: Add SA Command Line Debugger support to connect to debug server
  • e5ce97b: 8263206: assert(*error_msg != '\0') failed: Must have error_message while parsing -XX:CompileCommand=unknown
  • 3212f80: 8261937: LambdaForClassInBaseArchive: SimpleApp$$Lambda$1 missing
  • 2218e72: 8262486: Merge trivial JDWP agent changes from the loom repo to the jdk repo
  • 86fac95: 8263142: Delete unused entry points in libawt/libawt_xawt/libawt_headless
  • ... and 138 more: https://git.openjdk.java.net/jdk/compare/0a4e710ff600d001c0464f5b7bb5d3a2cd461c06...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 (@dfuch, @rhalade) 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).

@openjdk openjdk bot added the ready label Mar 9, 2021
@evwhelan
Copy link
Contributor Author

@evwhelan evwhelan commented Mar 10, 2021

/integrate

@openjdk openjdk bot added the sponsor label Mar 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

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

@robm-openjdk
Copy link
Member

@robm-openjdk robm-openjdk commented Mar 10, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@robm-openjdk @evwhelan Since your change was applied there have been 152 commits pushed to the master branch:

  • c8c0234: 8262471: Fix coding style in src/java.base/share/classes/java/lang/CharacterDataPrivateUse.java
  • 4d21a45: 8262913: KlassFactory::create_from_stream should never return NULL
  • fab5676: 8247869: Change NONCOPYABLE to delete the operations
  • c0542ed: 6251901: BasicTextUI: installDefaults method are contrary to the documentation
  • fdd3941: 8263233: Update java.net and java.nio to use instanceof pattern variable
  • 3fe8a46: 8263170: ComboBoxModel documentation refers to a nonexistent type
  • d8a9c3c: 8263002: Remove CDS MiscCode region
  • 67ea3bd: 8263102: Expand documention of Method.isBridge
  • d0c1aec: 8263140: Japanese chars garble in console window in HSDB
  • 70342e8: 8262520: Add SA Command Line Debugger support to connect to debug server
  • ... and 142 more: https://git.openjdk.java.net/jdk/compare/0a4e710ff600d001c0464f5b7bb5d3a2cd461c06...master

Your commit was automatically rebased without conflicts.

Pushed as commit b2a2ddf.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants