Re: RFR: 8245194: Unix domain socket channel implementation [v26]
> 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: - Alan's review patch from Oct 18 - Reverted changes to JFR unit tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/2d595423..d0ae1348 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=25 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=24-25 Stats: 764 lines in 21 files changed: 203 ins; 344 del; 217 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 [v27]
> 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 29 additional commits since the last revision: - Merge branch 'master' into unixdomainchannels - - Alan's review patch from Oct 18 - Reverted changes to JFR unit tests - - added NullTest and fix in SocketChannel.open - updated test bug ids - - use String.isEmpty where appropriate - fix white space error - - test updates from Daniel's review 14 Oct 2020 - src changes from Daniel's review. Tests will come later - Merge branch 'master' into unixdomainchannels - - reorganised the channel impls back into SocketChannelImpl and ServerSocketChannelImpl - removed the new Unix domain socket events and folded the behavior into the existing socket events - implemented other comments from Alan on Oct 11. - unixdomainchannels: updates from Chris's review 9 Oct 2020 - ... and 19 more: https://git.openjdk.java.net/jdk/compare/8d61d09d...eca95d86 - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/d0ae1348..eca95d86 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=26 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=25-26 Stats: 301287 lines in 667 files changed: 296118 ins; 3174 del; 1995 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: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer [v4]
> Hi, > > Could someone please review my doc-only fix for JDK-8253474: 'Javadoc clean > up in HttpsExchange, HttpsParameters, and > HttpsServer' ? > This fix is set of formatting changes intended to clean up the javadoc of the > following classes : > > `com.sun.net.httpserver.HttpsExchange` > `com.sun.net.httpserver.HttpsParameters` > `com.sun.net.httpserver.HttpsServer` > > This issue is a sub-task of > [JDK-8252822](https://bugs.openjdk.java.net/browse/JDK-8252822) > > Kind regards, > Patrick Patrick Concannon 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 remote-tracking branch 'origin/master' into JDK-8253474 - 8253474: Comment added to default constructor; fixed punctuation in create(InetSocketAddress, int) - Merge remote-tracking branch 'origin/master' into JDK-8253474 - 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer - Changes: - all: https://git.openjdk.java.net/jdk/pull/610/files - new: https://git.openjdk.java.net/jdk/pull/610/files/ef5b9fa7..e02bb1ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=610&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=610&range=02-03 Stats: 2169 lines in 170 files changed: 965 ins; 898 del; 306 mod Patch: https://git.openjdk.java.net/jdk/pull/610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/610/head:pull/610 PR: https://git.openjdk.java.net/jdk/pull/610
Re: RFR: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer [v4]
On Mon, 19 Oct 2020 15:33:25 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my doc-only fix for JDK-8253474: 'Javadoc clean >> up in HttpsExchange, HttpsParameters, and >> HttpsServer' ? >> This fix is set of formatting changes intended to clean up the javadoc of >> the following classes : >> >> `com.sun.net.httpserver.HttpsExchange` >> `com.sun.net.httpserver.HttpsParameters` >> `com.sun.net.httpserver.HttpsServer` >> >> This issue is a sub-task of >> [JDK-8252822](https://bugs.openjdk.java.net/browse/JDK-8252822) >> >> Kind regards, >> Patrick > > Patrick Concannon 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 remote-tracking branch 'origin/master' into JDK-8253474 > - 8253474: Comment added to default constructor; fixed punctuation in > create(InetSocketAddress, int) > - Merge remote-tracking branch 'origin/master' into JDK-8253474 > - 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and > HttpsServer Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/610
RFR: 8254967: com.sun.net.HttpsServer spins on TLS session close
This fixes a busy loop bug in the Http server which happens sometimes when an SSL connection is closed by the client. There is no regression test as it is not easy to reproduce and the only effect is that one executor thread gets tied up. - Commit messages: - 8254967: com.sun.net.HttpsServer spins on TLS session close Changes: https://git.openjdk.java.net/jdk/pull/742/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=742&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254967 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/742.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/742/head:pull/742 PR: https://git.openjdk.java.net/jdk/pull/742
Re: RFR: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer [v4]
On Mon, 19 Oct 2020 15:33:25 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my doc-only fix for JDK-8253474: 'Javadoc clean
>> up in HttpsExchange, HttpsParameters, and
>> HttpsServer' ?
>> This fix is set of formatting changes intended to clean up the javadoc of
>> the following classes :
>>
>> `com.sun.net.httpserver.HttpsExchange`
>> `com.sun.net.httpserver.HttpsParameters`
>> `com.sun.net.httpserver.HttpsServer`
>>
>> This issue is a sub-task of
>> [JDK-8252822](https://bugs.openjdk.java.net/browse/JDK-8252822)
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon 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 remote-tracking branch 'origin/master' into JDK-8253474
> - 8253474: Comment added to default constructor; fixed punctuation in
> create(InetSocketAddress, int)
> - Merge remote-tracking branch 'origin/master' into JDK-8253474
> - 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and
> HttpsServer
src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java
line 39:
> 37: * {@link HttpsConfigurator#configure(HttpsParameters)} for every
> incoming https
> 38: * connection,in order to determine the parameters to use.
> 39: *
looks like a space is missing between "connection,in"
src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java
line 33:
> 31: import javax.net.ssl.SSLParameters;
> 32: //END_TIGER_EXCLUDE
> 33:
These TIGER EXCLUDE comments could be removed at this stage.
-
PR: https://git.openjdk.java.net/jdk/pull/610
Re: RFR: 8254967: com.sun.net.HttpsServer spins on TLS session close
On Mon, 19 Oct 2020 15:50:32 GMT, Michael McMahon wrote:
> This fixes a busy loop bug in the Http server which happens sometimes when an
> SSL connection is closed by the client.
>
> There is no regression test as it is not easy to reproduce and the only
> effect is that one executor thread gets tied up.
Marked as reviewed by dfuchs (Reviewer).
src/jdk.httpserver/share/classes/sun/net/httpserver/SSLStreams.java line 445:
> 443: } finally {
> 444: handshaking.unlock();
> 445: }
Ok. The expectation is that doClosure() only involves sending a close
acknowedged and not receiving anything. This is
probably correct if doClosure() is called on reception of close_notify. In
which case the change looks reasonable. I
also see that a further expectation is that cycles of NEED_WRAP/NEED_UNWRAP
only happen during the handshake, which is
hopefully true.
-
PR: https://git.openjdk.java.net/jdk/pull/742
Integrated: 8254967: com.sun.net.HttpsServer spins on TLS session close
On Mon, 19 Oct 2020 15:50:32 GMT, Michael McMahon wrote: > This fixes a busy loop bug in the Http server which happens sometimes when an > SSL connection is closed by the client. > > There is no regression test as it is not easy to reproduce and the only > effect is that one executor thread gets tied up. This pull request has now been integrated. Changeset: 953e472d Author:Michael McMahon URL: https://git.openjdk.java.net/jdk/commit/953e472d Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod 8254967: com.sun.net.HttpsServer spins on TLS session close Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/742
Re: RFR: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer [v5]
> Hi, > > Could someone please review my doc-only fix for JDK-8253474: 'Javadoc clean > up in HttpsExchange, HttpsParameters, and > HttpsServer' ? > This fix is set of formatting changes intended to clean up the javadoc of the > following classes : > > `com.sun.net.httpserver.HttpsExchange` > `com.sun.net.httpserver.HttpsParameters` > `com.sun.net.httpserver.HttpsServer` > > This issue is a sub-task of > [JDK-8252822](https://bugs.openjdk.java.net/browse/JDK-8252822) > > Kind regards, > Patrick Patrick Concannon 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 five additional commits since the last revision: - Merge remote-tracking branch 'origin/master' into JDK-8253474 - Merge remote-tracking branch 'origin/master' into JDK-8253474 - 8253474: Comment added to default constructor; fixed punctuation in create(InetSocketAddress, int) - Merge remote-tracking branch 'origin/master' into JDK-8253474 - 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer - Changes: - all: https://git.openjdk.java.net/jdk/pull/610/files - new: https://git.openjdk.java.net/jdk/pull/610/files/e02bb1ae..94689aee Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=610&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=610&range=03-04 Stats: 1048 lines in 125 files changed: 698 ins; 99 del; 251 mod Patch: https://git.openjdk.java.net/jdk/pull/610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/610/head:pull/610 PR: https://git.openjdk.java.net/jdk/pull/610
Re: RFR: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer [v4]
On Mon, 19 Oct 2020 15:58:04 GMT, Michael McMahon wrote:
>> Patrick Concannon 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 remote-tracking branch 'origin/master' into JDK-8253474
>> - 8253474: Comment added to default constructor; fixed punctuation in
>> create(InetSocketAddress, int)
>> - Merge remote-tracking branch 'origin/master' into JDK-8253474
>> - 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and
>> HttpsServer
>
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java
> line 33:
>
>> 31: import javax.net.ssl.SSLParameters;
>> 32: //END_TIGER_EXCLUDE
>> 33:
>
> These TIGER EXCLUDE comments could be removed at this stage.
TIGER comments removed in commit
https://github.com/openjdk/jdk/pull/610/commits/45f451829c30339a10f03377a78b17b7291be82f
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java
> line 39:
>
>> 37: * {@link HttpsConfigurator#configure(HttpsParameters)} for every
>> incoming https
>> 38: * connection,in order to determine the parameters to use.
>> 39: *
>
> looks like a space is missing between "connection,in"
Typo fixed in
https://github.com/openjdk/jdk/pull/610/commits/45f451829c30339a10f03377a78b17b7291be82f
-
PR: https://git.openjdk.java.net/jdk/pull/610
Re: RFR: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer [v6]
> Hi, > > Could someone please review my doc-only fix for JDK-8253474: 'Javadoc clean > up in HttpsExchange, HttpsParameters, and > HttpsServer' ? > This fix is set of formatting changes intended to clean up the javadoc of the > following classes : > > `com.sun.net.httpserver.HttpsExchange` > `com.sun.net.httpserver.HttpsParameters` > `com.sun.net.httpserver.HttpsServer` > > This issue is a sub-task of > [JDK-8252822](https://bugs.openjdk.java.net/browse/JDK-8252822) > > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8253474: Fixed typo; removed TIGER macro/comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/610/files - new: https://git.openjdk.java.net/jdk/pull/610/files/94689aee..45f45182 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=610&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=610&range=04-05 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/610/head:pull/610 PR: https://git.openjdk.java.net/jdk/pull/610
Re: RFR: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer [v6]
On Mon, 19 Oct 2020 17:36:25 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my doc-only fix for JDK-8253474: 'Javadoc clean >> up in HttpsExchange, HttpsParameters, and >> HttpsServer' ? >> This fix is set of formatting changes intended to clean up the javadoc of >> the following classes : >> >> `com.sun.net.httpserver.HttpsExchange` >> `com.sun.net.httpserver.HttpsParameters` >> `com.sun.net.httpserver.HttpsServer` >> >> This issue is a sub-task of >> [JDK-8252822](https://bugs.openjdk.java.net/browse/JDK-8252822) >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8253474: Fixed typo; removed TIGER macro/comments Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/610
Re: RFR: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer [v6]
On Mon, 19 Oct 2020 17:36:25 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my doc-only fix for JDK-8253474: 'Javadoc clean >> up in HttpsExchange, HttpsParameters, and >> HttpsServer' ? >> This fix is set of formatting changes intended to clean up the javadoc of >> the following classes : >> >> `com.sun.net.httpserver.HttpsExchange` >> `com.sun.net.httpserver.HttpsParameters` >> `com.sun.net.httpserver.HttpsServer` >> >> This issue is a sub-task of >> [JDK-8252822](https://bugs.openjdk.java.net/browse/JDK-8252822) >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8253474: Fixed typo; removed TIGER macro/comments Marked as reviewed by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/610
Re: RFR: 8245194: Unix domain socket channel implementation [v28]
> 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: final feedback from Alan - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/eca95d86..36623a45 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=27 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=26-27 Stats: 191 lines in 9 files changed: 30 ins; 100 del; 61 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
