Re: RFR: JDK-8061729 : Update java/net tests to eliminate dependency on sun.net.www.MessageHeader and some other internal APIs [v5]
On Fri, 11 Feb 2022 11:15:56 GMT, Mahendra Chhipa wrote:
>> There are some regression tests depending on sun.net.www.MessageHeader, the
>> internal API dependency should be removed. Some of other internal API
>> dependancies are removed in following issues :
>> JDK-8273142
>> JDK-8268464
>> JDK-8268133
>
> Mahendra Chhipa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 1. Handled continuation line
> 2. handled \r, \n, or \r\n
> 3. Added the test for HttpHeaderParser class
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 100:
> 98: public List getHeaderValue(String key) {
> 99: if(headerMap.containsKey(key.toLowerCase())) {
> 100: return headerMap.get(key.toLowerCase());
I'd suggest to use `key.toLowerCase(Locale.ROOT)` - even though it shouldn't
really matter if it's guaranteed that keys have been syntactically checked for
illegal characters.
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 120:
> 118: * ( when true is returned ), which may not be fully consumed.
> 119: *
> 120: * @param input the ( partial ) header data
It doesn't look like this is a accurate. There's no ByteBuffers anywhere...
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 148:
> 146: case FINISHED -> false;
> 147: case STATUS_LINE_FOUND_LF, STATUS_LINE_END_LF,
> HEADER_FOUND_LF -> true;
> 148: default -> buffer.available() >= 0;
It's a bit chancy to use InputStream.available() - it would be better to have
some `eof` variable that's set to true when EOF is reached, and return `!eof`
here
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 162:
> 160: * value from [0, 255] representing the character code.
> 161: *
> 162: * @param input a {@code ByteBuffer} containing a partial input
Input is not a ByteBuffer and it doesn't contain a partial input.
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 171:
> 169: private char get(InputStream input) throws IOException {
> 170: return (char)(input.read() & 0xFF);
> 171: }
That doesn't look right as it will prevent you to read EOF. Maybe it should be:
boolean eof;
...
private int get(InputStream input) throws IOException {
int c = input.read();
if (c < 0) eof = true;
return c;
}
And then you would have to check for EOF everywhere where `get(input)` is
called too.
test/lib/jdk/test/lib/net/HttpHeaderParser.java line 175:
> 173: private void readResumeStatusLine(InputStream input) throws
> IOException {
> 174: char c = 0;
> 175: while (input.available() > 0 && (c = get(input)) != CR) {
It's not a too good idea to depend on input.available() - the spec leaves too
much room for odd behaviors.
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/InputStream.html#available()
An implementation may decide to return 0 always - as it is the amount of bytes
that is guaranteed to be readable without blocking (and an implementation may
not know how many bytes can be read without blocking).
-
PR: https://git.openjdk.java.net/jdk/pull/5937
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v3]
On Mon, 14 Feb 2022 13:38:06 GMT, Daniel Fuchs 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 12 additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into keepalive
>> - update after Daniel's first review
>> - Merge branch 'master' into keepalive
>> - added docs
>> - reverted change to LIFETIME constant. Were not necessary
>> - updates
>> - Merge branch 'master' into keepalive
>> - Merge branch 'master' into keepalive
>> - Merge branch 'master' into keepalive
>> - Merge branch 'master' into keepalive
>> - ... and 2 more:
>> https://git.openjdk.java.net/jdk/compare/2fc8366a...b0b7673c
>
> test/jdk/sun/net/www/http/KeepAliveCache/KeepAliveProperty.java line 111:
>
>> 109: out.print(BODY);
>> 110: out.flush();
>> 111: pass = true;
>
> should that be: pass = ! expectClose?
Good catch. That was masking another problem in the test where it wasn't
detecting the socket close properly.
> test/jdk/sun/net/www/http/KeepAliveCache/KeepAliveProperty.java line 125:
>
>> 123:
>> 124: static String fetch(URL url) throws Exception {
>> 125: InputStream in = url.openConnection(NO_PROXY).getInputStream();
>
> could use try-with-resource here
Ok
-
PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v4]
> Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: Fixed test case problem and update from Daniel's review - Changes: - all: https://git.openjdk.java.net/jdk/pull/7349/files - new: https://git.openjdk.java.net/jdk/pull/7349/files/b0b7673c..79f64a76 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7349&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7349&range=02-03 Stats: 22 lines in 1 file changed: 5 ins; 6 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v4]
On Tue, 15 Feb 2022 13:30:35 GMT, Michael McMahon wrote: >> Hi, >> >> Could I get the following patch reviewed please? (A CSR is also required >> which I will submit when the docs are agreed) >> >> It adds a pair of new system properties to make the keep alive timer in >> java.net.HttpURLConnection configurable. >> The proposed property names are: >> >> "http.keepAlive.time.server" and "http.keepAlive.time.proxy" >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > Fixed test case problem and update from Daniel's review Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7349
RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects.
As described in the title, this is a simple change to the `HttpRequest.Builder::build` method to highlight that an immutable and reusable instance of an `HttpRequest` is created when this method is invoked. This is done by adding an `@implSpec` Javadoc Tag (details on `@implSpec` given [here](https://openjdk.java.net/jeps/8068562)). The detail added under the `@implSpec` Tag is similar to that provided at the interface-level Javadoc for Builder. Please also review the CSR for this change: https://bugs.openjdk.java.net/browse/JDK-8281833 - Commit messages: - 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects. Changes: https://git.openjdk.java.net/jdk/pull/7479/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7479&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281223 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7479/head:pull/7479 PR: https://git.openjdk.java.net/jdk/pull/7479
Re: RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects.
On Tue, 15 Feb 2022 14:05:16 GMT, Conor Cleary wrote:
> As described in the title, this is a simple change to the
> `HttpRequest.Builder::build` method to highlight that an immutable and
> reusable instance of an `HttpRequest` is created when this method is invoked.
> This is done by adding an `@implSpec` Javadoc Tag (details on `@implSpec`
> given [here](https://openjdk.java.net/jeps/8068562)).
>
> The detail added under the `@implSpec` Tag is similar to that provided at the
> interface-level Javadoc for Builder.
>
> Please also review the CSR for this change:
> https://bugs.openjdk.java.net/browse/JDK-8281833
src/java.net.http/share/classes/java/net/http/HttpRequest.java line 297:
> 295: * @implSpec Returns a new {@code HttpRequest} each time it is
> 296: * invoked. Once built, the HttpRequest is immutable and can be
> 297: * sent multiple times.
Looks fine to me. Maybe "This method returns ..." would more appropriate for an
`@implSpec`. Also the second mention of `HttpRequest` should also have `{@code
}` around it.
-
PR: https://git.openjdk.java.net/jdk/pull/7479
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v5]
> Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: tighten up spec wording and move props beside existing keep alive ones - Changes: - all: https://git.openjdk.java.net/jdk/pull/7349/files - new: https://git.openjdk.java.net/jdk/pull/7349/files/79f64a76..9a7a4ced Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7349&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7349&range=03-04 Stats: 18 lines in 2 files changed: 10 ins; 8 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v5]
On Tue, 15 Feb 2022 15:18:46 GMT, Michael McMahon wrote: >> Hi, >> >> Could I get the following patch reviewed please? (A CSR is also required >> which I will submit when the docs are agreed) >> >> It adds a pair of new system properties to make the keep alive timer in >> java.net.HttpURLConnection configurable. >> The proposed property names are: >> >> "http.keepAlive.time.server" and "http.keepAlive.time.proxy" >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > tighten up spec wording and move props beside existing keep alive ones Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects.
On Tue, 15 Feb 2022 15:04:12 GMT, Daniel Fuchs wrote:
>> As described in the title, this is a simple change to the
>> `HttpRequest.Builder::build` method to highlight that an immutable and
>> reusable instance of an `HttpRequest` is created when this method is
>> invoked. This is done by adding an `@implSpec` Javadoc Tag (details on
>> `@implSpec` given [here](https://openjdk.java.net/jeps/8068562)).
>>
>> The detail added under the `@implSpec` Tag is similar to that provided at
>> the interface-level Javadoc for Builder.
>>
>> Please also review the CSR for this change:
>> https://bugs.openjdk.java.net/browse/JDK-8281833
>
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 297:
>
>> 295: * @implSpec Returns a new {@code HttpRequest} each time it is
>> 296: * invoked. Once built, the HttpRequest is immutable and can be
>> 297: * sent multiple times.
>
> Looks fine to me. Maybe "This method returns ..." would more appropriate for
> an `@implSpec`. Also the second mention of `HttpRequest` should also have
> `{@code }` around it.
Agree with both statements. "This method returns..." adds additional clarity
and the additional {@code } was left out in error. Will correct now.
-
PR: https://git.openjdk.java.net/jdk/pull/7479
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v6]
> Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: remove extraneous .swp file - Changes: - all: https://git.openjdk.java.net/jdk/pull/7349/files - new: https://git.openjdk.java.net/jdk/pull/7349/files/9a7a4ced..d047808e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7349&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7349&range=04-05 Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects. [v2]
> As described in the title, this is a simple change to the > `HttpRequest.Builder::build` method to highlight that an immutable and > reusable instance of an `HttpRequest` is created when this method is invoked. > This is done by adding an `@implSpec` Javadoc Tag (details on `@implSpec` > given [here](https://openjdk.java.net/jeps/8068562)). > > The detail added under the `@implSpec` Tag is similar to that provided at the > interface-level Javadoc for Builder. > > Please also review the CSR for this change: > https://bugs.openjdk.java.net/browse/JDK-8281833 Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8281223: Updated description, added missing code tag - Changes: - all: https://git.openjdk.java.net/jdk/pull/7479/files - new: https://git.openjdk.java.net/jdk/pull/7479/files/1bead0bd..bfc1de70 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7479&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7479&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7479.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7479/head:pull/7479 PR: https://git.openjdk.java.net/jdk/pull/7479
Re: RFR: 8281223: Improve the API documentation of HttpRequest.Builder::build to state that the default implementation provided by the JDK returns immutable objects. [v2]
On Tue, 15 Feb 2022 15:33:01 GMT, Conor Cleary wrote: >> As described in the title, this is a simple change to the >> `HttpRequest.Builder::build` method to highlight that an immutable and >> reusable instance of an `HttpRequest` is created when this method is >> invoked. This is done by adding an `@implSpec` Javadoc Tag (details on >> `@implSpec` given [here](https://openjdk.java.net/jeps/8068562)). >> >> The detail added under the `@implSpec` Tag is similar to that provided at >> the interface-level Javadoc for Builder. >> >> Please also review the CSR for this change: >> https://bugs.openjdk.java.net/browse/JDK-8281833 > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8281223: Updated description, added missing code tag Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7479
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v6]
On Tue, 15 Feb 2022 15:30:07 GMT, Michael McMahon wrote: >> Hi, >> >> Could I get the following patch reviewed please? (A CSR is also required >> which I will submit when the docs are agreed) >> >> It adds a pair of new system properties to make the keep alive timer in >> java.net.HttpURLConnection configurable. >> The proposed property names are: >> >> "http.keepAlive.time.server" and "http.keepAlive.time.proxy" >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > remove extraneous .swp file Good catch! I'd missed that. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7349
RFR: 8281305: Test com/sun/net/httpserver/simpleserver/MapToPathTest.java fails on Windows 11
This change adds a check for URI path segments that look like a root drive on Windows, e.g. "C:". Such path segments are not supported because a request path should really be relative to the working directory. The check is implemented with a platform-specific helper class, some additional test cases are included. Testing: tier 1-3, common Windows versions. - Commit messages: - initial commit Changes: https://git.openjdk.java.net/jdk/pull/7478/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7478&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281305 Stats: 138 lines in 4 files changed: 131 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7478.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7478/head:pull/7478 PR: https://git.openjdk.java.net/jdk/pull/7478
