Integrated: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

On Thu, 13 Jan 2022 12:10:11 GMT, Michael McMahon  wrote:

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

This pull request has now been integrated.

Changeset: de3113b9
Author:Michael McMahon 
URL:   
https://git.openjdk.java.net/jdk/commit/de3113b998550021bb502cd6f766036fb8351e7d
Stats: 858 lines in 12 files changed: 696 ins; 146 del; 16 mod

8279842: HTTPS Channel Binding support for Java GSS/Kerberos

Co-authored-by: Weijun Wang 
Reviewed-by: dfuchs, weijun, darcy

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v13]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  spec update from CSR

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/59f703da..468e5345

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=12
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=11-12

  Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]

[Michael McMahon]

On Thu, 27 Jan 2022 16:47:52 GMT, Daniel Fuchs  wrote:

>> It's `java.net.SocketException: Unexpected end of file from server`. Does 
>> not include any CBT words so don't know if it's worth parsing.
>
> Thanks. Then it would be better to catch only `SocketException` here rather 
> than `Exception`.

I'll make it catch `IOException`

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v12]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 15 additional commits 
since the last revision:

 - test update
 - Merge branch 'master' into spnego
 - test update
 - removed ^M from test
 - Added unit test and comment update
 - final review update (pre CSR)
 - more updates
 - fixed failing test issue and update for latest comments
 - Merge branch 'master' into spnego
 - added root cause to NamingException
 - ... and 5 more: https://git.openjdk.java.net/jdk/compare/35ce454c...59f703da

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/d604ee7f..59f703da

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=11
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=10-11

  Stats: 4735 lines in 368 files changed: 2835 ins; 809 del; 1091 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v11]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  test update

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/b44184de..d604ee7f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=10
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=09-10

  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  removed ^M from test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/e5a5a79a..b44184de

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=08-09

  Stats: 311 lines in 1 file changed: 0 ins; 0 del; 311 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v9]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  Added unit test and comment update

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/004466ea..e5a5a79a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=07-08

  Stats: 312 lines in 2 files changed: 311 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v8]

[Michael McMahon]

On Tue, 25 Jan 2022 11:34:57 GMT, Michael Osipov  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   final review update (pre CSR)
>
> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 150:
> 
>> 148:  * "domain:a,c.d,*.e.f" (sent to host a, or c.d or to the domain 
>> e.f and any of its subdomains). This is
>> 149:  * a comma separated list of arbitrary length with no white-space 
>> allowed.
>> 150:  * If enabled (for a particular destination) then SPNEGO 
>> authentication requests will include
> 
> Previously `Negotiate` was used, now `SPNEGO`?

"Negotiate" is the name of the HTTP authentication scheme which uses the SPNEGO 
mechanism. Possibly makes more sense to refer to Negotiate here.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v8]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  final review update (pre CSR)

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/0d529f9d..004466ea

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=06-07

  Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v7]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  more updates

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/ad80dfa2..0d529f9d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=05-06

  Stats: 38 lines in 2 files changed: 18 ins; 19 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]

[Michael McMahon]

On Mon, 24 Jan 2022 15:23:44 GMT, Weijun Wang  wrote:

>> Michael McMahon has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains eight additional 
>> commits since the last revision:
>> 
>>  - fixed failing test issue and update for latest comments
>>  - Merge branch 'master' into spnego
>>  - added root cause to NamingException
>>  - more tidy-up
>>  - removed sasl module dependency and added SaslException cause
>>  - changes after first review round
>>  - cleanup but still no test. Will be added in closed repo
>>  - First version of fix. No test and feature enabled always.
>
> src/java.base/share/classes/sun/security/util/TlsChannelBinding.java line 100:
> 
>> (failed to retrieve contents of file, check the PR for context)
> I think this method should stay here. Suppose one day the CBT type is 
> configurable for HTTPS we'll have to get it back. Of course we will need to 
> update the message to avoid talking about LDAP.

So, where should the two constant Strings go? It doesn't feel like they belong 
in java.base since they are JNDI/SASL related, and we can't have a method in 
`java.base` depending on code in other modules?

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]

[Michael McMahon]

On Fri, 21 Jan 2022 19:48:02 GMT, Weijun Wang  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   added root cause to NamingException
>
> src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:
> 
>> 218: This controls the generation and sending of TLS channel binding 
>> tokens (CBT) when Kerberos 
>> 219: or the Negotiate authentication scheme using Kerberos are 
>> employed over HTTPS with 
>> 220: {@code HttpsURLConnection}. There are three possible 
>> settings:
> 
> You can probably mention here that the 'tls-server-end-point' Channel Binding 
> Type defined in RFC 5929 is used.

I've updated this and moved the two properties to LdapSasl where they are used. 
Also, the test that was failing before needed some further updates.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains eight additional 
commits since the last revision:

 - fixed failing test issue and update for latest comments
 - Merge branch 'master' into spnego
 - added root cause to NamingException
 - more tidy-up
 - removed sasl module dependency and added SaslException cause
 - changes after first review round
 - cleanup but still no test. Will be added in closed repo
 - First version of fix. No test and feature enabled always.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/058c3830..ad80dfa2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=04-05

  Stats: 17855 lines in 614 files changed: 12290 ins; 2870 del; 2695 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  added root cause to NamingException

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/c9975fd1..058c3830

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=03-04

  Stats: 16 lines in 2 files changed: 8 ins; 4 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]

[Michael McMahon]

On Fri, 21 Jan 2022 13:39:06 GMT, Michael Osipov  wrote:

>> Actually, it turns out I should be throwing `NamingException` here. That is 
>> what was being thrown by `TlsChannelBinding.parseType` before and an 
>> existing test was expecting that. NamingException only takes a String 
>> message. So, there won't be a root cause exception.
>
> `NamingException` has `setRootCause()`. Why not use that? I use that one too 
> and full stack is retained.

Yes, I can do that. Though it will cause the existing LDAP channel binding test 
to fail which is checking for an empty root cause. That is checking unspecified 
behavior and I can change it to check for a `ChannelBindingException` as root 
cause. So long as we are okay having a non public exception type as the root 
cause, it's probably helpful to have the full stack there.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  more tidy-up

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/fd56b5e3..c9975fd1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=02-03

  Stats: 12 lines in 4 files changed: 5 ins; 1 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]

[Michael McMahon]

On Fri, 21 Jan 2022 13:38:08 GMT, Michael McMahon  wrote:

>> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 189:
>> 
>>> 187: } else {
>>> 188: logError("Unexpected value for \"jdk.https.negotiate.cbt\" 
>>> system property");
>>> 189: return s;
>> 
>> Should this return either "always" or "never" instead? It seems that junk 
>> values will be treated as "always". It would be better to make it clear here.
>
> It was being handled elsewhere as "never". But, I agree it would be clearer 
> to normalise it to "never" here.

Sorry, I should have checked back to the source rather than the snippet quoted. 
The problem is that the logError call is in the wrong place. It should be 
before line 186.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]

[Michael McMahon]

On Thu, 20 Jan 2022 11:16:16 GMT, Daniel Fuchs  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   removed sasl module dependency and added SaslException cause
>
> src/java.base/share/classes/sun/security/util/ChannelBindingException.java 
> line 31:
> 
>> 29:  * Thrown by TlsChannelBinding if an error occurs
>> 30:  */
>> 31: public class ChannelBindingException extends Exception {
> 
> Should this extend `GeneralSecurityException` instead? Or should we just 
> remove this class and throw plain `GeneralSecurityException` in 
> `TlsChannelBinding` ?

I think a distinct exception is necessary. I don't have a strong opinion on 
whether it should extend GeneralSecurityException.

> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 143:
> 
>> 141: tlsCB = TlsChannelBinding.create(cert);
>> 142: } catch (ChannelBindingException e) {
>> 143: throw new SaslException(e.getMessage());
> 
> Why is there a difference compared to line 133?

Right, that was a mistake.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]

[Michael McMahon]

On Thu, 20 Jan 2022 11:14:40 GMT, Michael Osipov  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   removed sasl module dependency and added SaslException cause
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:
> 
>> 131: 
>> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
>> 132: } catch (ChannelBindingException e) {
>> 133: throw new SaslException(e.getMessage(), e);
> 
> Why not ust pass the exception if the API allows? This looks like message 
> duplication.

Actually, it turns out I should be throwing `NamingException` here. That is 
what was being thrown by `TlsChannelBinding.parseType` before and an existing 
test was expecting that. NamingException only takes a String message. So, there 
won't be a root cause exception.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]

[Michael McMahon]

On Thu, 20 Jan 2022 11:04:18 GMT, Daniel Fuchs  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   removed sasl module dependency and added SaslException cause
>
> src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:
> 
>> 218: This controls the generation and sending of TLS channel binding 
>> tokens (CBT) when Kerberos 
>> 219: or the Negotiate authentication scheme using Kerberos are 
>> employed over HTTPS with 
>> 220: {@code HttpURLConnection}. There are three possible 
>> settings:
> 
> Should it be `{@code HttpsURLConnection}`?
> (BTW - can we use {@code } here ? Would be worth checking the generated doc)

Right HttpsURLConnection is better. {@code} works here.

> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 189:
> 
>> 187: } else {
>> 188: logError("Unexpected value for \"jdk.https.negotiate.cbt\" 
>> system property");
>> 189: return s;
> 
> Should this return either "always" or "never" instead? It seems that junk 
> values will be treated as "always". It would be better to make it clear here.

It was being handled elsewhere as "never". But, I agree it would be clearer to 
normalise it to "never" here.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v2]

[Michael McMahon]

On Wed, 19 Jan 2022 22:25:43 GMT, Weijun Wang  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   changes after first review round
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:
> 
>> 131: 
>> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
>> 132: } catch (ChannelBindingException e) {
>> 133: throw new SaslException(e.getMessage());
> 
> How about setting `e` as cause of new exception? In `TlsChannelBinding.java` 
> the when the original exception was thrown (the 2nd throws) there was a cause.

Agreed.

> src/java.security.jgss/share/classes/module-info.java line 36:
> 
>> 34: module java.security.jgss {
>> 35: requires java.naming;
>> 36: requires java.security.sasl;
> 
> Can this be removed now?

Yes, well spotted!

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  removed sasl module dependency and added SaslException cause

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/f2ee58ec..fd56b5e3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=01-02

  Stats: 2 lines in 2 files changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v2]

[Michael McMahon]

> Hi,
> 
> This change adds Channel Binding Token (CBT) support to HTTPS 
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
> authentication scheme. When enabled, the implementation preemptively includes 
> a CBT with authentication requests over Kerberos. The feature is enabled as 
> follows:
> 
> A system property "jdk.spnego.cbt" is defined which can have the values 
> "never" (default), which means the feature is disabled, "always", which means 
> the CBT is included for all https Negotiate authentications, or it can take 
> the form "domain:a,b.c,*.d.com" which is a comma separated list of 
> domains/hosts where the feature is enabled, and disabled everywhere else. In 
> the given example, the CBT would be included in authentication requests for 
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
> sub-domains.
> 
> A test will be added separately to the implementation.
> 
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  changes after first review round

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7065/files
  - new: https://git.openjdk.java.net/jdk/pull/7065/files/853ed4db..f2ee58ec

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=00-01

  Stats: 111 lines in 7 files changed: 88 ins; 5 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

On Fri, 14 Jan 2022 15:06:12 GMT, Daniel Fuchs  wrote:

> Have you been able to test this on a specific setup? Would be good to hear 
> from @msheppar too.

I have tested it with the server setup by Prajwal. Security SQE are looking 
into configuring a server with a similar setup which can be tested with an 
infra test.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

On Wed, 19 Jan 2022 15:36:16 GMT, Michael McMahon  wrote:

>>> It's actually a purely system property rather than a Net property at the 
>>> moment (same as the other spnego ones). Maybe, I should convert them all to 
>>> net properties, so they can be documented/set in that file?
>> 
>> AFAICS this file documents properties used by the networking stack - not 
>> necessarily net properties (e.g. java.net.preferIPv6Addresses is documented 
>> there but AFAICT it is a plain system property)
>
> Okay, good idea to document it in the properties file. Also, I think 
> "jdk.https.tls.cbt" is a reasonable name for the property.

Sorry, on reflection, something like "jdk.https.negotiate.cbt" might be better. 
There's no need for tls and https in the name and "negotiate" or "spnego" 
should be in it, but "negotiate" is probably better

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

On Sat, 15 Jan 2022 14:02:15 GMT, Michael Osipov  wrote:

>> I suggest moving the `TlsChannelBinding` class into 
>> `java.base/sun.security.util` since it's not only used by LDAP anymore. It's 
>> even not restricted to GSS-API. According to 
>> https://www.rfc-editor.org/rfc/rfc5056, "Although inspired by and derived 
>> from the GSS-API, the notion of channel binding described herein is not at 
>> all limited to use by GSS-API applications".
>> 
>> If so, you might need to modify the types of exceptions thrown in the class, 
>> and move the 2 final strings to some other class inside `java.security.sasl`.
>
> Seems like `com.sun.jndi.ldap.sasl.TlsChannelBinding` is not misplaced

Okay, I'll look at doing this refactoring.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

On Mon, 17 Jan 2022 13:44:06 GMT, Daniel Fuchs  wrote:

>> Shall we log a message if the value is not one of the 3 forms?
>
> Usually malformed values are just ignored - and the property takes its 
> default value. But yes - s.n.w.h.HttpClient has a logger so it wouldn't be 
> much effort to log it as a DEBUG trace for better diagnostic.

Yes, I will log it using the same debug/logging mechanism already in the same 
source file..

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

On Mon, 17 Jan 2022 13:49:35 GMT, Daniel Fuchs  wrote:

>> I vote for "jdk.https.tls.cbt"
>
>> It's actually a purely system property rather than a Net property at the 
>> moment (same as the other spnego ones). Maybe, I should convert them all to 
>> net properties, so they can be documented/set in that file?
> 
> AFAICS this file documents properties used by the networking task - not 
> necessarily net properties (e.g. java.net.preferIPv6Addresses is documented 
> there but AFAICT it is a plain system property)

Okay, good idea to document it in the properties file. Also, I think 
"jdk.https.tls.cbt" is a reasonable name for the property.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

On Fri, 14 Jan 2022 14:52:13 GMT, Daniel Fuchs  wrote:

>> Hi,
>> 
>> This change adds Channel Binding Token (CBT) support to HTTPS 
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, 
>> Kerberos) authentication scheme. When enabled, the implementation 
>> preemptively includes a CBT with authentication requests over Kerberos. The 
>> feature is enabled as follows:
>> 
>> A system property "jdk.spnego.cbt" is defined which can have the values 
>> "never" (default), which means the feature is disabled, "always", which 
>> means the CBT is included for all https Negotiate authentications, or it can 
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of 
>> domains/hosts where the feature is enabled, and disabled everywhere else. In 
>> the given example, the CBT would be included in authentication requests for 
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
>> sub-domains.
>> 
>> A test will be added separately to the implementation.
>> 
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>> 
>> Thanks,
>> Michael
>
> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 152:
> 
>> 150:  * If enabled (for a particular destination) then SPNEGO 
>> authentication requests will include
>> 151:  * a channel binding token for the destination server. The default 
>> behavior and setting for the
>> 152:  * property is "never"
> 
> Maybe this description should be added to 
> `src/java.base//share/classes/java/net/doc-files/net-properties.html` too?

It's actually a purely system property rather than a Net property at the moment 
(same as the other spnego ones). Maybe, I should convert them all to net 
properties, so they can be documented/set in that file?

> src/java.security.jgss/share/classes/module-info.java line 36:
> 
>> 34: module java.security.jgss {
>> 35: requires java.naming;
>> 36: requires java.security.sasl;
> 
> Someone from security-dev should probably review this and validate that this 
> is OK. I'm also a bit uncomfortable that we require a class from 
> `com.sun.jndi.ldap.sasl` even though `java.naming` is already required by 
> `java.security.jgss` - so maybe this is OK.

Yes. I would like the security team to validate this.

-

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

On Thu, 13 Jan 2022 18:18:24 GMT, Daniel Fuchs  wrote:

>> Hi,
>> 
>> This change adds Channel Binding Token (CBT) support to HTTPS 
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, 
>> Kerberos) authentication scheme. When enabled, the implementation 
>> preemptively includes a CBT with authentication requests over Kerberos. The 
>> feature is enabled as follows:
>> 
>> A system property "jdk.spnego.cbt" is defined which can have the values 
>> "never" (default), which means the feature is disabled, "always", which 
>> means the CBT is included for all https Negotiate authentications, or it can 
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of 
>> domains/hosts where the feature is enabled, and disabled everywhere else. In 
>> the given example, the CBT would be included in authentication requests for 
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
>> sub-domains.
>> 
>> A test will be added separately to the implementation.
>> 
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>> 
>> Thanks,
>> Michael
>
> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 180:
> 
>> 178: static String normalizeCBT(String s) {
>> 179: if (s == null || ! (s.equals("always") ||
>> 180: s.equals("never") || s.startsWith("domain:"))) {
> 
> I guess there's a `!` missing in front of  `s.startsWith("domain:")` here?

This is what was intended (equivalent)

`if (s ==null || (s!="always" && s!="never" && !s.startsWith("domain")))`

-

PR: https://git.openjdk.java.net/jdk/pull/7065

RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Michael McMahon]

Hi,

This change adds Channel Binding Token (CBT) support to HTTPS 
(java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) 
authentication scheme. When enabled, the implementation preemptively includes a 
CBT with authentication requests over Kerberos. The feature is enabled as 
follows:

A system property "jdk.spnego.cbt" is defined which can have the values "never" 
(default), which means the feature is disabled, "always", which means the CBT 
is included for all https Negotiate authentications, or it can take the form 
"domain:a,b.c,*.d.com" which is a comma separated list of domains/hosts where 
the feature is enabled, and disabled everywhere else. In the given example, the 
CBT would be included in authentication requests for hosts "a", "b.c" and all 
hosts under the domain "d.com" and all of its sub-domains.

A test will be added separately to the implementation.

Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842

Thanks,
Michael

-

Commit messages:
 - cleanup but still no test. Will be added in closed repo
 - First version of fix. No test and feature enabled always.

Changes: https://git.openjdk.java.net/jdk/pull/7065/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7065&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8279842
  Stats: 149 lines in 7 files changed: 143 ins; 0 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7065.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065

PR: https://git.openjdk.java.net/jdk/pull/7065

Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v10]

[Michael McMahon]

On Fri, 29 Oct 2021 16:17:46 GMT, Aleksei Efimov  wrote:

>> This change implements a new service provider interface for host name and 
>> address resolution, so that java.net.InetAddress API can make use of 
>> resolvers other than the platform's built-in resolver.
>> 
>> The following API classes are added to `java.net.spi` package to facilitate 
>> this:
>> - `InetAddressResolverProvider` -  abstract class defining a service, and 
>> is, essentially, a factory for `InetAddressResolver` resolvers.
>> - `InetAddressResolverProvider.Configuration ` - an interface describing the 
>> platform's built-in configuration for resolution operations that could be 
>> used to bootstrap a resolver construction, or to implement partial 
>> delegation of lookup operations.
>> - `InetAddressResolver` - an interface that defines methods for the 
>> fundamental forward and reverse lookup operations.
>> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the 
>> characteristics of one forward lookup operation.  
>> 
>> More details in [JEP-418](https://openjdk.java.net/jeps/418).
>> 
>> Testing: new and existing `tier1:tier3` tests
>
> Aleksei Efimov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Replace 'system' with 'the system-wide', move comment section

Marked as reviewed by michaelm (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5822

Re: RFR: 8273660: De-Serialization Stack is suppressing ClassNotFoundException [v2]

[Michael McMahon]

On Fri, 29 Oct 2021 15:35:50 GMT, Roger Riggs  wrote:

>> The ObjectInputStream.GetField method `get(String name, Object val)` should 
>> have been throwing
>> a ClassNotFoundException if the class was not found.  Instead the 
>> implementation was returning null.
>> A design error does not allow the `get(String name, Object val)`  method to 
>> throw CNFE as it should.
>> However, an exception must be thrown to prevent invalid data from being 
>> returned.
>> Wrapping the CNFE in IOException allows it to be thrown and the exception 
>> handled.
>> The call to `get(String name, Object val)`  is always from within a 
>> `readObject` method
>> so the deserialization logic can catch the IOException and unwrap it to 
>> handle the CNFE.
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Correct comment on the handling of ClassNotFoundException

How likely is it that existing code is using ObjectInputStream::getFields and 
is already handling class not found by checking for null return from the 
returned GetField?

-

PR: https://git.openjdk.java.net/jdk/pull/6053

Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v9]

[Michael McMahon]

On Tue, 26 Oct 2021 16:24:48 GMT, Aleksei Efimov  wrote:

>> This change implements a new service provider interface for host name and 
>> address resolution, so that java.net.InetAddress API can make use of 
>> resolvers other than the platform's built-in resolver.
>> 
>> The following API classes are added to `java.net.spi` package to facilitate 
>> this:
>> - `InetAddressResolverProvider` -  abstract class defining a service, and 
>> is, essentially, a factory for `InetAddressResolver` resolvers.
>> - `InetAddressResolverProvider.Configuration ` - an interface describing the 
>> platform's built-in configuration for resolution operations that could be 
>> used to bootstrap a resolver construction, or to implement partial 
>> delegation of lookup operations.
>> - `InetAddressResolver` - an interface that defines methods for the 
>> fundamental forward and reverse lookup operations.
>> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the 
>> characteristics of one forward lookup operation.  
>> 
>> More details in [JEP-418](https://openjdk.java.net/jeps/418).
>> 
>> Testing: new and existing `tier1:tier3` tests
>
> Aleksei Efimov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add @ throws NPE to hosts file resolver javadoc

src/java.base/share/classes/java/net/InetAddress.java line 841:

> 839: // 'resolver.lookupByAddress' and 'InetAddress.getAllByName0' 
> delegate to the system-wide resolver,
> 840: // which could be a custom one. At that point we treat any 
> unexpected RuntimeException thrown by
> 841: // the resolver as we would treat an UnknownHostException or an 
> unmatched host name.

indentation issue in comment above

src/java.base/share/classes/java/net/InetAddress.java line 1308:

> 1306:  * Creates an InetAddress based on the provided host name and IP 
> address.
> 1307:  * System {@linkplain InetAddressResolver resolver} is not used to 
> check
> 1308:  * the validity of the address.

Is this term "system resolver" defined somewhere? I think it means the 
configured resolver for the current VM. Previously, it really was the system 
resolver. So, I think it's potentially confusing, as there is also reference to 
the java.net.preferIPv6Addresses system property as having a possible value 
"system" which refers to the operating system. Since the CSR is approved, I'm 
happy to discuss this point post integration.

-

PR: https://git.openjdk.java.net/jdk/pull/5822

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v9]

[Michael McMahon]

On Tue, 12 Oct 2021 10:40:15 GMT, Julia Boes  wrote:

>> This change implements a simple web server that can be run on the 
>> command-line with `java -m jdk.httpserver`.
>> 
>> This is facilitated by adding an entry point for the `jdk.httpserver` 
>> module, an implementation class whose main method is run when the above 
>> command is executed. This is the first such module entry point in the JDK.
>> 
>> The server is a minimal HTTP server that serves the static files of a given 
>> directory, similar to existing alternatives on other platforms and 
>> convenient for testing, development, and debugging.
>> 
>> Additionally, a small API is introduced for programmatic creation and 
>> customization.
>> 
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 24 commits:
> 
>  - Minor rewording of bind address output
>  - Merge branch 'master' into simpleserver
>  - Merge branch 'master' into simpleserver
>  - update output for all interfaces
>  - use ipv4/ipv6 specific loopback address and add add how-to output for all 
> interfaces
>  - Merge branch 'master' into simpleserver
>  - change default bind address from anylocal to loopback
>  - address PR comments
>  - Merge branch 'master' into simpleserver
>  - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>  - ... and 14 more: 
> https://git.openjdk.java.net/jdk/compare/b1b66965...e86609d0

Marked as reviewed by michaelm (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5505

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v9]

[Michael McMahon]

On Tue, 12 Oct 2021 10:40:15 GMT, Julia Boes  wrote:

>> This change implements a simple web server that can be run on the 
>> command-line with `java -m jdk.httpserver`.
>> 
>> This is facilitated by adding an entry point for the `jdk.httpserver` 
>> module, an implementation class whose main method is run when the above 
>> command is executed. This is the first such module entry point in the JDK.
>> 
>> The server is a minimal HTTP server that serves the static files of a given 
>> directory, similar to existing alternatives on other platforms and 
>> convenient for testing, development, and debugging.
>> 
>> Additionally, a small API is introduced for programmatic creation and 
>> customization.
>> 
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 24 commits:
> 
>  - Minor rewording of bind address output
>  - Merge branch 'master' into simpleserver
>  - Merge branch 'master' into simpleserver
>  - update output for all interfaces
>  - use ipv4/ipv6 specific loopback address and add add how-to output for all 
> interfaces
>  - Merge branch 'master' into simpleserver
>  - change default bind address from anylocal to loopback
>  - address PR comments
>  - Merge branch 'master' into simpleserver
>  - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>  - ... and 14 more: 
> https://git.openjdk.java.net/jdk/compare/b1b66965...e86609d0

test/jdk/com/sun/net/httpserver/simpleserver/ServerMimeTypesResolutionTest.java 
line 200:

> 198:  */
> 199: //@Test(dataProvider = "commonExtensions")
> 200: public static void testCommonExtensions(String extension) {

Is this test and the one below supposed to be commented out?

-

PR: https://git.openjdk.java.net/jdk/pull/5505

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]

[Michael McMahon]

On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes  wrote:

>> This change implements a simple web server that can be run on the 
>> command-line with `java -m jdk.httpserver`.
>> 
>> This is facilitated by adding an entry point for the `jdk.httpserver` 
>> module, an implementation class whose main method is run when the above 
>> command is executed. This is the first such module entry point in the JDK.
>> 
>> The server is a minimal HTTP server that serves the static files of a given 
>> directory, similar to existing alternatives on other platforms and 
>> convenient for testing, development, and debugging.
>> 
>> Additionally, a small API is introduced for programmatic creation and 
>> customization.
>> 
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' into simpleserver
>  - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>  - Merge branch 'master' into simpleserver
>  - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>  - Merge branch 'master' into simpleserver
>  - check isHidden, isSymlink, isReadable for all path segments 
>  - add checks for all path segments
>  - Merge branch 'master' into componentcheck
>  - Merge branch 'master' into simpleserver
>  - improve output on startup
>  - ... and 6 more: 
> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131

src/jdk.httpserver/share/classes/sun/net/httpserver/simpleserver/SimpleFileServerImpl.java
 line 135:

> 133: var socketAddr = new InetSocketAddress(addr, port);
> 134: var server = SimpleFileServer.createFileServer(socketAddr, 
> root, outputLevel);
> 135: server.setExecutor(Executors.newSingleThreadExecutor());

I think this code has the effect of creating one thread for the selector and a 
second one for the execution of the handlers. If we want to keep thread usage 
to an absolute minimum then it might be better to not set an executor. Then, 
the selector thread executes the handlers as well.

-

PR: https://git.openjdk.java.net/jdk/pull/5505

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]

[Michael McMahon]

On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes  wrote:

>> This change implements a simple web server that can be run on the 
>> command-line with `java -m jdk.httpserver`.
>> 
>> This is facilitated by adding an entry point for the `jdk.httpserver` 
>> module, an implementation class whose main method is run when the above 
>> command is executed. This is the first such module entry point in the JDK.
>> 
>> The server is a minimal HTTP server that serves the static files of a given 
>> directory, similar to existing alternatives on other platforms and 
>> convenient for testing, development, and debugging.
>> 
>> Additionally, a small API is introduced for programmatic creation and 
>> customization.
>> 
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' into simpleserver
>  - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>  - Merge branch 'master' into simpleserver
>  - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>  - Merge branch 'master' into simpleserver
>  - check isHidden, isSymlink, isReadable for all path segments 
>  - add checks for all path segments
>  - Merge branch 'master' into componentcheck
>  - Merge branch 'master' into simpleserver
>  - improve output on startup
>  - ... and 6 more: 
> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131

Changes requested by michaelm (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5505

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v5]

[Michael McMahon]

On Tue, 21 Sep 2021 14:09:54 GMT, Julia Boes  wrote:

>> This change implements a simple web server that can be run on the 
>> command-line with `java -m jdk.httpserver`.
>> 
>> This is facilitated by adding an entry point for the `jdk.httpserver` 
>> module, an implementation class whose main method is run when the above 
>> command is executed. This is the first such module entry point in the JDK.
>> 
>> The server is a minimal HTTP server that serves the static files of a given 
>> directory, similar to existing alternatives on other platforms and 
>> convenient for testing, development, and debugging.
>> 
>> Additionally, a small API is introduced for programmatic creation and 
>> customization.
>> 
>> Testing: tier1-3.
>
> Julia Boes has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' into simpleserver
>  - Merge remote-tracking branch 'origin/simpleserver' into simpleserver
>  - Merge branch 'master' into simpleserver
>  - refactor isHidden,isReadable,isSymlink checks and cleanup tests
>  - Merge branch 'master' into simpleserver
>  - check isHidden, isSymlink, isReadable for all path segments 
>  - add checks for all path segments
>  - Merge branch 'master' into componentcheck
>  - Merge branch 'master' into simpleserver
>  - improve output on startup
>  - ... and 6 more: 
> https://git.openjdk.java.net/jdk/compare/6d91a3eb...fe059131

test/jdk/com/sun/net/httpserver/FilterTest.java line 330:

> 328: var response = client.send(request, 
> HttpResponse.BodyHandlers.ofString());
> 329: assertEquals(response.statusCode(), 200);
> 330: assertEquals(inspectedURI.get(), URI.create("/"));

Could you make the request URI something like /foo/bar instead of just / here?

-

PR: https://git.openjdk.java.net/jdk/pull/5505

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v2]

[Michael McMahon]

On Wed, 15 Sep 2021 08:42:40 GMT, Julia Boes  wrote:

>> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsServer.java 
>> line 152:
>> 
>>> 150: return server;
>>> 151: }
>>> 152: 
>> 
>> Too bad we couldn't simplify the setting up a basic certificate for https.
>
> That would be nice indeed, but the goal of this JEP is a minimal HTTP-only 
> server, intentionally leaving anything HTTPS aside. `HttpsServer::create` 
> being the exception, added to provide the same convenience as for 
> `HttpServer`. Any HTTPS configuration can be done using the existing API.

I agree the JEP should focus on a minimal HTTP server and the new API does 
allow an HTTPS based file server to be setup in one or two lines of code. I 
don't think there would be much value in providing something like self signed 
certificates, but it has become a lot easier to obtain real https certificates 
through services like LetsEncrypt so it might be interesting to write an 
article for inside.java showing how to set up a HTTPS server from start to 
finish.

-

PR: https://git.openjdk.java.net/jdk/pull/5505

Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server

[Michael McMahon]

On Tue, 14 Sep 2021 16:51:40 GMT, Daniel Fuchs  wrote:

>> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpHandlers.java 
>> line 129:
>> 
>>> 127:  * response body bytes are a {@code UTF-8} encoded byte 
>>> sequence of
>>> 128:  * {@code body}. The response {@linkplain 
>>> HttpExchange#sendResponseHeaders(int, long) is sent}
>>> 129:  * with the given {@code statusCode} and the body bytes' length. 
>>> The body
>> 
>> That might give the impression that chunked encoding will be used if the 
>> body length is 0. I wonder if it should say instead:
>> 
>> 
>> with the given {@code statusCode} and a {@code Content-Length} field set to 
>> the body bytes' length.
>
> Or maybe - which would be more accurate:
> 
> 
> with the given {@code statusCode} and the body bytes' length (or {@code -1} 
> if the body is empty).

I agree with your second suggestion. It's better not to refer to the 
'Content-Length' header at all.

-

PR: https://git.openjdk.java.net/jdk/pull/5505

Re: RFR: 8262883: doccheck: Broken links in java.base

[Michael McMahon]

On Wed, 14 Apr 2021 15:27:36 GMT, Alan Bateman  wrote:

>> Hi,
>> 
>> Could I get the following trivial doc changes reviewed please, caused by:
>> 
>> - broken  tags in MethodHandles referring to package.html instead of 
>> package-summary.html
>> 
>> - references to a package level #unixdomain anchor that no longer exists.
>> 
>> - a  tag missing a "../" in SocketChannel
>> 
>> Thanks,
>> 
>> Michael
>
> src/java.base/share/classes/java/nio/channels/DatagramChannel.java line 175:
> 
>> 173:  * connected.
>> 174:  *
>> 175:  * @apiNote {@linkplain java.net.UnixDomainSocketAddress Unix 
>> domain} sockets
> 
> The parameter to this method is a ProtocolFamily and maybe this note should 
> be say that StandardProtocolFamily#UNIX is not supported.

Okay, reasonable suggestion. I was a bit quick integrating. I will file a new 
issue to update the spec as suggested.

-

PR: https://git.openjdk.java.net/jdk/pull/3491

Integrated: 8262883: doccheck: Broken links in java.base

[Michael McMahon]

On Wed, 14 Apr 2021 14:03:01 GMT, Michael McMahon  wrote:

> Hi,
> 
> Could I get the following trivial doc changes reviewed please, caused by:
> 
> - broken  tags in MethodHandles referring to package.html instead of 
> package-summary.html
> 
> - references to a package level #unixdomain anchor that no longer exists.
> 
> - a  tag missing a "../" in SocketChannel
> 
> Thanks,
> 
> Michael

This pull request has now been integrated.

Changeset: 46616909
Author:Michael McMahon 
URL:   https://git.openjdk.java.net/jdk/commit/46616909
Stats: 5 lines in 4 files changed: 0 ins; 0 del; 5 mod

8262883: doccheck: Broken links in java.base

Reviewed-by: lancea

-

PR: https://git.openjdk.java.net/jdk/pull/3491

RFR: 8262883: doccheck: Broken links in java.base

[Michael McMahon]

Hi,

Could I get the following trivial doc changes reviewed please, caused by:

- broken  tags in MethodHandles referring to package.html instead of 
package-summary.html

- references to a package level #unixdomain anchor that no longer exists.

- a  tag missing a "../" in SocketChannel

Thanks,

Michael

-

Commit messages:
 - Fixed SocketChannel
 - fixed links in apidoc

Changes: https://git.openjdk.java.net/jdk/pull/3491/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3491&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262883
  Stats: 5 lines in 4 files changed: 0 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3491.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3491/head:pull/3491

PR: https://git.openjdk.java.net/jdk/pull/3491

Integrated: 8252971: WindowsFileAttributes does not know about Unix domain sockets

[Michael McMahon]

On Fri, 5 Feb 2021 09:52:11 GMT, Michael McMahon  wrote:

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

This pull request has now been integrated.

Changeset: 9ffabf30
Author:Michael McMahon 
URL:   https://git.openjdk.java.net/jdk/commit/9ffabf30
Stats: 245 lines in 10 files changed: 228 ins; 10 del; 7 mod

8252971: WindowsFileAttributes does not know about Unix domain sockets

Reviewed-by: alanb

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v9]

[Michael McMahon]

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 17 additional commits 
since the last revision:

 - Merge branch 'master' into 8252971-socket-attributes
 - Rearrange test in WindowsPath. Change to Security.java test to explicitly 
delete socket files
 - Merge branch 'master' into 8252971-socket-attributes
 - update
 - fix mistake in last push
 - update
 - Merge branch 'master' into 8252971-socket-attributes
 - add specific check for unix domain socket
 - Merge branch 'master' into 8252971-socket-attributes
 - update
 - ... and 7 more: https://git.openjdk.java.net/jdk/compare/3bc0384d...11ae5e72

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2424/files
  - new: https://git.openjdk.java.net/jdk/pull/2424/files/70832057..11ae5e72

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=07-08

  Stats: 2988 lines in 123 files changed: 1583 ins; 865 del; 540 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v7]

[Michael McMahon]

On Tue, 9 Feb 2021 15:33:48 GMT, Alan Bateman  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   update
>
> src/java.base/windows/classes/sun/nio/fs/WindowsPath.java line 840:
> 
>> 838: throw e;
>> 839: // Object could be a Unix domain socket
>> 840: try {
> 
> Thanks for the updates, I think the fallback checking is correct everywhere 
> now.
> What would you think about inverting the above so that it only checks for a 
> socket file when followLinks && e.lastError() == ERROR_CANT_ACCESS_FILE. That 
> would make it more consistent with the new code in WIndowsFileSystemProvider.

Okay. No problem. The next revision also includes an updated test where I found 
some socket files weren't being cleaned up at the end of the test.

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v8]

[Michael McMahon]

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 16 additional commits 
since the last revision:

 - Rearrange test in WindowsPath. Change to Security.java test to explicitly 
delete socket files
 - Merge branch 'master' into 8252971-socket-attributes
 - update
 - fix mistake in last push
 - update
 - Merge branch 'master' into 8252971-socket-attributes
 - add specific check for unix domain socket
 - Merge branch 'master' into 8252971-socket-attributes
 - update
 - update
 - ... and 6 more: https://git.openjdk.java.net/jdk/compare/a7819067...70832057

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2424/files
  - new: https://git.openjdk.java.net/jdk/pull/2424/files/92031fbc..70832057

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=06-07

  Stats: 9487 lines in 220 files changed: 5561 ins; 2694 del; 1232 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v7]

[Michael McMahon]

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  update

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2424/files
  - new: https://git.openjdk.java.net/jdk/pull/2424/files/c1fd3565..92031fbc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=05-06

  Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v6]

[Michael McMahon]

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  fix mistake in last push

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2424/files
  - new: https://git.openjdk.java.net/jdk/pull/2424/files/f644e939..c1fd3565

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=04-05

  Stats: 5 lines in 1 file changed: 1 ins; 2 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v4]

[Michael McMahon]

On Mon, 8 Feb 2021 21:59:53 GMT, Michael McMahon  wrote:

>> src/java.base/windows/classes/sun/nio/fs/WindowsPath.java line 862:
>> 
>>> 860:  * and a handle to the socket file if it is.
>>> 861:  */
>>> 862: private long openSocketForReadAttributeAccess() {
>> 
>> The methods here throw WindowsException when they fail and might be better 
>> to keep it consistent and throw WindowsException rather than returning 
>> INVALID_HANDLE_VALUE.
>
> Okay

Actually, there's a mistake in the current latest version (04). Should have 
tested it before pushing. Will push update shortly.

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v4]

[Michael McMahon]

On Mon, 8 Feb 2021 16:50:02 GMT, Alan Bateman  wrote:

>> Michael McMahon has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 11 additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - add specific check for unix domain socket
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - update
>>  - update
>>  - update
>>  - update
>>  - update after Alan's first review
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - test update
>>  - ... and 1 more: 
>> https://git.openjdk.java.net/jdk/compare/4e1be4ef...746b4762
>
> src/java.base/windows/classes/sun/nio/fs/WindowsPath.java line 862:
> 
>> 860:  * and a handle to the socket file if it is.
>> 861:  */
>> 862: private long openSocketForReadAttributeAccess() {
> 
> The methods here throw WindowsException when they fail and might be better to 
> keep it consistent and throw WindowsException rather than returning 
> INVALID_HANDLE_VALUE.

Okay

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v5]

[Michael McMahon]

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  update

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2424/files
  - new: https://git.openjdk.java.net/jdk/pull/2424/files/746b4762..f644e939

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=03-04

  Stats: 29 lines in 2 files changed: 2 ins; 16 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v4]

[Michael McMahon]

On Mon, 8 Feb 2021 16:48:44 GMT, Alan Bateman  wrote:

>> Michael McMahon has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 11 additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - add specific check for unix domain socket
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - update
>>  - update
>>  - update
>>  - update
>>  - update after Alan's first review
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - test update
>>  - ... and 1 more: 
>> https://git.openjdk.java.net/jdk/compare/6a98ca22...746b4762
>
> src/java.base/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java line 
> 346:
> 
>> 344: 
>> Set.of(WindowsChannelFactory.OPEN_REPARSE_POINT),
>> 345: 0L);
>> 346: fc.close();
> 
> The new version looks a bit strange. As I read it, the attempt to open the 
> file fails with ERROR_CANT_ACCESS_FILE so you then check test if the file is 
> a socket file. That succeeds so we should be done. What is the reason for 
> opening the file again?

I added the test of the socket reparse tag to be 100% sure that we weren't 
returning false positives for other cases of ERROR_CANT_ACCESS_FILE. But, 
otherwise, I thought I had to open the file the same way as before to be sure 
that it is readable. If checking the reparse tag is sufficient then great, that 
simplifies the change.

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v4]

[Michael McMahon]

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 11 additional commits 
since the last revision:

 - Merge branch 'master' into 8252971-socket-attributes
 - add specific check for unix domain socket
 - Merge branch 'master' into 8252971-socket-attributes
 - update
 - update
 - update
 - update
 - update after Alan's first review
 - Merge branch 'master' into 8252971-socket-attributes
 - test update
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/fa59fe8c...746b4762

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2424/files
  - new: https://git.openjdk.java.net/jdk/pull/2424/files/de2a3428..746b4762

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=02-03

  Stats: 2471 lines in 59 files changed: 1349 ins; 861 del; 261 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v2]

[Michael McMahon]

On Sat, 6 Feb 2021 17:10:39 GMT, Alan Bateman  wrote:

>> Michael McMahon has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains ten additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - update
>>  - update
>>  - update
>>  - update
>>  - update after Alan's first review
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - test update
>>  - initial fix for review
>
> src/java.base/windows/classes/sun/nio/fs/WindowsPath.java line 866:
> 
>> 864: 
>> 865: try {
>> 866: // needs one additional flag to open a Unix domain socket
> 
> The comment here is confusing as there are two flags.

Okay

> src/java.base/windows/classes/sun/nio/fs/WindowsPath.java line 859:
> 
>> 857: 
>> 858: /**
>> 859:  * returns INVALID_HANDLE_VALUE if path is not a socket
> 
> Can you change this to "Returns".

Okay

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v2]

[Michael McMahon]

On Sun, 7 Feb 2021 09:15:35 GMT, Alan Bateman  wrote:

>> Though looking at that piece of code, I think its purpose would be clearer 
>> if it were put in a separate method with a name that shows were trying to 
>> open it as a socket.
>
> Moving it to a separate method would make it easier to maintain and discuss 
> here but I would really prefer if it checked the reparse tag and re-throw the 
> original exception if the tag is IO_REPARSE_TAG_SYMLINK. That way it wouldn't 
> get tripped up by ERROR_CANT_ACCESS_FILE being thrown for other reasons, e.g. 
> sym link -> sym link -> target where an intermediate sym link can't be 
> accessed.

Okay, I can add a check for WindowsFileAttributes.isUnixDomainSocket() in there 
when ERROR_CANT_ACCESS_FILE is returned.

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v3]

[Michael McMahon]

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  add specific check for unix domain socket

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2424/files
  - new: https://git.openjdk.java.net/jdk/pull/2424/files/b61686f4..de2a3428

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=01-02

  Stats: 10 lines in 2 files changed: 7 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v2]

[Michael McMahon]

On Sat, 6 Feb 2021 20:35:58 GMT, Michael McMahon  wrote:

>> src/java.base/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java line 
>> 344:
>> 
>>> 342: 
>>> Set.of(WindowsChannelFactory.OPEN_REPARSE_POINT),
>>> 343: 0L);
>>> 344: fc.close();
>> 
>> checkAccess follows sym links and I wonder if there is any scenario where 
>> ERROR_CANT_ACCESS_FILE can be returned when respare point rather than the 
>> target cannot be accessed. This may be something the Microsoft folks can 
>> comment on.
>> 
>> Minor nit is the params to newFileChannel is are mis-aligned now.
>
> So, if the reparse point is a symbolic link, and if the target of the link 
> does not exist, then  ERROR_FILE_NOT_FOUND is returned. I've tested and 
> confirmed this. It's also what you would expect because you would expect 
> usage of symbolic links to be transparent and the same error code to be 
> returned as when traversing a path and some element of the path does not 
> exist, or isn't accessible.
> 
> From what I can see ERROR_CANT_ACCESS_FILE can be returned for other reasons 
> such as file corruption, which I think would be likely to return the same 
> error again the second time.
> 
> I've also done some manual testing of other kinds of reparse point (junctions 
> and mount points) and have tried various "target not existing" type of 
> scenarios and ERROR_CANT_ACCESS_FILE is never returned.

Though looking at that piece of code, I think its purpose would be clearer if 
it were put in a separate method with a name that shows were trying to open it 
as a socket.

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v2]

[Michael McMahon]

On Sat, 6 Feb 2021 17:09:11 GMT, Alan Bateman  wrote:

>> Michael McMahon has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains ten additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - update
>>  - update
>>  - update
>>  - update
>>  - update after Alan's first review
>>  - Merge branch 'master' into 8252971-socket-attributes
>>  - test update
>>  - initial fix for review
>
> src/java.base/windows/classes/sun/nio/fs/WindowsFileSystemProvider.java line 
> 344:
> 
>> 342: 
>> Set.of(WindowsChannelFactory.OPEN_REPARSE_POINT),
>> 343: 0L);
>> 344: fc.close();
> 
> checkAccess follows sym links and I wonder if there is any scenario where 
> ERROR_CANT_ACCESS_FILE can be returned when respare point rather than the 
> target cannot be accessed. This may be something the Microsoft folks can 
> comment on.
> 
> Minor nit is the params to newFileChannel is are mis-aligned now.

So, if the reparse point is a symbolic link, and if the target of the link does 
not exist, then  ERROR_FILE_NOT_FOUND is returned. I've tested and confirmed 
this. It's also what you would expect because you would expect usage of 
symbolic links to be transparent and the same error code to be returned as when 
traversing a path and some element of the path does not exist, or isn't 
accessible.

>From what I can see ERROR_CANT_ACCESS_FILE can be returned for other reasons 
>such as file corruption, which I think would be likely to return the same 
>error again the second time.

I've also done some manual testing of other kinds of reparse point (junctions 
and mount points) and have tried various "target not existing" type of 
scenarios and ERROR_CANT_ACCESS_FILE is never returned.

-

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets [v2]

[Michael McMahon]

> Could I get the following change reviewed please? It fixes a problem (in 
> JEP380) on Windows where some file operations on Unix domain sockets were not 
> working and led to the feature being disabled on Windows 2019 Server in JDK 
> 16. So, the fix re-enables the feature on all versions of Windows that 
> support it.
> 
> The test checks all the file APIs that were affected by the problem. The 
> change touches the code that handles symbolic links in Windows (since they 
> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
> any specific testing in this area, as I assume the existing unit tests for 
> NIO symbolic links should cover that. If I should add more tests here, then I 
> can do that.
> 
> Thanks,
> Michael

Michael McMahon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains ten additional 
commits since the last revision:

 - Merge branch 'master' into 8252971-socket-attributes
 - update
 - update
 - update
 - update
 - update after Alan's first review
 - Merge branch 'master' into 8252971-socket-attributes
 - test update
 - initial fix for review

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2424/files
  - new: https://git.openjdk.java.net/jdk/pull/2424/files/fb6c719d..b61686f4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=00-01

  Stats: 10870 lines in 557 files changed: 6876 ins; 2384 del; 1610 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets

[Michael McMahon]

On Fri, 5 Feb 2021 12:03:30 GMT, Daniel Fuchs  wrote:

>> Could I get the following change reviewed please? It fixes a problem (in 
>> JEP380) on Windows where some file operations on Unix domain sockets were 
>> not working and led to the feature being disabled on Windows 2019 Server in 
>> JDK 16. So, the fix re-enables the feature on all versions of Windows that 
>> support it.
>> 
>> The test checks all the file APIs that were affected by the problem. The 
>> change touches the code that handles symbolic links in Windows (since they 
>> are implemented as NTFS reparse points, like Unix sockets), but I didn't add 
>> any specific testing in this area, as I assume the existing unit tests for 
>> NIO symbolic links should cover that. If I should add more tests here, then 
>> I can do that.
>> 
>> Thanks,
>> Michael
>
> test/jdk/java/nio/channels/unixdomain/FileAttributes.java line 82:
> 
>> 80: 
>> 81: // Check deletion
>> 82: assertTrue(f.delete(), "File.delete failed");
> 
> What happens to the socket if you delete the file before closing the socket? 
> Isn't that going to cause trouble?

Deleting the file prevents new connections from being accepted, but doesn't 
cause any problems for the channel itself. Since the test is finished at that 
point I don't think there is a problem.

-

PR: https://git.openjdk.java.net/jdk/pull/2424

RFR: 8252971: WindowsFileAttributes does not know about Unix domain sockets

[Michael McMahon]

Could I get the following change reviewed please? It fixes a problem (in 
JEP380) on Windows where some file operations on Unix domain sockets were not 
working and led to the feature being disabled on Windows 2019 Server in JDK 16. 
So, the fix re-enables the feature on all versions of Windows that support it.

The test checks all the file APIs that were affected by the problem. The change 
touches the code that handles symbolic links in Windows (since they are 
implemented as NTFS reparse points, like Unix sockets), but I didn't add any 
specific testing in this area, as I assume the existing unit tests for NIO 
symbolic links should cover that. If I should add more tests here, then I can 
do that.

Thanks,
Michael

-

Commit messages:
 - update
 - update
 - update
 - update
 - update after Alan's first review
 - Merge branch 'master' into 8252971-socket-attributes
 - test update
 - initial fix for review

Changes: https://git.openjdk.java.net/jdk/pull/2424/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2424&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252971
  Stats: 241 lines in 8 files changed: 226 ins; 9 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2424.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2424/head:pull/2424

PR: https://git.openjdk.java.net/jdk/pull/2424

Re: RFR: 8258582: HttpClient: the HttpClient doesn't explicitly shutdown its default executor when stopping.

[Michael McMahon]

On Thu, 17 Dec 2020 14:51:44 GMT, Daniel Fuchs  wrote:

> Hi,
> 
> Please find an almost trivial fix for:
> 8258582: HttpClient: the HttpClient doesn't explicitly shutdown its default 
> executor when stopping.
> 
> The HttpClient should shutdown his executor when stopping, when the executor 
> was created by the HttpClient itself.
> 
> best regards,
> 
> -- daniel

Marked as reviewed by michaelm (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/1822

Re: RFR: 8245194: Unix domain socket channel implementation [v7]

[Michael McMahon]

> Continuing this review as a PR on github with the comments below 
> incorporated. I expect there will be a few more
> iterations before integrating.
> On 06/09/2020 19:47, Alan Bateman wrote:
>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>
>>> As I mentioned the other day, I wasn't able to use the suggested method on 
>>> Windows which returns an absolute path. So,
>>> I added a method to WindowsPath which returns the path in the expected 
>>> UTF-8 encoding as a byte[]. Let me know what you
>>> think of that.
>>>
>>> There is a fair bit of other refactoring and simplification done also. Next 
>>> version is at:
>>>
>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>
>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I 
>> don't think we should be caching it as the
>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about 
>> encoding a relative path when the resolved
>> path (before encoding) is > 247 chars. The documentation on the MS site 
>> isn't very completely and I think there are a
>> number points that require clarification to fully understand how this will 
>> work with relative, directly relative and
>> drive relative paths.
>>
> 
> Maybe it would be better to just use the path returned from toString() and do 
> the conversion to UTF-8 externally. That
> would leave WindowsPath unchanged.
>> In the same area, the new PathUtil is a bit inconsistent with the existing 
>> provider code. One suggestion is to add a
>> method to AbstractFileSystemProvider instead. That is the base class the 
>> platform providers and would be a better place
>> to get the file path in bytes.
>>
> 
> Okay, I gave the method a name that is specific to Unix domain sockets 
> because this UTF-8 strangeness is not likely to
> be useful by other components.
>> One other comment on the changes to the file system provider it should be 
>> okay to change UnixUserPrinipals ad its
>> fromUid and fromGid method to be public. This would mean that the patch 
>> shouldn't need to add UnixUserGroup (the main
>> issue is that class is that it means we end up with two classes with static 
>> factory methods doing the same thing).
> 
> Okay, that does simplify it a bit.
> 
> Thanks,
> Michael.
> 
>> -Alan.
>>
>>
>>
>>
>>
>>

Michael McMahon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev
excludes the unrelated changes brought in by the merge/rebase. The pull request 
contains eight additional commits since
the last revision:

 - Merge branch 'master' into unixdomainchannels
 - unixdomainchannels: some tests were failing on Windows version prior to 
10/2019
 - unixdomainchannels: updates after comments sent by Alan 14 Sept
 - unixdomainchannels: updates from review of Sept 11 2020
 - Merge branch 'master' into unixdomainchannels
 - Made some changes relating to selection of the local directory where 
automatically
   bound server sockets are created. After this change it is no longer necessary
   to specify the location in individual tests.
 - unixdomainchannels: fixing compile error on Windows and Alan's review 
comment this morning
 - unixdomainchannels: initial commit from hg sandbox with changes from Alan's 
email 06/09/2020

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/52/files
  - new: https://git.openjdk.java.net/jdk/pull/52/files/d29e8cc2..a08186d7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=05-06

  Stats: 15223 lines in 623 files changed: 7434 ins; 6123 del; 1666 mod
  Patch: https://git.openjdk.java.net/jdk/pull/52.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52

PR: https://git.openjdk.java.net/jdk/pull/52

Re: RFR: 8245194: Unix domain socket channel implementation [v6]

[Michael McMahon]

> Continuing this review as a PR on github with the comments below 
> incorporated. I expect there will be a few more
> iterations before integrating.
> On 06/09/2020 19:47, Alan Bateman wrote:
>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>
>>> As I mentioned the other day, I wasn't able to use the suggested method on 
>>> Windows which returns an absolute path. So,
>>> I added a method to WindowsPath which returns the path in the expected 
>>> UTF-8 encoding as a byte[]. Let me know what you
>>> think of that.
>>>
>>> There is a fair bit of other refactoring and simplification done also. Next 
>>> version is at:
>>>
>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>
>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I 
>> don't think we should be caching it as the
>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about 
>> encoding a relative path when the resolved
>> path (before encoding) is > 247 chars. The documentation on the MS site 
>> isn't very completely and I think there are a
>> number points that require clarification to fully understand how this will 
>> work with relative, directly relative and
>> drive relative paths.
>>
> 
> Maybe it would be better to just use the path returned from toString() and do 
> the conversion to UTF-8 externally. That
> would leave WindowsPath unchanged.
>> In the same area, the new PathUtil is a bit inconsistent with the existing 
>> provider code. One suggestion is to add a
>> method to AbstractFileSystemProvider instead. That is the base class the 
>> platform providers and would be a better place
>> to get the file path in bytes.
>>
> 
> Okay, I gave the method a name that is specific to Unix domain sockets 
> because this UTF-8 strangeness is not likely to
> be useful by other components.
>> One other comment on the changes to the file system provider it should be 
>> okay to change UnixUserPrinipals ad its
>> fromUid and fromGid method to be public. This would mean that the patch 
>> shouldn't need to add UnixUserGroup (the main
>> issue is that class is that it means we end up with two classes with static 
>> factory methods doing the same thing).
> 
> Okay, that does simplify it a bit.
> 
> Thanks,
> Michael.
> 
>> -Alan.
>>
>>
>>
>>
>>
>>

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  unixdomainchannels: some tests were failing on Windows version prior to 
10/2019

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/52/files
  - new: https://git.openjdk.java.net/jdk/pull/52/files/36695095..d29e8cc2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=04-05

  Stats: 11 lines in 3 files changed: 8 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/52.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52

PR: https://git.openjdk.java.net/jdk/pull/52

Re: RFR: 8245194: Unix domain socket channel implementation [v5]

[Michael McMahon]

> Continuing this review as a PR on github with the comments below 
> incorporated. I expect there will be a few more
> iterations before integrating.
> On 06/09/2020 19:47, Alan Bateman wrote:
>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>
>>> As I mentioned the other day, I wasn't able to use the suggested method on 
>>> Windows which returns an absolute path. So,
>>> I added a method to WindowsPath which returns the path in the expected 
>>> UTF-8 encoding as a byte[]. Let me know what you
>>> think of that.
>>>
>>> There is a fair bit of other refactoring and simplification done also. Next 
>>> version is at:
>>>
>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>
>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I 
>> don't think we should be caching it as the
>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about 
>> encoding a relative path when the resolved
>> path (before encoding) is > 247 chars. The documentation on the MS site 
>> isn't very completely and I think there are a
>> number points that require clarification to fully understand how this will 
>> work with relative, directly relative and
>> drive relative paths.
>>
> 
> Maybe it would be better to just use the path returned from toString() and do 
> the conversion to UTF-8 externally. That
> would leave WindowsPath unchanged.
>> In the same area, the new PathUtil is a bit inconsistent with the existing 
>> provider code. One suggestion is to add a
>> method to AbstractFileSystemProvider instead. That is the base class the 
>> platform providers and would be a better place
>> to get the file path in bytes.
>>
> 
> Okay, I gave the method a name that is specific to Unix domain sockets 
> because this UTF-8 strangeness is not likely to
> be useful by other components.
>> One other comment on the changes to the file system provider it should be 
>> okay to change UnixUserPrinipals ad its
>> fromUid and fromGid method to be public. This would mean that the patch 
>> shouldn't need to add UnixUserGroup (the main
>> issue is that class is that it means we end up with two classes with static 
>> factory methods doing the same thing).
> 
> Okay, that does simplify it a bit.
> 
> Thanks,
> Michael.
> 
>> -Alan.
>>
>>
>>
>>
>>
>>

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  unixdomainchannels: updates after comments sent by Alan 14 Sept

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/52/files
  - new: https://git.openjdk.java.net/jdk/pull/52/files/22f37a82..36695095

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=03-04

  Stats: 1344 lines in 17 files changed: 604 ins; 704 del; 36 mod
  Patch: https://git.openjdk.java.net/jdk/pull/52.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52

PR: https://git.openjdk.java.net/jdk/pull/52

Re: RFR: 8245194: Unix domain socket channel implementation [v4]

[Michael McMahon]

> Continuing this review as a PR on github with the comments below 
> incorporated. I expect there will be a few more
> iterations before integrating.
> On 06/09/2020 19:47, Alan Bateman wrote:
>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>
>>> As I mentioned the other day, I wasn't able to use the suggested method on 
>>> Windows which returns an absolute path. So,
>>> I added a method to WindowsPath which returns the path in the expected 
>>> UTF-8 encoding as a byte[]. Let me know what you
>>> think of that.
>>>
>>> There is a fair bit of other refactoring and simplification done also. Next 
>>> version is at:
>>>
>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>
>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I 
>> don't think we should be caching it as the
>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about 
>> encoding a relative path when the resolved
>> path (before encoding) is > 247 chars. The documentation on the MS site 
>> isn't very completely and I think there are a
>> number points that require clarification to fully understand how this will 
>> work with relative, directly relative and
>> drive relative paths.
>>
> 
> Maybe it would be better to just use the path returned from toString() and do 
> the conversion to UTF-8 externally. That
> would leave WindowsPath unchanged.
>> In the same area, the new PathUtil is a bit inconsistent with the existing 
>> provider code. One suggestion is to add a
>> method to AbstractFileSystemProvider instead. That is the base class the 
>> platform providers and would be a better place
>> to get the file path in bytes.
>>
> 
> Okay, I gave the method a name that is specific to Unix domain sockets 
> because this UTF-8 strangeness is not likely to
> be useful by other components.
>> One other comment on the changes to the file system provider it should be 
>> okay to change UnixUserPrinipals ad its
>> fromUid and fromGid method to be public. This would mean that the patch 
>> shouldn't need to add UnixUserGroup (the main
>> issue is that class is that it means we end up with two classes with static 
>> factory methods doing the same thing).
> 
> Okay, that does simplify it a bit.
> 
> Thanks,
> Michael.
> 
>> -Alan.
>>
>>
>>
>>
>>
>>

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  unixdomainchannels: updates from review of Sept 11 2020

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/52/files
  - new: https://git.openjdk.java.net/jdk/pull/52/files/110b45c0..22f37a82

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=02-03

  Stats: 40 lines in 7 files changed: 16 ins; 7 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/52.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52

PR: https://git.openjdk.java.net/jdk/pull/52

Re: RFR: 8245194: Unix domain socket channel implementation [v3]

[Michael McMahon]

> Continuing this review as a PR on github with the comments below 
> incorporated. I expect there will be a few more
> iterations before integrating.
> On 06/09/2020 19:47, Alan Bateman wrote:
>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>
>>> As I mentioned the other day, I wasn't able to use the suggested method on 
>>> Windows which returns an absolute path. So,
>>> I added a method to WindowsPath which returns the path in the expected 
>>> UTF-8 encoding as a byte[]. Let me know what you
>>> think of that.
>>>
>>> There is a fair bit of other refactoring and simplification done also. Next 
>>> version is at:
>>>
>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>
>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I 
>> don't think we should be caching it as the
>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about 
>> encoding a relative path when the resolved
>> path (before encoding) is > 247 chars. The documentation on the MS site 
>> isn't very completely and I think there are a
>> number points that require clarification to fully understand how this will 
>> work with relative, directly relative and
>> drive relative paths.
>>
> 
> Maybe it would be better to just use the path returned from toString() and do 
> the conversion to UTF-8 externally. That
> would leave WindowsPath unchanged.
>> In the same area, the new PathUtil is a bit inconsistent with the existing 
>> provider code. One suggestion is to add a
>> method to AbstractFileSystemProvider instead. That is the base class the 
>> platform providers and would be a better place
>> to get the file path in bytes.
>>
> 
> Okay, I gave the method a name that is specific to Unix domain sockets 
> because this UTF-8 strangeness is not likely to
> be useful by other components.
>> One other comment on the changes to the file system provider it should be 
>> okay to change UnixUserPrinipals ad its
>> fromUid and fromGid method to be public. This would mean that the patch 
>> shouldn't need to add UnixUserGroup (the main
>> issue is that class is that it means we end up with two classes with static 
>> factory methods doing the same thing).
> 
> Okay, that does simplify it a bit.
> 
> Thanks,
> Michael.
> 
>> -Alan.
>>
>>
>>
>>
>>
>>

Michael McMahon has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev
excludes the unrelated changes brought in by the merge/rebase. The pull request 
contains four additional commits since
the last revision:

 - Merge branch 'master' into unixdomainchannels
 - Made some changes relating to selection of the local directory where 
automatically
   bound server sockets are created. After this change it is no longer necessary
   to specify the location in individual tests.
 - unixdomainchannels: fixing compile error on Windows and Alan's review 
comment this morning
 - unixdomainchannels: initial commit from hg sandbox with changes from Alan's 
email 06/09/2020

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/52/files
  - new: https://git.openjdk.java.net/jdk/pull/52/files/4d08c15a..110b45c0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=01-02

  Stats: 13647 lines in 426 files changed: 7255 ins; 4331 del; 2061 mod
  Patch: https://git.openjdk.java.net/jdk/pull/52.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52

PR: https://git.openjdk.java.net/jdk/pull/52

Re: RFR: 8245194: Unix domain socket channel implementation

[Michael McMahon]

On Mon, 7 Sep 2020 12:05:07 GMT, Michael McMahon  wrote:

> Continuing this review as a PR on github with the comments below 
> incorporated. I expect there will be a few more
> iterations before integrating.
> On 06/09/2020 19:47, Alan Bateman wrote:
>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>
>>> As I mentioned the other day, I wasn't able to use the suggested method on 
>>> Windows which returns an absolute path. So,
>>> I added a method to WindowsPath which returns the path in the expected 
>>> UTF-8 encoding as a byte[]. Let me know what you
>>> think of that.
>>>
>>> There is a fair bit of other refactoring and simplification done also. Next 
>>> version is at:
>>>
>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>
>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I 
>> don't think we should be caching it as the
>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about 
>> encoding a relative path when the resolved
>> path (before encoding) is > 247 chars. The documentation on the MS site 
>> isn't very completely and I think there are a
>> number points that require clarification to fully understand how this will 
>> work with relative, directly relative and
>> drive relative paths.
>>
> 
> Maybe it would be better to just use the path returned from toString() and do 
> the conversion to UTF-8 externally. That
> would leave WindowsPath unchanged.
>> In the same area, the new PathUtil is a bit inconsistent with the existing 
>> provider code. One suggestion is to add a
>> method to AbstractFileSystemProvider instead. That is the base class the 
>> platform providers and would be a better place
>> to get the file path in bytes.
>>
> 
> Okay, I gave the method a name that is specific to Unix domain sockets 
> because this UTF-8 strangeness is not likely to
> be useful by other components.
>> One other comment on the changes to the file system provider it should be 
>> okay to change UnixUserPrinipals ad its
>> fromUid and fromGid method to be public. This would mean that the patch 
>> shouldn't need to add UnixUserGroup (the main
>> issue is that class is that it means we end up with two classes with static 
>> factory methods doing the same thing).
> 
> Okay, that does simplify it a bit.
> 
> Thanks,
> Michael.
> 
>> -Alan.
>>
>>
>>
>>
>>
>>

> _Mailing list message from [Alan Bateman](mailto:alan.bate...@oracle.com) on
> [nio-dev](mailto:nio-...@openjdk.java.net):_
> On 07/09/2020 13:14, Michael McMahon wrote:
> 
> > > In the same area, the new PathUtil is a bit inconsistent with the 
> > > existing provider code. One suggestion is to add a
> > > method to AbstractFileSystemProvider instead. That is the base class the 
> > > platform providers and would be a better place
> > > to get the file path in bytes.
> > 
> > 
> > Okay, I gave the method a name that is specific to Unix domain sockets 
> > because this UTF-8 strangeness is not likely to
> > be useful by other components.
> 
> The file system provider shouldn't know anything about Unix domain
> sockets so I prefer not to have "UnixDomain" in the name of this method.
> I also would prefer if null is an exception so that it is consistent
> with the other methods.
> 

I'm not sure what to call the method then. It returns a UTF-8 string
converted to bytes on Windows and the output of getByteArrayForSysCalls on Unix.
It could be called getByteArrayUTF8 on Windows, but that would not
be right on Unix.

Since sockets are file system entities, why not have socket in the method name?

What about getByteArrayForSocket() ?

> I think it would be useful to also summarize how the bind/connect works
> on Windows. For example, suppose the working directory is 240 characters
> and the UnixDomainSocketAddress is to a simple relative path of 20
> characters. If I understand correctly, the proposal will encode the
> simple relative path (not the resolved absolute path) to bytes using
> UTF-8 so it will probably be 20+ bytes in this case.

Right

> This hould be okay
> but inconsistent with the rest of? file system implementation which will
> use the long form. Also if the name is long then it won't use the long
> path prefix (\?) but instead rely on failure because the resulting
> sequence of bytes when encoded is > 100, is that correct?
> 

Yes, that is how it is working currently. We check the length in native code
just before calling bind(). Whether the long path prefix is present or not
won't matter because the path will not be used once it is longer than ~100 
bytes.

Michael.
> -Alan.

-

PR: https://git.openjdk.java.net/jdk/pull/52

Re: RFR: 8245194: Unix domain socket channel implementation [v2]

[Michael McMahon]

> Continuing this review as a PR on github with the comments below 
> incorporated. I expect there will be a few more
> iterations before integrating.
> On 06/09/2020 19:47, Alan Bateman wrote:
>> On 26/08/2020 15:24, Michael McMahon wrote:
>>>
>>> As I mentioned the other day, I wasn't able to use the suggested method on 
>>> Windows which returns an absolute path. So,
>>> I added a method to WindowsPath which returns the path in the expected 
>>> UTF-8 encoding as a byte[]. Let me know what you
>>> think of that.
>>>
>>> There is a fair bit of other refactoring and simplification done also. Next 
>>> version is at:
>>>
>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>>
>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I 
>> don't think we should be caching it as the
>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about 
>> encoding a relative path when the resolved
>> path (before encoding) is > 247 chars. The documentation on the MS site 
>> isn't very completely and I think there are a
>> number points that require clarification to fully understand how this will 
>> work with relative, directly relative and
>> drive relative paths.
>>
> 
> Maybe it would be better to just use the path returned from toString() and do 
> the conversion to UTF-8 externally. That
> would leave WindowsPath unchanged.
>> In the same area, the new PathUtil is a bit inconsistent with the existing 
>> provider code. One suggestion is to add a
>> method to AbstractFileSystemProvider instead. That is the base class the 
>> platform providers and would be a better place
>> to get the file path in bytes.
>>
> 
> Okay, I gave the method a name that is specific to Unix domain sockets 
> because this UTF-8 strangeness is not likely to
> be useful by other components.
>> One other comment on the changes to the file system provider it should be 
>> okay to change UnixUserPrinipals ad its
>> fromUid and fromGid method to be public. This would mean that the patch 
>> shouldn't need to add UnixUserGroup (the main
>> issue is that class is that it means we end up with two classes with static 
>> factory methods doing the same thing).
> 
> Okay, that does simplify it a bit.
> 
> Thanks,
> Michael.
> 
>> -Alan.
>>
>>
>>
>>
>>
>>

Michael McMahon has updated the pull request incrementally with one additional 
commit since the last revision:

  Made some changes relating to selection of the local directory where 
automatically
  bound server sockets are created. After this change it is no longer necessary
  to specify the location in individual tests.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/52/files
  - new: https://git.openjdk.java.net/jdk/pull/52/files/6d164c91..4d08c15a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=00-01

  Stats: 119 lines in 10 files changed: 89 ins; 18 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/52.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52

PR: https://git.openjdk.java.net/jdk/pull/52

RFR: 8245194: Unix domain socket channel implementation

[Michael McMahon]

Continuing this review as a PR on github with the comments below incorporated. 
I expect there will be a few more
iterations before integrating.

On 06/09/2020 19:47, Alan Bateman wrote:
> On 26/08/2020 15:24, Michael McMahon wrote:
>>
>> As I mentioned the other day, I wasn't able to use the suggested method on 
>> Windows which returns an absolute path. So,
>> I added a method to WindowsPath which returns the path in the expected UTF-8 
>> encoding as a byte[]. Let me know what you
>> think of that.
>>
>> There is a fair bit of other refactoring and simplification done also. Next 
>> version is at:
>>
>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/
>>
> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I 
> don't think we should be caching it as the
> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about 
> encoding a relative path when the resolved
> path (before encoding) is > 247 chars. The documentation on the MS site isn't 
> very completely and I think there are a
> number points that require clarification to fully understand how this will 
> work with relative, directly relative and
> drive relative paths.
>

Maybe it would be better to just use the path returned from toString() and do 
the conversion to UTF-8 externally. That
would leave WindowsPath unchanged. I also changed the

> In the same area, the new PathUtil is a bit inconsistent with the existing 
> provider code. One suggestion is to add a
> method to AbstractFileSystemProvider instead. That is the base class the 
> platform providers and would be a better place
> to get the file path in bytes.
>

Okay, I gave the method a name that is specific to Unix domain sockets because 
this UTF-8 strangeness is not likely to
be useful by other components.

> One other comment on the changes to the file system provider it should be 
> okay to change UnixUserPrinipals ad its
> fromUid and fromGid method to be public. This would mean that the patch 
> shouldn't need to add UnixUserGroup (the main
> issue is that class is that it means we end up with two classes with static 
> factory methods doing the same thing).

Okay, that does simplify it a bit.

Thanks,
Michael.

> -Alan.
>
>
>
>
>
>

-

Commit messages:
 - unixdomainchannels: fixing compile error on Windows and Alan's review 
comment this morning
 - unixdomainchannels: initial commit from hg sandbox with changes from Alan's 
email 06/09/2020

Changes: https://git.openjdk.java.net/jdk/pull/52/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8245194
  Stats: 6960 lines in 87 files changed: 5902 ins; 763 del; 295 mod
  Patch: https://git.openjdk.java.net/jdk/pull/52.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52

PR: https://git.openjdk.java.net/jdk/pull/52

Re: Undocumented exceptions in java.net.http.HttpClient.newHttpClient()

[Michael McMahon]

Hi,

Yes, that report probably should not have been closed as it is 
definitely an issue.


I will re-open it.

Michael

On 22/06/2020 16:39, Sebastian Stenzel wrote:

Certain users of my software run into problems with HttpClient.newHttpClient() 
on JDK 14.0.1 and I don't feel like I can handle it properly without catching 
Errors.

Calling said method fails when encountering Selector.open(), which in its 
Windows-specific implementation relies on loopback-TCP connections. These 
connections can, of course, be blocked by the OS or, as in the case of my user, 
by some anti-malware tool.

I can't argue whether it is good or bad implementing the selector the way it is, however 
I'm pretty sure that commenting exceptions being "unlikely" without further 
explanation isn't exactly the best practice for this case:

https://github.com/openjdk/jdk/blob/5adfaa39866f3127000f0779158c65afe1d24007/src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java#L309-L314
 


Since my related bug report (JDK-8247996) was turned down (maybe the tester 
forgot to or chose not to block the socket connection), I'd like to ask if it 
is at least possible to document the fact that creating an HttpClient can have 
the side effect of instantaneously creating a loopback connection. I would 
prefer to even throw a checked exception, but this would break the API.

Happy to create a PR, but I'd like to do this the right way, therefore starting 
this discussion.

Re: RFC: 8132359: JarURLConnection.getJarFile() resource leak when file is not found

[Michael McMahon]

Hi Alex,

(and redirecting the thread to net-dev)

It looks like a straight forward solution and perhaps the compatibility test
could be challenged on the basis of reliance on implementation behavior 
rather than the spec.


But, more important I think is the behavior change of the fix itself and 
that will require
careful review which we can't commit to immediately. We will try and get 
back to you

about it in a week or so.

Thanks,

Michael.

On 14/03/2020 00:08, Alex Kashchenko wrote:

Hi,

Based on these maillist threads:

https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-March/065076.html 

https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-November/063643.html 



I am looking for comments and suggestions, whether the following 
change to JarURLConnection.getJarFile() behaviour may be acceptable:


If, during connect() call, jarFile itself was created successfully, 
but access to (non-existent) jarEntry failed - return this jarFile to 
caller instead of throwing exception.


bug: https://bugs.openjdk.java.net/browse/JDK-8132359
webrev: http://cr.openjdk.java.net/~akasko/jdk/8132359/webrev.00/

This change also allows to fix JDK-8232854 with the minimal change to 
URLClassPath (included with the patch).


This change doesn't cause regression failures in java/net.

This change causes one compatibility failure, when getManifest() 
doesn't throw expected IOException when URL points to non-existent 
class inside JAR.

Re: RFR 8210318: idea.sh script doesn't work on Mac

[Michael McMahon]

The change looks fine to me Maurizio. Maybe you could append a .$$ to 
the temporary
file name to make it less likely to overwrite something that already 
exists in /tmp


- Michael.

On 03/09/2018, 13:39, Maurizio Cimadamore wrote:

Hi,
following the latest updates to the idea.sh script, Mac users reported 
issues - mostly having to do with usage of 'sed' - more specifically:


* sed -i option is not portable - it has different formats in Mac vs. 
Linux. This patch does without -i, by moving the replaced file onto a 
temporary file, then moving such file on top of the template file in a 
subsequent step. This should be more robust.


* sed doesn't like newlines in replaced text in Mac. I've thus omitted 
the newline from the SOURCE template - as that was mostly cosmetic.


Thanks for Michael McMahon to report (and figure out how to deal with) 
these issues, and to Alan Bateman for testing the patch.


I also fixed another minor glitch, this time in the langtools-only 
template - which was still referring to the old ant file location in 
the various run configuration.


Webrev:

http://cr.openjdk.java.net/~mcimadamore/8210318/

Cheers
Maurizio

Re: RFR[10]:8159526 Deprivilege jdk.httpserver

[Michael McMahon]

Looks good Vyom.

- Michael

On 12/09/2017, 09:46, vyom tewari wrote:



On Tuesday 12 September 2017 02:12 PM, Alan Bateman wrote:

On 12/09/2017 09:06, vyom tewari wrote:

Hi,

Please review the below code change.

BugId: https://bugs.openjdk.java.net/browse/JDK-8159526

Webrev-1: 
http://cr.openjdk.java.net/~vtewari/8159526/jdk/webrev/index.html


Webrev-2: 
http://cr.openjdk.java.net/~vtewari/8159526/root/webrev/index.html


Code change will De-privilege jdk.httpserver, we gave 
"jdk.httpserver" all permission for now.
Moving jdk.httpserver to the platform class loader looks fine. Are 
you planning a second phase to identify the permissions needed so 
that it doesn't have to be granted AllPermission?

yes, i will file a separate issue for this.

Vyom


-Alan

Re: SubmissionPublisher - Subscriber#onComplete() not invoked when publisher is closed

[Michael McMahon]

Maybe the spec could be tighter around this, but it's not unreasonable 
that there is a
delay in receiving onComplete() notification because of the subscriber 
controlled flow control.
Notifying onError() is not subject to flow control; so you might expect 
that it would be triggered immediately.


Michael.

On 21/02/2017, 11:32, Pavel Bucek wrote:
SubmissionPublisher#closeExceptionally does trigger 
Subscriber#onError, but based on javadoc, I cannot really be sure that 
it will be called, since it contains exactly the same wording as 
SubmissionPublisher#close


/** * Unless already closed, issues {@link * 
Flow.Subscriber#onError(Throwable) onError} signals to current * 
subscribers with the given error, and disallows subsequent * attempts 
to publish. Future subscribers also receive the given * error. Upon 
return, this method does NOTguarantee * that all subscribers 
have yet completed. * * @param error the {@code onError} argument sent 
to subscribers * @throws NullPointerException if error is null */


So, Pavel, if that is not a bug, how can the SubmissionPublisher be 
closed in a way that subscribers are notified?


Thanks for the link to the other mailing list - do I understand it 
correctly that I should move this thread there?


Thanks and regards,
Pavel


On 21/02/2017 12:15, Pavel Rappo wrote:
I believe, the most appropriate place for concurrency-related 
questions is


 http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest

As for the question itself. I don't think this behaviour is a bug.
SubmissionPublisher.close() seems to be a graceful way of shutting 
down (in
contrast with SubmissionPublisher.closeExceptionally()), akin to 
putting a EOF

on an input stream.

My reading of the javadoc is that after SubmissionPublisher.close has 
been
invoked, the publisher will no longer accept any attempts to publish 
items and

will call Subscriber.onClose() *eventually*.


On 21 Feb 2017, at 09:24, Pavel Bucek  wrote:

there is a formatting issue in the code snippet, publisher.close() 
should be on the new line:


{
SubmissionPublisher publisher =new SubmissionPublisher<>();
publisher.subscribe(new Flow.Subscriber() {
@Override public void onSubscribe(Flow.Subscription 
subscription) { }


@Override public void onNext(String item) { }

@Override public void onError(Throwable throwable) {
System.out.println("onError()");
}

@Override public void onComplete() {
System.out.println("onComplete()");
}
});
publisher.submit("item");// if this is commented out, 
#onComplete is invoked.


publisher.close();
}


On 21/02/2017 10:16, Pavel Bucek wrote:

Hi all,

firstly - please let me know if this is is a wrong place to send 
this; I wasn't able to find list specific to concurrency.


Consider following example:

{
SubmissionPublisher publisher =new 
SubmissionPublisher<>();

publisher.subscribe(new Flow.Subscriber() {
@Override public void onSubscribe(Flow.Subscription 
subscription) { }


@Override public void onNext(String item) { }

@Override public void onError(Throwable throwable) {
System.out.println("onError()");
}

@Override public void onComplete() {
System.out.println("onComplete()");
}
});
publisher.submit("item");// if this is commented out, 
#onComplete is invoked. publisher.close();

}

I'd expect that Subscriber#onComplete is invoked after calling 
publisher.close(), but it is not happening. Curiously, when I 
comment out 'publisher.submit("item")', Subscriber#onComplete is 
indeed invoked.


SubmissionPublisher#close() javadoc says:

/** * Unless already closed, issues {@link * 
Flow.Subscriber#onComplete() onComplete} signals to current * 
subscribers, and disallows subsequent attempts to publish. * Upon 
return, this method does NOTguarantee that all * 
subscribers have yet completed. */


So it seems like it will be invoked in different thread or 
something like that, but it is not invoked ever (or more precisely 
- not during 10 second after the publisher is closed. There is 
nothing else running on that particular jvm instance).


Also, publisher#isClosed() returns true and 
publisher#getNumberOfSubscribers() returns 0.


I'm using Java(TM) SE Runtime Environment (build 
9-ea+157-jigsaw-nightly-h6115-20170219)


What am I doing wrong?

Thanks and regards, Pavel

Re: SubmissionPublisher - Subscriber#onComplete() not invoked when publisher is closed

[Michael McMahon]

On 21/02/2017, 11:15, Pavel Rappo wrote:

I believe, the most appropriate place for concurrency-related questions is

 http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest

As for the question itself. I don't think this behaviour is a bug.
SubmissionPublisher.close() seems to be a graceful way of shutting down (in
contrast with SubmissionPublisher.closeExceptionally()), akin to putting a EOF
on an input stream.

My reading of the javadoc is that after SubmissionPublisher.close has been
invoked, the publisher will no longer accept any attempts to publish items and
will call Subscriber.onClose() *eventually*.

Yes, you are right. You can close the SubmissionPublisher, then
call Subscription.request() and then receive notification of the item.

- Michael.

On 21 Feb 2017, at 09:24, Pavel Bucek  wrote:

there is a formatting issue in the code snippet, publisher.close() should be on 
the new line:

{
SubmissionPublisher  publisher =new SubmissionPublisher<>();
publisher.subscribe(new Flow.Subscriber() {
@Override public void onSubscribe(Flow.Subscription subscription) { }

@Override public void onNext(String item) { }

@Override public void onError(Throwable throwable) {
System.out.println("onError()");
}

@Override public void onComplete() {
System.out.println("onComplete()");
}
});
publisher.submit("item");// if this is commented out, #onComplete is 
invoked.

publisher.close();
}


On 21/02/2017 10:16, Pavel Bucek wrote:

Hi all,

firstly - please let me know if this is is a wrong place to send this; I wasn't 
able to find list specific to concurrency.

Consider following example:

{
SubmissionPublisher  publisher =new SubmissionPublisher<>();
publisher.subscribe(new Flow.Subscriber() {
@Override public void onSubscribe(Flow.Subscription subscription) { }

@Override public void onNext(String item) { }

@Override public void onError(Throwable throwable) {
System.out.println("onError()");
}

@Override public void onComplete() {
System.out.println("onComplete()");
}
});
publisher.submit("item");// if this is commented out, #onComplete is 
invoked. publisher.close();
}

I'd expect that Subscriber#onComplete is invoked after calling publisher.close(), but it 
is not happening. Curiously, when I comment out 'publisher.submit("item")', 
Subscriber#onComplete is indeed invoked.

SubmissionPublisher#close() javadoc says:

/** * Unless already closed, issues {@link * Flow.Subscriber#onComplete() onComplete} 
signals to current * subscribers, and disallows subsequent attempts to publish. * Upon 
return, this method doesNOTguarantee that all * subscribers have yet 
completed. */

So it seems like it will be invoked in different thread or something like that, 
but it is not invoked ever (or more precisely - not during 10 second after the 
publisher is closed. There is nothing else running on that particular jvm 
instance).

Also, publisher#isClosed() returns true and publisher#getNumberOfSubscribers() 
returns 0.

I'm using Java(TM) SE Runtime Environment (build 
9-ea+157-jigsaw-nightly-h6115-20170219)

What am I doing wrong?

Thanks and regards, Pavel

Re: SubmissionPublisher - Subscriber#onComplete() not invoked when publisher is closed

[Michael McMahon]

Sounds like a bug. It seems like the fact there isn't a call to 
Subscription.request()
is what causes the problem. But by my reading of the spec, 
Subscriber.onComplete()
should still be called, as it is known that " no additional Subscriber 
method invocations will occur".


- Michael.

On 21/02/2017, 09:24, Pavel Bucek wrote:
there is a formatting issue in the code snippet, publisher.close() 
should be on the new line:


{
SubmissionPublisher publisher =new SubmissionPublisher<>();
publisher.subscribe(new Flow.Subscriber() {
@Override public void onSubscribe(Flow.Subscription 
subscription) { }


@Override public void onNext(String item) { }

@Override public void onError(Throwable throwable) {
System.out.println("onError()");
}

@Override public void onComplete() {
System.out.println("onComplete()");
}
});
publisher.submit("item");// if this is commented out, #onComplete 
is invoked.


publisher.close();
}


On 21/02/2017 10:16, Pavel Bucek wrote:

Hi all,

firstly - please let me know if this is is a wrong place to send 
this; I wasn't able to find list specific to concurrency.


Consider following example:

{
SubmissionPublisher publisher =new SubmissionPublisher<>();
publisher.subscribe(new Flow.Subscriber() {
@Override public void onSubscribe(Flow.Subscription 
subscription) { }


@Override public void onNext(String item) { }

@Override public void onError(Throwable throwable) {
System.out.println("onError()");
}

@Override public void onComplete() {
System.out.println("onComplete()");
}
});
publisher.submit("item");// if this is commented out, #onComplete 
is invoked. publisher.close();

}

I'd expect that Subscriber#onComplete is invoked after calling 
publisher.close(), but it is not happening. Curiously, when I comment 
out 'publisher.submit("item")', Subscriber#onComplete is indeed invoked.


SubmissionPublisher#close() javadoc says:

/** * Unless already closed, issues {@link * 
Flow.Subscriber#onComplete() onComplete} signals to current * 
subscribers, and disallows subsequent attempts to publish. * Upon 
return, this method does NOTguarantee that all * subscribers 
have yet completed. */


So it seems like it will be invoked in different thread or something 
like that, but it is not invoked ever (or more precisely - not during 
10 second after the publisher is closed. There is nothing else 
running on that particular jvm instance).


Also, publisher#isClosed() returns true and 
publisher#getNumberOfSubscribers() returns 0.


I'm using Java(TM) SE Runtime Environment (build 
9-ea+157-jigsaw-nightly-h6115-20170219)


What am I doing wrong?

Thanks and regards, Pavel

Re: java.net.http.ExecutorWrapper "memory fence"

[Michael McMahon]

Thanks Aleksey,

I will take care of it.

- Michael

On 09/03/16 11:34, Aleksey Shipilev wrote:

Alan mentioned I should have sent this to net-dev@. Instead, I submitted
a new bug:
  https://bugs.openjdk.java.net/browse/JDK-8151505

-Aleksey

On 03/09/2016 02:06 PM, Aleksey Shipilev wrote:

Hi,

In recently committed java.net.http.ExecutorWrapper, there is a
synchronize() method [1], which is used as "memory fence" [2]:

  public synchronized void synchronize() {}

  public void execute(Runnable r, Supplier
ctxSupplier) {
synchronize();
Runnable r1 = () -> {
  try {
r.run();
  } catch (Throwable t) {
Log.logError(t);
  }
};

...

executor.execute(r1);
  }


How's that supposed to work? Is that supposed to guard from bad Runnable
$r?

The problem is, once you get $r via the race, there is no way to recover
with local synchronization (IOW: There is no way to sanitize a racy
input, once it happened. Races are bad like that) And if $r got to you
properly, you don't need to do anything special too (IOW: API may as
well assume it is coming from the current thread).

Therefore, I think synchronize() method there is superfluous.

In fact, assuming that a synchronized method has any *detached* memory
semantics is wrong too -- compilers are known to elide associated
fences. E.g. if ExecutorWrapper is known to never escape a thread, or a
single thread locks on it, and biases a lock towards itself.

Thanks,
-Aleksey

[1]
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/e0da6c2a5c32/src/java.httpclient/share/classes/java/net/http/ExecutorWrapper.java#l74
[2]
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/e0da6c2a5c32/src/java.httpclient/share/classes/java/net/http/ExecutorWrapper.java#l77

Re: [9] RFR: 8151182: HttpHeaders.allValues should return unmodifiable List as per JavaDoc

[Michael McMahon]

Yes, there is a mutability test already. We will have to fix the thread 
safety problem

(and also the fact HttpHeaders1 was left public by mistake). Probably will
separate the mutable and immutable types completely. Vaibhav, if you'd 
like to do it,
you can define a package private implementation of HttpHeaders whose 
constructor
takes a HttpHeadersImpl and uses final fields to ensure thread safety, 
rather than

using makeUnmodifiable(). I'll contact you with some other ideas.

I'll open a new report on this point and we probably should review on 
net-dev


- Michael.


On 04/03/16 14:05, Paul Sandoz wrote:

Hi Vaibhav,

This will not work, as Claes points out. You also might wanna check if there 
are tests in place asserting unmodfiablity, if there are none you could add 
some. Michael can point you in the right direction.

Follow the trail of HttpHeaders1.makeUnmodifiable(), which is used to 
transition map values from mutable to unmodifiable:

 @Override
 public void makeUnmodifiable() {
 if (isUnmodifiable)
 return;

 Set keys = new HashSet<>(headers.keySet());
 for (String key : keys) {
 List values = headers.remove(key);
 if (values != null) {
 headers.put(key, Collections.unmodifiableList(values));
 }
 }
 isUnmodifiable = true;
 }

In fact duplication of the key set can be avoided if one does this:

   Iterator> ie = headers.entrySet().iterator();
   while (ie.hashNext()) {
 Map.Entry<…> e = ie.next();
 if (e.getValue() != null) {
   e.setValue(Collections.unmodifiableList(e.getValue()));
 }
 else {
   ie.remove();
 }
   }

However, i suspect this could be simplified to:

   headers.replaceAll((k, v) -> Collections.unmodifiableList(v)))

If there are never any explicit null values placed in the map, which should be 
the case as that is really an anti-pattern.

Also this:

 private List getOrCreate(String name) {
 List l = headers.get(name);
 if (l == null) {
 l = new LinkedList<>();
 headers.put(name, l);
 }
 return l;
 }

can be replaced with this:

   return headers.computeIfAbsent(name, k -> new LinkedList<>());

Paul.


On 4 Mar 2016, at 14:19, vaibhav x.choudhary  
wrote:

Hi,

Please review :-

Review Link :- http://cr.openjdk.java.net/~ntv/vaibhav/JDK8151182/webrev.00/
Bug ID: https://bugs.openjdk.java.net/browse/JDK-8151182

--
Thank You,
Vaibhav Choudhary
http://blogs.oracle.com/vaibhav

Re: SO_REUSEPORT feature support in JDK 9 for socket communication

[Michael McMahon]

On 22/10/15 14:24, Alan Bateman wrote:



On 22/10/2015 14:04, Roger Riggs wrote:

Hi Sandhya,

The folks on net-...@openjdk.java.net will be interested too.

Yes, net-dev is the best list for this.

One other thing to mention is the SocketOption interface and the 
setOption/getOption methods. This allows for platform or JDK-specific 
specific socket options, it also allows it to be implemented by the 
NIO SocketChannel and friends.


-Alan


and there is the jdk.net API which extends this to the java.net socket 
types.


I think a Java SE API would be fine if it is widely and consistently 
implemented
across all the reference platforms and if it does not cause 
compatibility issues.


If it only works on some platforms then maybe jdk.net could be the place 
for it.


- Michael

Re: RFR 9: JDK-8132705 : Refactor SharedSecrets in sun.misc.JavaNetAccess

[Michael McMahon]

Looks good Roger. Only minor quibble would be the name of the new type

JavaInetAddressAccess could be JavaNetInetAddressAccess to be consistent.

Michael


On 30/07/15 15:49, Roger Riggs wrote:
Please review this refactoring of SharedSecret initialization to 
create and

InetAddressAccess access that is independent of the initialization
of JavaNetAccess in URLClassLoader.

(jprt in progress)

Webrev:
 http://cr.openjdk.java.net/~rriggs/webrev-inetaccess-8132705/

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8132705

Roger

Re: RFR(xs): 6991580: IPv6 Nameservers in resolv.conf throws NumberFormatException

[Michael McMahon]

On 21/04/15 14:56, Alan Bateman wrote:


On 20/04/2015 18:32, Severin Gehwolf wrote:

:
OK fixed:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6991580/webrev.02/

FWIW, I don't think the test needs IP addresses of DNS servers to be
functional, though. All it really does is passing it to
InetAddress.getByName(). Prior this patch DnsClient fails to parse the
IP literals.

Can the test be moved to jdk/test/com/sun/jndi/dns as that seems to be 
where the other tests are? Also I assume this will need a copyright 
header before it is pushed.


-Alan


I had pushed it already. So, I'll fix it in this webrev

http://cr.openjdk.java.net/~michaelm/8078276/webrev.1/

Thanks,
Michael

Re: RFR(xs): 6991580: IPv6 Nameservers in resolv.conf throws NumberFormatException

[Michael McMahon]

On 20/04/15 18:32, Severin Gehwolf wrote:

On Mon, 2015-04-20 at 12:24 -0400, Andrew Hughes wrote:

- Original Message -

Adding in net-dev.

On Mon, 2015-04-20 at 14:02 +0200, Severin Gehwolf wrote:

Hi,

Could I please get a review and a sponsor for the following patch?

The issue is that JDK's internal /etc/resolv.conf nameserver parsing
does not properly account for IPv6 addresses on Linux/Unix. While the
code in com.sun.jndi.dns.DnsClient seems to support IPv6 addresses
passed in via the servers list in the constructor it expects IPv6
addresses to be wrapped in '[' and ']' respectively. However,
sun.net.dns.ResolverConfigurationImpl does no wrapping of IPv6 addresses
when parsing "nameserver" keywords from /etc/resolv.conf, thus breaking
DnsClient's contract.

The fix is to properly wrap IPv6 literal addresses in '[' and ']' if not
already wrapped.

Bug: https://bugs.openjdk.java.net/browse/JDK-6991580
Webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6991580/webrev.01/

Testing done: Added regression test fails for unpatched 9 and passes for
patched 9.

Thanks,
Severin






It'd be better to provide public nameservers in the test comment so everyone
is able to run this test if they update /etc/resolv.conf appropriately.

OK fixed:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-6991580/webrev.02/

FWIW, I don't think the test needs IP addresses of DNS servers to be
functional, though. All it really does is passing it to
InetAddress.getByName(). Prior this patch DnsClient fails to parse the
IP literals.


That's true, but no harm having a public IP address there anyway.
I can sponsor this change for you.

- Michael



Google provides some: https://developers.google.com/speed/public-dns/docs/using

$ host redhat.com 2001:4860:4860::
Using domain server:
Name: 2001:4860:4860::
Address: 2001:4860:4860::#53
Aliases:

redhat.com has address 209.132.183.105

On my F21 machine:
$ host redhat.com 2001:4860:4860::
;; connection timed out; no servers could be reached

But the test still works with a patched JDK:
[...]
DEBUG: 'nameserver = 127.0.0.1'
DEBUG: 'nameserver = [2001:4860:4860::]'
DEBUG: ==> Found IPv6 address in servers list: [2001:4860:4860::]
DEBUG: 'nameserver = [::1]:5353'
DEBUG: ==> Found IPv6 address in servers list: [::1]:5353
DEBUG: 'nameserver = 127.0.0.1:5353'
PASS: Found IPv6 address and DnsClient parsed it correctly.

Contrast this to unpatched:
[...]
DEBUG: 'nameserver = 127.0.0.1'
DEBUG: 'nameserver = 2001:4860:4860::'
DEBUG: ==> Found IPv6 address in servers list: 2001:4860:4860::
DEBUG: 'nameserver = [::1]:5353'
DEBUG: ==> Found IPv6 address in servers list: [::1]:5353
DEBUG: 'nameserver = 127.0.0.1:5353'
Exception in thread "main" java.lang.RuntimeException: FAIL: Tried to parse 
non-[]-encapsulated IPv6 address.
at 
IPv6NameserverPlatformParsingTest.main(IPv6NameserverPlatformParsingTest.java:59)
Caused by: java.lang.NumberFormatException: For input string: "4860:4860::"
at 
java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
at java.lang.Integer.parseInt(Integer.java:580)
at java.lang.Integer.parseInt(Integer.java:615)
at com.sun.jndi.dns.DnsClient.(DnsClient.java:127)
at 
IPv6NameserverPlatformParsingTest.main(IPv6NameserverPlatformParsingTest.java:57)

Cheers,
Severin

JEP 110 HTTP 2 client API

[Michael McMahon]

Hi,

[this has already been posted to net-dev]

JEP 110 HTTP 2 client

in JDK 9, is defining and implementing a new API for HTTP which also 
supports
the new HTTP version 2 that has recently been working its way through 
the IETF.

The work also includes support for websockets (RFC 6455).

In fact, the majority of the API is agnostic about the HTTP protocol 
version, with only minor
configuration settings, and support for multiple responses (Http server 
push) having any direct impact.


The HTTP API is defined around three main types (HttpClient, which is 
the central
point for configuration of SSL, executor service cookie management etc), 
HttpRequest

and HttpResponse (which should be self explanatory).

Requests are sent/received either synchronously (blocking) or in a 
non-blocking (asynchronous)

mode using java.util.future.CompletableFuture.

The API docs can be seen at the link below:

http://cr.openjdk.java.net/~michaelm/httpclient/01/

All new classes and interfaces belong to the java.net package.

A prototype implementation of this API supporting HTTP/1.1, is available
in the JDK 9 sandbox forest in JEP-110-branch.

Comments welcome!

Thanks,
Michael.

RE: [9] Review request : JDK-6933879: URISyntaxException when non-alphanumeric characters are present in scope_id

[Michael Mcmahon]

Hi,

I'm afraid you have the wrong Michael McMahon.

-Original Message-
From: Konstantin Shefov 
Sent: Monday, December 15, 2014 6:16 AM
To: Ivan Gerasimov; net-...@openjdk.java.net; core-libs-dev@openjdk.java.net; 
Alan Bateman; Chris Hegarty; MICHAEL.MCMAHON
Subject: Re: [9] Review request : JDK-6933879: URISyntaxException when 
non-alphanumeric characters are present in scope_id

Gently reminder. Please review.

-Konstantin

On 11.12.2014 14:09, Konstantin Shefov wrote:
> CC'ed core-libs-dev@openjdk.java.net
>
> On 10.12.2014 18:21, Konstantin Shefov wrote:
>> Hello,
>>
>> Please, review the bug fix: 
>> https://bugs.openjdk.java.net/browse/JDK-6933879
>> Webrev: http://cr.openjdk.java.net/~kshefov/6933879/webrev.00
>>
>> It is suggested to add some more symbols allowed for scope id in IPv6 
>> URI, namely "_", "." and ":", because these symbols may be included 
>> in network interface names. Now only alphanumeric characters are 
>> allowed to be in a scope id.
>>
>> Thanks
>> -Konstantin
>

Re: RFR (S) 8057936: java.net.URLClassLoader.findClass uses exceptions in control flow

[Michael McMahon]

On 10/09/14 16:04, Claes Redestad wrote:

On 09/10/2014 04:38 PM, mark.reinh...@oracle.com wrote:

New webrev: http://cr.openjdk.java.net/~redestad/8057936/webrev.4/

Looks fine, but when an exception declaration is on its own line then
the opening brace of the method should be on its own line too, as in
the original:

 protected Class findClass(final String name)
 throws ClassNotFoundException
 {


Original had weird 5 space indentation though:

http://cr.openjdk.java.net/~redestad/8057936/webrev.6/

Will need a sponsor for this, if it looks good to everyone.

/Claes



I'll sponsor that for you.

Michael



- Mark

Re: RFR (S) 8057936: java.net.URLClassLoader.findClass uses exceptions in control flow

[Michael McMahon]

or how about just returning a null Class from the privileged block
instead of the new result type only in the case where 
URLClassPath.getResource() returns null?

That's the main "normal" case where the resource doesn't exist, I think.

If defineClass() throws an IOException, then that is more likely to be a 
"genuine" exception


- Michael

diff --git a/src/java.base/share/classes/java/net/URLClassLoader.java 
b/src/java.base/share/classes/java/net/URLClassLoader.java

--- a/src/java.base/share/classes/java/net/URLClassLoader.java
+++ b/src/java.base/share/classes/java/net/URLClassLoader.java
@@ -356,8 +356,9 @@
 protected Class findClass(final String name)
  throws ClassNotFoundException
 {
+final Class result;
 try {
-return AccessController.doPrivileged(
+result = AccessController.doPrivileged(
 new PrivilegedExceptionAction>() {
 public Class run() throws ClassNotFoundException {
 String path = name.replace('.', 
'/').concat(".class");

@@ -369,13 +370,17 @@
 throw new ClassNotFoundException(name, e);
 }
 } else {
-throw new ClassNotFoundException(name);
+return null;
 }
 }
 }, acc);
 } catch (java.security.PrivilegedActionException pae) {
 throw (ClassNotFoundException) pae.getException();
 }
+if (result == null) {
+throw new ClassNotFoundException(name);
+}
+return result;
 }

 /*

On 10/09/14 11:55, Ivan Gerasimov wrote:


If a lambda were used instead of an anonymous class, it would save us 
one line of code :-)


Sincerely yours,
Ivan

On 10.09.2014 14:11, Claes Redestad wrote:

Hi,

please review this simple patch to avoid raising 
PrivilegedActionExceptions

when failing to find a class in URLClassLoader.

 bug: https://bugs.openjdk.java.net/browse/JDK-8057936
 webrev: http://cr.openjdk.java.net/~redestad/8057936/webrev.2/
/Claes

Re: sun.net.www.protocol.https.HttpsURLConnectionImpl doesn't equal to self

[Michael McMahon]

I'll file a bug for this Stanimir. Thanks for reporting it.
Should be able to fix it in JDK 9 fairly promptly
and we'll see about back porting it then.

- Michael.

On 18/08/14 15:04, Stanimir Simeonoff wrote:

Hi,

As the title says there is a major bug with HttpsURLConnection as it breaks
the contract of equals. So the instance  cannot be contained (mostly
removed) in any collection reliably. (Concurrent)HashMap has a provision to
test equality by reference prior calling equals, though.
Finding the bug in production is quite a ride as the http variant doesn't
exhibit the problem.

Here is a simple test case.

public static void main(String[] args) throws Throwable{
  URLConnection c = new URL("https://oracle.com";).openConnection();
  System.out.println(c.getClass().getName()+" equals self: "
+c.equals(c));
  c.getInputStream().close();
  System.out.println(c.getClass().getName()+" equals self: "
+c.equals(c));
}


The culprit is in HttpsURLConnectionImpl.equals that blindly calls
delagate.equals:

 public boolean equals(Object obj) {
 return delegate.equals(obj);
 }

It should be changed to:

 public boolean equals(Object obj) {
 return this==obj || (obj instanceof HttpsURLConnectionImpl) &&
delegate.equals( ((HttpsURLConnectionImpl)obj).delegate );
 }


The class has some other issue that involves declaring "finalize" method to
simply call delegate's dispose. The finalize method is unneeded and just
creates unnecessary link in the finalization queue + Finalizer object.

Thanks
Stanimir

RFR 8040809: '}' left in the spec for j.u.Random.doubles(..)

[Michael McMahon]

Trivial doc change to remove extraneous  '}' characters in two places

http://cr.openjdk.java.net/~michaelm/8040809/webrev.1/

Thanks,
Michael

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Michael McMahon]

On 14/02/14 18:20, Alan Bateman wrote:

On 14/02/2014 17:42, Michael McMahon wrote:

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.
The removal is good. Maybe the test should be moved to URLClassLoader 
as it doesn't make sense to have it in ClassLoaderUtil anymore.


-Alan.


I think I'd probably just delete the test since URLClassLoader.close has 
its own tests,

I thought it might be useful as a point of reference
in case there could be someone still using the sun.misc API.
Also, I think mercurial won't delete the empty directory after the file 
is gone - not that that matters very much.


Thanks
Michael

Re: RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Michael McMahon]

Thanks Joe. Will do that.

Michael

On 14/02/14 18:09, Joe Darcy wrote:

Hi Michael,

Good to see more of sun.misc go away :-)

For the test, I recommend updating the @summary and removing the 
comment about the old API.


Thanks,

-Joe

On 02/14/2014 09:42 AM, Michael McMahon wrote:

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.

Thanks
Michael

RFR: 8034853 remove sun.misc.ClassLoaderUtil

[Michael McMahon]

Could I get the following change reviewed please?

http://cr.openjdk.java.net/~michaelm/8034853/webrev.1/

The change is to remove the class sun.misc.ClassLoaderUtil.
The functionality provided by this class is now in the public
java.net.URLClassloader.close() method.

Thanks
Michael

Re: RFR: (8030875) Macros for checking and returning on exceptions

[Michael McMahon]

On 10/01/14 15:37, roger riggs wrote:

Please review:

To enable native code checking consistently for thrown exceptions,
the macros in net_util.h and java/util/jar/pack/coding.cpp are
made consolidated and promoted to jni_util.h

webrev:
http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/

[1] https://bugs.openjdk.java.net/browse/JDK-8030875


Looks fine to me

Michael.

Re: RFR: 8029451 : Tidy warnings cleanup for java.util package

[Michael McMahon]

On 06/12/13 11:44, Alan Bateman wrote:

On 05/12/2013 05:30, Sergey Lugovoy wrote:

Hi all,
please review the fix
http://cr.openjdk.java.net/~yan/8029451/webrev.01/
for
https://bugs.openjdk.java.net/browse/JDK-8029451

This patch cleanup tidy warnings for generated html documentation, 
and do

not affect the appearance of the documentation.

In java/util/zip/package.html then I see that the  tags have been 
replaced by . Is that okay in HTML 3.2? I just wonder if it might 
be better to just remove the  tags. I don't see a problem with 
changing the spacing of the list items.


-Alan


I just noticed some broken link fragments in the java.util.stream package.
Would it be worthwhile including the fix in this change, or should I 
file a separate bug?


Michael

Re: RFR: 8022213 Intermittent test failures in java/net/URLClassLoader (Add jdk/testlibrary/FileUtils.java)

[Michael McMahon]

On 07/11/13 11:34, Chris Hegarty wrote:

On 11/07/2013 11:19 AM, Michael McMahon wrote:

Chris,

Would it be useful to add some instrumentation/logging (to System.err)
if it's taking
more than one iteration to delete a file? We could end up with degraded
test performance if there is a general problem deleting files, and
otherwise would have
no way of understanding what the problem is (eg. up to 7.5 seconds are
allowed to delete the file)


I had an optional PrintStream param on delete/deleteTree, but removed 
it. If the file fails to delete, then you will see all the suppressed 
exceptions, but I agree if it fails a number of times and then 
succeeds you do not see this.


I'm not sure that it is worth adding PrintStream ( and this is why I 
removed it ) since the external interference occurs infrequently and 
does not appear to last long. I just wanted to keep this as simply as 
possible, but I could be convinced to reintroduce it.




Personally, I would have thought System.err would be okay, but maybe 
there are tests that need to run
without this kind of noise. I agree adding a PrintStream arg is not 
worth doing.



Also, just curious what is the value in throwing InterruptedException
out of the delete() method?
It seems onerous for calling code to have to catch it.


Right handling this is a pain. Since we may now be possibly sleeping 
during a delete I wanted jtreg to be able to interrupt the test and 
have the test take correct action, rather than just swallowing it. It 
is really annoying to see hung tests in logs that do not respond to 
jtregs attempts to stop them.




I wasn't suggesting to swallow the exception - rather to wrap it in an 
unchecked exception. The issue
is really only whether callers need to distinguish this situation from 
the various other errors and environmental

problems  that will all probably cause a test to be interrupted/stopped.

Michael

I've also received another comment offline about the method names. 
They should be more descriptive, and highlight the difference between 
these methods and regular delete. So maybe move to public 
deleteWithRetry?


-Chris.



Michael

On 07/11/13 10:05, Chris Hegarty wrote:

Virus checkers and, on more modern Windows OSes, Windows Experience
Service, and other external processes, have been causing trouble when
it comes to tests deleting files.

On a number of occasions in the past retry logic has been added to
tests that need to delete files. Also, the jtreg harness has similar
logic when cleaning up temporary files.

This has reared its head again with:
  8027902: TEST_BUG: java/net/URLClassLoader/closetest/CloseTest.java
and java/net/URLClassLoader/closetest/GetResourceAsStream.java failed
due to dir operation failed.
  8022213: Intermittent test failures in java/net/URLClassLoader

I'd like to add file utility functions to the common jdk/testlibrary,
starting with a "more reliable" delete. I've made minimal changes to
the URLClassloader tests to use this ( more could be done, but this is
just a start ).

http://cr.openjdk.java.net/~chegar/fileUtils/webrev/

The important part here for review is the "API" and impl in FileUtils.

Thanks,
-Chris.

Re: RFR: 8022213 Intermittent test failures in java/net/URLClassLoader (Add jdk/testlibrary/FileUtils.java)

[Michael McMahon]

Chris,

Would it be useful to add some instrumentation/logging (to System.err) 
if it's taking

more than one iteration to delete a file? We could end up with degraded
test performance if there is a general problem deleting files, and 
otherwise would have
no way of understanding what the problem is (eg. up to 7.5 seconds are 
allowed to delete the file)


Also, just curious what is the value in throwing InterruptedException 
out of the delete() method?

It seems onerous for calling code to have to catch it.

Michael

On 07/11/13 10:05, Chris Hegarty wrote:
Virus checkers and, on more modern Windows OSes, Windows Experience 
Service, and other external processes, have been causing trouble when 
it comes to tests deleting files.


On a number of occasions in the past retry logic has been added to 
tests that need to delete files. Also, the jtreg harness has similar 
logic when cleaning up temporary files.


This has reared its head again with:
  8027902: TEST_BUG: java/net/URLClassLoader/closetest/CloseTest.java 
and java/net/URLClassLoader/closetest/GetResourceAsStream.java failed 
due to dir operation failed.

  8022213: Intermittent test failures in java/net/URLClassLoader

I'd like to add file utility functions to the common jdk/testlibrary, 
starting with a "more reliable" delete. I've made minimal changes to 
the URLClassloader tests to use this ( more could be done, but this is 
just a start ).


http://cr.openjdk.java.net/~chegar/fileUtils/webrev/

The important part here for review is the "API" and impl in FileUtils.

Thanks,
-Chris.

Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE

[Michael McMahon]

Looks fine to me.

Michael

On 11/10/13 16:15, Brian Burkhalter wrote:

Thanks.

Any further comments from anyone else?

Brian

On Oct 10, 2013, at 9:04 PM, David Holmes wrote:


Ship it! :)

Thanks,
David

On 11/10/2013 5:24 AM, Brian Burkhalter wrote:

On Oct 10, 2013, at 11:21 AM, Brian Burkhalter wrote:


On Oct 10, 2013, at 11:05 AM, Brian Burkhalter wrote:


Nit: In the test there are a few places where you have t on a line by itself:

65t);

but it can go on the previous line and not exceed the length of other lines nearby. Also 
"+ should be " +

I don't see what you are referring to.

Oh, sorry, now I do. Will update.

I hope that as of now all nits have been picked.

http://cr.openjdk.java.net/~bpb/7179567/webrev.5/

Thanks,

Brian

Re: JDK 8 RFR 7179567: JCK8 tests: api/java_net/URLClassLoader/index.html#Ctor3 failed with NPE

[Michael McMahon]

On 08/10/13 12:08, Alan Bateman wrote:

On 04/10/2013 21:58, Brian Burkhalter wrote:

:
An updated webrev which I hope adequately addresses the expressed 
concerns may be found at:


http://cr.openjdk.java.net/~bpb/7179567.2/


This looks much better.

If I read the code correctly then the long standing behavior of 
URLClassLoader.getPermissions(null) was to throw a NPE, now it is 
changed to return the empty collection in order to match the super 
type (SecureClassLoader). I can't think of anything that would break 
so this is probably okay, just maybe a bit surprising.




I'm still not sure about that change. Its effect in sub-classes will be 
to continue on in the sub-class implementation
of getPermissions() with a null codesource parameter, where currently 
NPE would be thrown.


Michael