Integrated: 8255148: Confusing log output: SSLSocket duplex close failed
On Fri, 4 Jun 2021 10:44:32 GMT, Evan Whelan wrote: > Hi, > > Please review my fix for JDK-8255148 which clarifies when an exception > contains debug information only. > > Regards, > Evan This pull request has now been integrated. Changeset: 408e0a9c Author:Evan Whelan Committer: Sean Mullan URL: https://git.openjdk.java.net/jdk/commit/408e0a9c696888d41809e35bf252869f09f735db Stats: 106 lines in 2 files changed: 102 ins; 0 del; 4 mod 8255148: Confusing log output: SSLSocket duplex close failed Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/4354
Re: RFR: 8255148: Confusing log output: SSLSocket duplex close failed [v2]
On Wed, 9 Jun 2021 20:27:41 GMT, Sean Mullan wrote: >> Hi @seanjmullan @coffeys >> >> I also don't have a logging level preference, so if there's merit to >> changing it, I'll be happy to do so. >> I've updated the test case and verified it passes on all platforms. >> >> Looking forward to any further feedback :) > > Ok, It sounds like whether it should be a different logging level (fine) or > not can be handled separately as part of a more global effort across other > SSL logging messages. So I am ok with the change as-is. Thanks Sean! - PR: https://git.openjdk.java.net/jdk/pull/4354
Re: RFR: 8255148: Confusing log output: SSLSocket duplex close failed [v2]
On Fri, 4 Jun 2021 14:08:58 GMT, Sean Mullan wrote:
>> Evan Whelan has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Added new line at end of file
>> - Cleaned up test case
>
> src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 590:
>
>> 588: // ignore the exception
>> 589: if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
>> 590: SSLLogger.warning("SSLSocket duplex close failed. Debug
>> info only. Exception details:", ioe);
>
> If this is a debug message, shouldn't we just use `SSLLogger.fine()` instead
> of `SSLLogger.warning()`, with the same message "SSLSocket duplex close
> failed"? @coffeys what do you think?
Hi @seanjmullan @coffeys
I also don't have a logging level preference, so if there's merit to changing
it, I'll be happy to do so.
I've updated the test case and verified it passes on all platforms.
Looking forward to any further feedback :)
-
PR: https://git.openjdk.java.net/jdk/pull/4354
Re: RFR: 8255148: Confusing log output: SSLSocket duplex close failed [v2]
> Hi, > > Please review my fix for JDK-8255148 which clarifies when an exception > contains debug information only. > > Regards, > Evan Evan Whelan has updated the pull request incrementally with two additional commits since the last revision: - Added new line at end of file - Cleaned up test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/4354/files - new: https://git.openjdk.java.net/jdk/pull/4354/files/8bf9b687..c4ad5abb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4354&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4354&range=00-01 Stats: 20 lines in 1 file changed: 5 ins; 7 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/4354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4354/head:pull/4354 PR: https://git.openjdk.java.net/jdk/pull/4354
RFR: 8255148: Confusing log output: SSLSocket duplex close failed
Hi, Please review my fix for JDK-8255148 which clarifies when an exception contains debug information only. Regards, Evan - Commit messages: - Remove othervm mode - 8255148: Confusing log output: SSLSocket duplex close failed Changes: https://git.openjdk.java.net/jdk/pull/4354/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4354&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255148 Stats: 108 lines in 2 files changed: 104 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4354.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4354/head:pull/4354 PR: https://git.openjdk.java.net/jdk/pull/4354
Integrated: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed"
On Fri, 26 Feb 2021 14:28:23 GMT, Evan Whelan wrote: > 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 This pull request has now been integrated. Changeset: b2a2ddff Author:Evan Whelan Committer: Rob McKenna URL: https://git.openjdk.java.net/jdk/commit/b2a2ddff Stats: 68 lines in 1 file changed: 6 ins; 61 del; 1 mod 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed" Reviewed-by: rhalade - PR: https://git.openjdk.java.net/jdk/pull/2749
Re: RFR: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed" [v3]
On Mon, 8 Mar 2021 19:25:09 GMT, Rajan Halade wrote:
>> 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.
>
> Can you please confirm that you saw all protocols ("TLSv1", "TLSv1.1",
> "TLSv1.2", "TLSv1.3") negotiated with updated test run?
Yes, the updated test run successfully tested with all of the protocols listed.
-
PR: https://git.openjdk.java.net/jdk/pull/2749
Re: RFR: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed"
On Tue, 2 Mar 2021 15:04:23 GMT, Evan Whelan wrote: >> 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? > > 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 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 - PR: https://git.openjdk.java.net/jdk/pull/2749
Re: RFR: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed" [v2]
On Tue, 2 Mar 2021 15:13:56 GMT, Daniel Fuchs wrote:
>> Evan Whelan has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8262438: Stream operations on new lines
>> - 8262438: Ensure all streams are emptied in socket connection
>
> test/jdk/sun/security/ssl/SSLLogger/LoggingFormatConsistency.java line 133:
>
>> 131: out.flush();
>> 132: } finally {
>> 133: socket.getInputStream().readAllBytes();
>
> 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.
I've updated the PR by removing the custom server logic, more detail can be
found in my latest comment.
Evan :)
-
PR: https://git.openjdk.java.net/jdk/pull/2749
Re: RFR: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed" [v3]
> 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 Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: 8262438: Removed custom socket handling logic - Changes: - all: https://git.openjdk.java.net/jdk/pull/2749/files - new: https://git.openjdk.java.net/jdk/pull/2749/files/ae1acb21..e143fee1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2749&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2749&range=01-02 Stats: 68 lines in 1 file changed: 0 ins; 68 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2749.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2749/head:pull/2749 PR: https://git.openjdk.java.net/jdk/pull/2749
Re: RFR: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed"
On Fri, 26 Feb 2021 17:17:09 GMT, Daniel Fuchs wrote: >> 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 > > 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? 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 - PR: https://git.openjdk.java.net/jdk/pull/2749
Re: RFR: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed" [v2]
> 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 Evan Whelan has updated the pull request incrementally with two additional commits since the last revision: - 8262438: Stream operations on new lines - 8262438: Ensure all streams are emptied in socket connection - Changes: - all: https://git.openjdk.java.net/jdk/pull/2749/files - new: https://git.openjdk.java.net/jdk/pull/2749/files/363466f8..ae1acb21 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2749&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2749&range=00-01 Stats: 22 lines in 1 file changed: 12 ins; 4 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2749.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2749/head:pull/2749 PR: https://git.openjdk.java.net/jdk/pull/2749
RFR: 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed"
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 - Commit messages: - 8262438: sun/security/ssl/SSLLogger/LoggingFormatConsistency.java failed with "SocketException: Socket is closed" Changes: https://git.openjdk.java.net/jdk/pull/2749/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2749&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8262438 Stats: 11 lines in 1 file changed: 5 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2749.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2749/head:pull/2749 PR: https://git.openjdk.java.net/jdk/pull/2749
Integrated: 8211227: Inconsistent TLS protocol version in debug output
On Mon, 1 Feb 2021 10:37:35 GMT, Evan Whelan wrote: > Hi everyone, > > Please review my fix for JDK-8211227 > > This supportability fix will result in a more consistent debug format when > reading and writing TLS protocol versions. > > Thanks, > Evan This pull request has now been integrated. Changeset: a8672885 Author:Evan Whelan Committer: Rajan Halade URL: https://git.openjdk.java.net/jdk/commit/a8672885 Stats: 162 lines in 6 files changed: 151 ins; 0 del; 11 mod 8211227: Inconsistent TLS protocol version in debug output Reviewed-by: xuelei, rhalade - PR: https://git.openjdk.java.net/jdk/pull/2331
Re: RFR: 8211227: Inconsistent TLS protocol version in debug output [v3]
On Mon, 22 Feb 2021 18:44:15 GMT, Rajan Halade wrote: >> Evan Whelan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8211227: Re-wrote LoggingFormatConsistency to leverage SSLSocketTemplate > > Thanks for developing test for this change and updating it to use template. Thanks Rajan, I appreciate the guidance. - PR: https://git.openjdk.java.net/jdk/pull/2331
Re: RFR: 8211227: Inconsistent TLS protocol version in debug output [v3]
> Hi everyone, > > Please review my fix for JDK-8211227 > > This supportability fix will result in a more consistent debug format when > reading and writing TLS protocol versions. > > Thanks, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: 8211227: Re-wrote LoggingFormatConsistency to leverage SSLSocketTemplate - Changes: - all: https://git.openjdk.java.net/jdk/pull/2331/files - new: https://git.openjdk.java.net/jdk/pull/2331/files/ff7e2eb4..bc9d87d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2331&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2331&range=01-02 Stats: 229 lines in 1 file changed: 17 ins; 166 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/2331.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2331/head:pull/2331 PR: https://git.openjdk.java.net/jdk/pull/2331
Re: RFR: 8211227: Inconsistent TLS protocol version in debug output [v2]
On Thu, 18 Feb 2021 22:24:28 GMT, Rajan Halade wrote:
>> Evan Whelan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8211227: Re-wrote LoggingFormatConsistency to use local SSL server rather
>> than an existing URL
>
> test/jdk/sun/security/ssl/SSLLogger/LoggingFormatConsistency.java line 145:
>
>> 143: * Fork off the other side, then do your work.
>> 144: */
>> 145: LoggingFormatConsistency() throws Exception {
>
> Have you looked at the templates available at
> test/jdk/javax/net/ssl/templates? Check SSLSocketTemplate.java which is using
> CountDownLatch.
I have not! I'll take a look, thanks
-
PR: https://git.openjdk.java.net/jdk/pull/2331
Re: RFR: 8211227: Inconsistent TLS protocol version in debug output [v2]
> Hi everyone, > > Please review my fix for JDK-8211227 > > This supportability fix will result in a more consistent debug format when > reading and writing TLS protocol versions. > > Thanks, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: 8211227: Re-wrote LoggingFormatConsistency to use local SSL server rather than an existing URL - Changes: - all: https://git.openjdk.java.net/jdk/pull/2331/files - new: https://git.openjdk.java.net/jdk/pull/2331/files/e8c262f2..ff7e2eb4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2331&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2331&range=00-01 Stats: 232 lines in 1 file changed: 213 ins; 5 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/2331.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2331/head:pull/2331 PR: https://git.openjdk.java.net/jdk/pull/2331
Re: RFR: 8211227: Inconsistent TLS protocol version in debug output [v2]
On Mon, 1 Feb 2021 19:41:09 GMT, Rajan Halade wrote:
>> Evan Whelan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8211227: Re-wrote LoggingFormatConsistency to use local SSL server rather
>> than an existing URL
>
> test/jdk/sun/security/ssl/SSLLogger/LoggingFormatConsistency.java line 50:
>
>> 48: public class LoggingFormatConsistency {
>> 49: public static void main(String[] args) throws Exception {
>> 50: if (args.length == 0){
>
> Please add a comment to explain when the test should be run with parameter.
Done! Thanks
> test/jdk/sun/security/ssl/SSLLogger/LoggingFormatConsistency.java line 36:
>
>> 34: /*
>> 35: * This test runs in another process so we can monitor the debug
>> 36: * results. The OutputAnalyzer must see correct debug output to return a
>
> Suggestion:
>
> * results. The OutputAnalyzer must see correct debug output to return a
Blank space removed
-
PR: https://git.openjdk.java.net/jdk/pull/2331
Re: RFR: 8211227: Inconsistent TLS protocol version in debug output
On Mon, 1 Feb 2021 19:40:07 GMT, Rajan Halade wrote:
>> Hi everyone,
>>
>> Please review my fix for JDK-8211227
>>
>> This supportability fix will result in a more consistent debug format when
>> reading and writing TLS protocol versions.
>>
>> Thanks,
>> Evan
>
> test/jdk/sun/security/ssl/SSLLogger/LoggingFormatConsistency.java line 83:
>
>> 81: // Re-enabling as test depends on these algorithms
>> 82: SecurityUtils.removeFromDisabledTlsAlgs("TLSv1", "TLSv1.1");
>> 83: var url = new URL("https://jpg-data.us.oracle.com/";);
>
> This URL is not accessible outside.
Thanks Rajan,
I'm going to change my approach for this test, I will set up a dummy server on
localhost and create my URL based off that, rather than URLs pointing to an
existing site
-
PR: https://git.openjdk.java.net/jdk/pull/2331
RFR: 8211227: Inconsistent TLS protocol version in debug output
Hi everyone, Please review my fix for JDK-8211227 This supportability fix will result in a more consistent debug format when reading and writing TLS protocol versions. Thanks, Evan - Commit messages: - 8211227: Inconsistent TLS protocol version in debug output Changes: https://git.openjdk.java.net/jdk/pull/2331/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2331&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8211227 Stats: 103 lines in 6 files changed: 92 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/2331.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2331/head:pull/2331 PR: https://git.openjdk.java.net/jdk/pull/2331
