Re: RFR[8251496]: ‘Fix doclint warnings in jdk.net.httpserver’
On 08/09/2020 21:02, Roger Riggs wrote: And will need to be updated as a new PR under git. PR: https://git.openjdk.java.net/jdk/pull/81 best regards, -- daniel
Re: RFR: 8251496: Fix doclint warnings in jdk.net.httpserver [v2]
> Hi, > > Could someone please review my doc-only fix for JDK-8251496 - ‘Fix doclint > warnings in jdk.net.httpserver’ ? > > This fix addresses the warnings generated by `javadoc -Xdoclint` due to > missing/incomplete API documentation for > several classes within `jdk.net.httpserver`. > CSR: https://bugs.openjdk.java.net/browse/JDK-8252585 > > 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 two additional commits since the last revision: - Merge branch 'master' into JDK-8251496 - 8251496: Fix doclint warnings in jdk.net.httpserver - Changes: - all: https://git.openjdk.java.net/jdk/pull/81/files - new: https://git.openjdk.java.net/jdk/pull/81/files/8faf6076..90e7ec0c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=81&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=81&range=00-01 Stats: 304 lines in 41 files changed: 88 ins; 126 del; 90 mod Patch: https://git.openjdk.java.net/jdk/pull/81.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/81/head:pull/81 PR: https://git.openjdk.java.net/jdk/pull/81
Re: RFR: 8251496: Fix doclint warnings in jdk.net.httpserver [v3]
> Hi, > > Could someone please review my doc-only fix for JDK-8251496 - ‘Fix doclint > warnings in jdk.net.httpserver’ ? > > This fix addresses the warnings generated by `javadoc -Xdoclint` due to > missing/incomplete API documentation for > several classes within `jdk.net.httpserver`. > Kind regards, > Patrick Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8251496: reworded abstract cstr comments; removed unnecessary punctuation - Changes: - all: https://git.openjdk.java.net/jdk/pull/81/files - new: https://git.openjdk.java.net/jdk/pull/81/files/90e7ec0c..660a549c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=81&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=81&range=01-02 Stats: 29 lines in 9 files changed: 1 ins; 0 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/81.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/81/head:pull/81 PR: https://git.openjdk.java.net/jdk/pull/81
Re: RFR: 8251496: Fix doclint warnings in jdk.net.httpserver [v3]
On Tue, 8 Sep 2020 16:34:12 GMT, Daniel Fuchs wrote:
>> Patrick Concannon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8251496: reworded abstract cstr comments; removed unnecessary punctuation
>
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/Authenticator.java
> line 93:
>
>> 91:
>> 92: /**
>> 93: * Creates a {@code Success} instance with given Principal.
>
> For consistency with line 103 below maybe Principal should be quoted `{@code
> Principal}` here and at line 95 too
Changed to use {@code Principal}.
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpExchange.java
> line 69:
>
>> 67:
>> 68: /**
>> 69: * Creates a HttpExchange.
>
> I'd suggest to keep consistent... You have been using `* Constructor for
> subclasses to call` at other places. Maybe you
> should use that sentence here too.
Replaced wording -- now using `* Constructor for subclasses to call`.
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpContext.java line
> 45:
>
>> 43:
>> 44: /**
>> 45: * Creates a HttpContext.
>
> I'd suggest to keep consistent... You have been using * Constructor for
> subclasses to call at other places. Maybe you
> should use that sentence here too.
Replaced wording as suggested.
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/Filter.java line 45:
>
>> 43:
>> 44: /**
>> 45: * Creates a Filter.
>
> I'd suggest to keep consistent... You have been using * Constructor for
> subclasses to call at other places. Maybe you
> should use that sentence here too.
Replaced wording as suggested.
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java
> line 30:
>
>> 28: import java.net.InetSocketAddress;
>> 29:
>> 30: //BEGIN_TIGER_EXCLUDE
>
> I don't have the context here - but if we're removing this import then I
> guess we should be removing the `// BEGIN` and
> `// END` comments too?
Thanks for spotting this. IntelliJ re-arranged the imports automatically. I
reverted it back to its original state
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java
> line 61:
>
>> 59:
>> 60: /**
>> 61: * Creates a HttpsParameters.
>
> Same comment about consistency.
Replaced wording as suggested.
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java
> line 68:
>
>> 66: * Returns the HttpsConfigurator for this HttpsParameters.
>> 67: *
>> 68: * @returns HttpsConfigurator for this instance of HttpsParameters.
>
> Probably would be worth saying if the result can be `null`. I guess it can.
> However I realize that this may be a more
> general issue with the whole API, so maybe what you have is OK for now, and
> we should leave further tightening of the
> spec to a followup.
I have opened another issue for this --
https://bugs.openjdk.java.net/browse/JDK-8252822 -- and will address them there
-
PR: https://git.openjdk.java.net/jdk/pull/81
Re: RFR: 8251496: Fix doclint warnings in jdk.net.httpserver [v3]
On Wed, 9 Sep 2020 12:16:44 GMT, Patrick Concannon wrote: >> src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpsParameters.java >> line 30: >> >>> 28: import java.net.InetSocketAddress; >>> 29: >>> 30: //BEGIN_TIGER_EXCLUDE >> >> I don't have the context here - but if we're removing this import then I >> guess we should be removing the `// BEGIN` and >> `// END` comments too? > > Thanks for spotting this. IntelliJ re-arranged the imports automatically. I > reverted it back to its original state I suspect the commented lines are obsolete TIGER was a long time ago. That looks like some markup for a tool to use a non-ssl version as a demo. @AlanBateman Alan might know. - PR: https://git.openjdk.java.net/jdk/pull/81
Re: RFR: 8251496: Fix doclint warnings in jdk.net.httpserver [v3]
On Wed, 9 Sep 2020 12:14:28 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my doc-only fix for JDK-8251496 - ‘Fix doclint >> warnings in jdk.net.httpserver’ ? >> >> This fix addresses the warnings generated by `javadoc -Xdoclint` due to >> missing/incomplete API documentation for >> several classes within `jdk.net.httpserver`. >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8251496: reworded abstract cstr comments; removed unnecessary punctuation The @param and @return lines that start with a capital letter seem out of place. The contents of most @tags are usually phrases without capitalization or period. src/jdk.httpserver/share/classes/com/sun/net/httpserver/Authenticator.java line 75: > 73: > 74: /** > 75: * returns the response code to send to the client Initial Capital and add a period please. src/jdk.httpserver/share/classes/com/sun/net/httpserver/Authenticator.java line 101: > 99: } > 100: /** > 101: * returns the authenticated user Principal Capitalize and add a period. Check all the method for the first line comments. src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpPrincipal.java line 74: > 72: > 73: /** > 74: * returns the username this object was created with. Capitalize and add a period. Please check all methods. - Changes requested by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/81
Re: RFR: 8251496: Fix doclint warnings in jdk.net.httpserver [v3]
On Wed, 9 Sep 2020 13:29:05 GMT, Roger Riggs wrote: >> Patrick Concannon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8251496: reworded abstract cstr comments; removed unnecessary punctuation > > The @param and @return lines that start with a capital letter seem out of > place. > The contents of most @tags are usually phrases without capitalization or > period. @RogerRiggs There are many many formatting and cleanup tasks that could be done to this particular code. My advise to Patrick was (and continues to be) to just focus on the particular issue at hand - clean up the doclint warnings. Other annoyances can be fixed up later during another pass. This issue is already overloaded with cleanup and semantic spec changes. It will just increase the cognitive complexity to continue to fix (arguably) unrelated annoyances. - PR: https://git.openjdk.java.net/jdk/pull/81
Re: RFR: 8251496: Fix doclint warnings in jdk.net.httpserver [v3]
On Wed, 9 Sep 2020 12:14:28 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my doc-only fix for JDK-8251496 - ‘Fix doclint >> warnings in jdk.net.httpserver’ ? >> >> This fix addresses the warnings generated by `javadoc -Xdoclint` due to >> missing/incomplete API documentation for >> several classes within `jdk.net.httpserver`. >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8251496: reworded abstract cstr comments; removed unnecessary punctuation Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/81
Re: RFR: 8251496: Fix doclint warnings in jdk.net.httpserver [v2]
On Wed, 9 Sep 2020 08:50:38 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my doc-only fix for JDK-8251496 - ‘Fix doclint >> warnings in jdk.net.httpserver’ ? >> >> This fix addresses the warnings generated by `javadoc -Xdoclint` due to >> missing/incomplete API documentation for >> several classes within `jdk.net.httpserver`. >> 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 two additional commits > since the last revision: > - Merge branch 'master' into JDK-8251496 > - 8251496: Fix doclint warnings in jdk.net.httpserver src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpExchange.java line 192: > 190: *no response body may be written. > 191: * @throws IOException An IOException will be thrown if an error > occurs during > 192: * the sending of response headers, or if headers have > already been sent. > "or if headers have already been sent." Maybe I missed it, but is there a test to assert this? It is not obvious to me why this is being added now. - PR: https://git.openjdk.java.net/jdk/pull/81
Re: Thread safety problem in java.net.ProxySelector
Seems like a bug. Can you please file an issue for it.
Thanks,
-Chris.
> On 2 Sep 2020, at 14:16, David Lloyd wrote:
>
> The default proxy selector field in java.net.ProxySelector is
> essentially a global variable with no thread safety measures taken to
> ensure that accesses are valid. Anecdotally, a symptom of this bug
> *may* be that the field appears `null` after assignment (based on an
> IRC discussion with a confused user).
>
> Here's the trivial fix:
>
> diff --git a/src/java.base/share/classes/java/net/ProxySelector.java
> b/src/java.base/share/classes/java/net/ProxySelector.java
> index c1e97ecc981..e52c888f755 100644
> --- a/src/java.base/share/classes/java/net/ProxySelector.java
> +++ b/src/java.base/share/classes/java/net/ProxySelector.java
> @@ -65,7 +65,7 @@ public abstract class ProxySelector {
> *
> * @see #setDefault(ProxySelector)
> */
> -private static ProxySelector theProxySelector;
> +private static volatile ProxySelector theProxySelector;
>
> static {
> try {
>
>
> --
> - DML • he/him
>
