RFR 8133686: REOPEN JDK-8080659 : HttpURLConnection?s getHeaderFields method returns field values in reverse order
Hi all, I'm Evan and I've just recently joined the Java Platform Group here in Oracle so this is my first time getting to interact with the OpenJDK Community. This is a request to review my fix for https://bugs.openjdk.java.net/browse/JDK-8133686 The webrev for this fix can be found at: http://cr.openjdk.java.net/~kravikumar/8133686/webrev.00/webrev/ The fix for this bug is straightforward and I don't believe that this change needs a CSR, as this change makes the source code conform to the RFC2616 spec, and does not introduce any behavioural changes. That being said, I welcome any differing thoughts! I look forward to your responses. Thanks, Evan
RFR 8250748: Doc of URL(String, String, int, String, URLStreamHandler) does not use link
Hi all,
I have another small fix for net-dev reviewers.
This is a small fix to the java/net/URL.java documentation to add a link to one
of the constructors.
--- old/open/src/java.base/share/classes/java/net/URL.java 2020-08-18
14:53:33.886033182 +
+++ new/open/src/java.base/share/classes/java/net/URL.java 2020-08-18
14:53:33.770030806 +
@@ -397,8 +397,8 @@
* a {@code handler} of {@code null} indicates that the URL
* should use a default stream handler for the protocol, as outlined
* for:
- * java.net.URL#URL(java.lang.String, java.lang.String, int,
- * java.lang.String)
+ * {@link java.net.URL#URL(java.lang.String, java.lang.String, int,
+ * java.lang.String)}
*
* If the handler is not null and there is a security manager,
* the security manager's {@code checkPermission}
JBS Link: https://bugs.openjdk.java.net/browse/JDK-8250748
Thank you in advance everyone.
Evan
RE: RFR 8133686: REOPEN JDK-8080659 : HttpURLConnection?s getHeaderFields method returns field values in reverse order
Hi all, A revised webrev for this change can be found at http://cr.openjdk.java.net/~coffeys/evwhelan/8133686/webrev/ This revision fixes a test that failed as a result of the source change. If anyone has any further thoughts on whether this change needs a CSR, I would love to hear them. Thanks in advance, Evan From: Evan Whelan Sent: Friday 14 August 2020 13:10 To: net-dev@openjdk.java.net Subject: RFR 8133686: REOPEN JDK-8080659 : HttpURLConnection?s getHeaderFields method returns field values in reverse order Hi all, I'm Evan and I've just recently joined the Java Platform Group here in Oracle so this is my first time getting to interact with the OpenJDK Community. This is a request to review my fix for https://bugs.openjdk.java.net/browse/JDK-8133686 The webrev for this fix can be found at: http://cr.openjdk.java.net/~kravikumar/8133686/webrev.00/webrev/ The fix for this bug is straightforward and I don't believe that this change needs a CSR, as this change makes the source code conform to the RFC2616 spec, and does not introduce any behavioural changes. That being said, I welcome any differing thoughts! I look forward to your responses. Thanks, Evan
RE: RFR 8133686: REOPEN JDK-8080659 : HttpURLConnection?s getHeaderFields method returns field values in reverse order
Hi,
Thanks for the feedback Daniel, I've added the test verifying the issue.
The test checks both getHeaderFields and getRequestProperties.
Please find another revision of this fix at:
http://cr.openjdk.java.net/~kravikumar/8133686/webrev.03/
A CSR for this issue has also been written and is found at:
https://bugs.openjdk.java.net/browse/JDK-8252456
Please let me know if you have any more suggestions or feedback.
Kind regards,
Evan
-Original Message-
From: Daniel Fuchs
Sent: Thursday 27 August 2020 20:01
To: Evan Whelan ; net-dev@openjdk.java.net
Subject: Re: RFR 8133686: REOPEN JDK-8080659 : HttpURLConnection?s
getHeaderFields method returns field values in reverse order
Hi Evan,
Actually - I believe you are still missing a test that verifies the actual
issue - that is:
If a multi valued field is sent over the wire as a repeated list of headers -
that is - if it is transmitted as:
foo: foo1
foo: foo2
foo: foo3
then you need to verify that HttpURLConnection.getHeaderFields().get("foo")
will now return [foo1, foo2, foo3] and not [foo3, foo2, foo1].
Also I notice that your change also fixes URLConnection::getRequestProperties(),
directly implemented in URLConnection, which is both good and bad.
Good because the previous behavior was IMO a bug. Bad because it slightly
increases the chances of regression (from minimal to low).
So I now believe that writing a CSR is indeed required, to raise the awareness
on this behavior change.
best regards,
-- daniel
On 20/08/2020 17:06, Evan Whelan wrote:
> Hi all,
>
> A revised webrev for this change can be found at
> http://cr.openjdk.java.net/~coffeys/evwhelan/8133686/webrev/
>
> This revision fixes a test that failed as a result of the source change.
>
> If anyone has any further thoughts on whether this change needs a CSR,
> I would love to hear them.
>
> Thanks in advance,
>
> Evan
RFR: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order
Hi all, Please review this fix for which corrects the order in which field values are returned from the `HttpURLConnection.getHeaderFields` and `URLConnection.getRequestProperties` methods. Currently, the implementation of these methods returns the values in reverse. This does not conform with the RFC2616 spec which outlines that the order of these field values should not be changed. Thanks, Evan - Commit messages: - 8133686: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order v2 - 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order Changes: https://git.openjdk.java.net/jdk/pull/2294/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2294&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8133686 Stats: 418 lines in 7 files changed: 371 ins; 31 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/2294.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2294/head:pull/2294 PR: https://git.openjdk.java.net/jdk/pull/2294
Re: RFR: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order [v2]
On Thu, 28 Jan 2021 17:02:31 GMT, Daniel Fuchs wrote: >> Evan Whelan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> URLConnection doc fixes > > src/java.base/share/classes/java/net/URLConnection.java line 596: > >> 594: * corresponding values, they must be returned in the order they >> were added, >> 595: * preserving the insertion-order. >> 596: * > > Here the `@implSpec` is right because it describes how subclasses should > implement the method. > Maybe there should be an additional `@implNote` as well to describe what the > default implementation does. > > * @implNote The default implementation of this method returns an empty > map always. > > The `@implNote` is useful for subclasses to decide whether they should > override the default implementation. Added, thanks Daniel > src/java.base/share/classes/java/net/URLConnection.java line 1201: > >> 1199: * @implSpec The default implementation of this method should >> preserve insertion order when >> 1200: * multiple values are added for a given key. They must be >> 1201: * returned in the order they were added. > > If we are speaking about what the default implementation actually does, then > the conditional tense should be removed. And this should probably be an > @implNote. > > * @implNote The default implementation of this method preserves the > insertion order when > * multiple values are added for a given key. The values are returned in > the order they > * were added. This has been changed, thanks - PR: https://git.openjdk.java.net/jdk/pull/2294
Re: RFR: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order [v2]
> Hi all, > > Please review this fix for which corrects the order in which field values are > returned from the `HttpURLConnection.getHeaderFields` and > `URLConnection.getRequestProperties` methods. > > Currently, the implementation of these methods returns the values in reverse. > This does not conform with the RFC2616 spec which outlines that the order of > these field values should not be changed. > > Thanks, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: URLConnection doc fixes - Changes: - all: https://git.openjdk.java.net/jdk/pull/2294/files - new: https://git.openjdk.java.net/jdk/pull/2294/files/e332fff5..1a74ae6e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2294&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2294&range=00-01 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2294.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2294/head:pull/2294 PR: https://git.openjdk.java.net/jdk/pull/2294
Re: RFR: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order [v3]
> Hi all, > > Please review this fix for which corrects the order in which field values are > returned from the `HttpURLConnection.getHeaderFields` and > `URLConnection.getRequestProperties` methods. > > Currently, the implementation of these methods returns the values in reverse. > This does not conform with the RFC2616 spec which outlines that the order of > these field values should not be changed. > > Thanks, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: MessageHeaderTest add comma to copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/2294/files - new: https://git.openjdk.java.net/jdk/pull/2294/files/1a74ae6e..4e0c96cb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2294&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2294&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2294.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2294/head:pull/2294 PR: https://git.openjdk.java.net/jdk/pull/2294
Re: RFR: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order [v4]
> Hi all, > > Please review this fix for which corrects the order in which field values are > returned from the `HttpURLConnection.getHeaderFields` and > `URLConnection.getRequestProperties` methods. > > Currently, the implementation of these methods returns the values in reverse. > This does not conform with the RFC2616 spec which outlines that the order of > these field values should not be changed. > > Thanks, > Evan Evan Whelan 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 six additional commits since the last revision: - Changed spec text to make correct use of tags - Merge branch 'master' into JDK-8133686_MessageHeader - MessageHeaderTest add comma to copyright year - URLConnection doc fixes - 8133686: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order v2 - 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order - Changes: - all: https://git.openjdk.java.net/jdk/pull/2294/files - new: https://git.openjdk.java.net/jdk/pull/2294/files/4e0c96cb..160d9e33 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2294&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2294&range=02-03 Stats: 1550176 lines in 16228 files changed: 796447 ins; 678052 del; 75677 mod Patch: https://git.openjdk.java.net/jdk/pull/2294.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2294/head:pull/2294 PR: https://git.openjdk.java.net/jdk/pull/2294
Integrated: 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order
On Thu, 28 Jan 2021 16:42:02 GMT, Evan Whelan wrote: > Hi all, > > Please review this fix for which corrects the order in which field values are > returned from the `HttpURLConnection.getHeaderFields` and > `URLConnection.getRequestProperties` methods. > > Currently, the implementation of these methods returns the values in reverse. > This does not conform with the RFC2616 spec which outlines that the order of > these field values should not be changed. > > Thanks, > Evan This pull request has now been integrated. Changeset: 00e059dd Author:Evan Whelan Committer: Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/00e059ddb34b5f2d6ba1ea0f38308c5b382a8e4d Stats: 419 lines in 7 files changed: 373 ins; 31 del; 15 mod 8133686: HttpURLConnection.getHeaderFields and URLConnection.getRequestProperties methods return field values in reverse order Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/2294
RFR: 8274779: HttpClient and HttpsClient incorrectly check request method when set to POST
Hi,
Please review my fix for JDK-8274779 which changes how HttpClient and
HttpsClient checks for equality when comparing request methods.
When `HttpURLConnection.setRequestMethod` is passed `new String("POST")` rather
than the "POST" String literal, the old behaviour resulted in broken
HttpClients being reused from the `KeepAliveCache`.
This is because a call to `HttpClient.available()` was never reachable due to
identity equality being used instead of logical equality.
The test case uses an injected KeepAliveCache, to which we put a HttpClient
that is unavailable. By comparing the initial HttpClient's `connectTimeout`
value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we
can assert that these values should never be equal as a new HttpClient should
be created in cases where we can no longer use the cached one.
All CI testing is green for this fix.
Kind regards,
Evan
-
Commit messages:
- 8274779: HttpClient and HttpsClient incorrectly check request method when
set to POST
Changes: https://git.openjdk.java.net/jdk/pull/5964/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8274779
Stats: 143 lines in 3 files changed: 141 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/5964.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5964/head:pull/5964
PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v2]
> Hi,
>
> Please review my fix for JDK-8274779 which changes how HttpClient and
> HttpsClient checks for equality when comparing request methods.
>
> When `HttpURLConnection.setRequestMethod` is passed `new String("POST")`
> rather than the "POST" String literal, the old behaviour resulted in broken
> HttpClients being reused from the `KeepAliveCache`.
>
> This is because a call to `HttpClient.available()` was never reachable due to
> identity equality being used instead of logical equality.
>
> The test case uses an injected KeepAliveCache, to which we put a HttpClient
> that is unavailable. By comparing the initial HttpClient's `connectTimeout`
> value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we
> can assert that these values should never be equal as a new HttpClient should
> be created in cases where we can no longer use the cached one.
>
> All CI testing is green for this fix.
>
> Kind regards,
> Evan
Evan Whelan has updated the pull request incrementally with one additional
commit since the last revision:
Test changes
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5964/files
- new: https://git.openjdk.java.net/jdk/pull/5964/files/04a97fe2..a65e493d
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=00-01
Stats: 16 lines in 2 files changed: 10 ins; 4 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/5964.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5964/head:pull/5964
PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v3]
On Fri, 15 Oct 2021 09:36:06 GMT, Daniel Fuchs wrote: >> Evan Whelan has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Extracted both connectTimeouts to variables and added copyright header to >> accessor >> - Removed reflection in favour of injected accessor for KeepAliveCache > > test/jdk/sun/net/www/http/KeepAliveCache/RequestMethodEquality.java line 32: > >> 30: * @modules java.base/sun.net.www.http:+open >> 31: * java.base/sun.net.www.protocol.http >> 32: * @run testng RequestMethodEquality > > This test modifies some static fields in HttpClient so should run in /othervm Fixed thanks! - PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v3]
> Hi,
>
> Please review my fix for JDK-8274779 which changes how HttpClient and
> HttpsClient checks for equality when comparing request methods.
>
> When `HttpURLConnection.setRequestMethod` is passed `new String("POST")`
> rather than the "POST" String literal, the old behaviour resulted in broken
> HttpClients being reused from the `KeepAliveCache`.
>
> This is because a call to `HttpClient.available()` was never reachable due to
> identity equality being used instead of logical equality.
>
> The test case uses an injected KeepAliveCache, to which we put a HttpClient
> that is unavailable. By comparing the initial HttpClient's `connectTimeout`
> value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we
> can assert that these values should never be equal as a new HttpClient should
> be created in cases where we can no longer use the cached one.
>
> All CI testing is green for this fix.
>
> Kind regards,
> Evan
Evan Whelan has updated the pull request incrementally with two additional
commits since the last revision:
- Extracted both connectTimeouts to variables and added copyright header to
accessor
- Removed reflection in favour of injected accessor for KeepAliveCache
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5964/files
- new: https://git.openjdk.java.net/jdk/pull/5964/files/a65e493d..90ab8587
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=01-02
Stats: 46 lines in 3 files changed: 35 ins; 8 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/5964.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5964/head:pull/5964
PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v3]
On Fri, 15 Oct 2021 09:38:50 GMT, Daniel Fuchs wrote: >> Evan Whelan has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Extracted both connectTimeouts to variables and added copyright header to >> accessor >> - Removed reflection in favour of injected accessor for KeepAliveCache > > test/jdk/sun/net/www/http/KeepAliveCache/RequestMethodEquality.java line 95: > >> 93: // Injecting a mock KeepAliveCache that the HttpClient can >> use >> 94: KeepAliveCache kac = new KeepAliveCache(); >> 95: kac.put(url, null, freshClient); > > Instead of injecting a new KeepAliveCache you could get the cache already > present in HttpClient. One way to do this is to use reflection - another is > to inject an accessor class in the sun.net.www.http package. You may not even > need to have to create a "fresh" HttpClient - there should be one there > corresponding to the connection you already created? Thanks Daniel! I've used an injected accessor instead like you've suggested. In regards to creating a "fresh" client. From looking at the implementation of HttpURLConnection, the internal HttpClient (namely, `http`) is not actually initialised during this test. There are various methods in which it gets initialised (`setNewClient`, `proxiedConnect`, `plainConnect0`). With the way I've written this test, none of these methods are called and the internal `http` object remains null throughout. This is why I am explicitly creating a fresh and cached client. By setting the differing `connectTimeout` values and comparing them, I feel it nicely demonstrates whether or not the same client is being used, while trying to minimise the amount of actual network communication that gets carried out. I verified the test behaviour before and after the source fix and before, the `cachedClient` was always being initialised with the broken client we manually inserted into the cache - PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v3]
On Fri, 15 Oct 2021 15:25:33 GMT, Daniel Fuchs wrote:
>> Evan Whelan has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Extracted both connectTimeouts to variables and added copyright header to
>> accessor
>> - Removed reflection in favour of injected accessor for KeepAliveCache
>
> test/jdk/sun/net/www/http/RequestMethodCheck/RequestMethodEquality.java line
> 96:
>
>> 94: Field inCache = HttpClient.class.getDeclaredField("inCache");
>> 95: inCache.setAccessible(true);
>> 96: inCache.setBoolean(freshClient, true); // allows the
>> assertion in HttpClient.New to pass
>
> You can use the HttpClientAccess to get and set this field too, since the
> field is protected in HttpClient.
> Just add a method:
>
> public void setInCache(HttpClient client, boolean inCache) { client.inCache =
> inCache; }
>
> to HttpClientAccess.
You're right, made that change! Thanks
-
PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v4]
> Hi,
>
> Please review my fix for JDK-8274779 which changes how HttpClient and
> HttpsClient checks for equality when comparing request methods.
>
> When `HttpURLConnection.setRequestMethod` is passed `new String("POST")`
> rather than the "POST" String literal, the old behaviour resulted in broken
> HttpClients being reused from the `KeepAliveCache`.
>
> This is because a call to `HttpClient.available()` was never reachable due to
> identity equality being used instead of logical equality.
>
> The test case uses an injected KeepAliveCache, to which we put a HttpClient
> that is unavailable. By comparing the initial HttpClient's `connectTimeout`
> value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we
> can assert that these values should never be equal as a new HttpClient should
> be created in cases where we can no longer use the cached one.
>
> All CI testing is green for this fix.
>
> Kind regards,
> Evan
Evan Whelan has updated the pull request incrementally with one additional
commit since the last revision:
Added setInCache to HttpClientAccess to replace reflection
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5964/files
- new: https://git.openjdk.java.net/jdk/pull/5964/files/90ab8587..e5de4b6f
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=02-03
Stats: 8 lines in 2 files changed: 4 ins; 3 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/5964.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5964/head:pull/5964
PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v5]
> Hi,
>
> Please review my fix for JDK-8274779 which changes how HttpClient and
> HttpsClient checks for equality when comparing request methods.
>
> When `HttpURLConnection.setRequestMethod` is passed `new String("POST")`
> rather than the "POST" String literal, the old behaviour resulted in broken
> HttpClients being reused from the `KeepAliveCache`.
>
> This is because a call to `HttpClient.available()` was never reachable due to
> identity equality being used instead of logical equality.
>
> The test case uses an injected KeepAliveCache, to which we put a HttpClient
> that is unavailable. By comparing the initial HttpClient's `connectTimeout`
> value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we
> can assert that these values should never be equal as a new HttpClient should
> be created in cases where we can no longer use the cached one.
>
> All CI testing is green for this fix.
>
> Kind regards,
> Evan
Evan Whelan has updated the pull request incrementally with one additional
commit since the last revision:
Removed +open from @modules
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5964/files
- new: https://git.openjdk.java.net/jdk/pull/5964/files/e5de4b6f..f5b0d2d8
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=03-04
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/5964.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5964/head:pull/5964
PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v4]
On Tue, 19 Oct 2021 17:19:08 GMT, Daniel Fuchs wrote: >> Evan Whelan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added setInCache to HttpClientAccess to replace reflection > > test/jdk/sun/net/www/http/RequestMethodCheck/RequestMethodEquality.java line > 30: > >> 28: * @bug 8274779 >> 29: * @library /test/lib >> 30: * @modules java.base/sun.net.www.http:+open > > I think that now you can probably remove `:+open` - since you're no longer > using reflection. Done, thanks! - PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v6]
> Hi,
>
> Please review my fix for JDK-8274779 which changes how HttpClient and
> HttpsClient checks for equality when comparing request methods.
>
> When `HttpURLConnection.setRequestMethod` is passed `new String("POST")`
> rather than the "POST" String literal, the old behaviour resulted in broken
> HttpClients being reused from the `KeepAliveCache`.
>
> This is because a call to `HttpClient.available()` was never reachable due to
> identity equality being used instead of logical equality.
>
> The test case uses an injected KeepAliveCache, to which we put a HttpClient
> that is unavailable. By comparing the initial HttpClient's `connectTimeout`
> value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we
> can assert that these values should never be equal as a new HttpClient should
> be created in cases where we can no longer use the cached one.
>
> All CI testing is green for this fix.
>
> Kind regards,
> Evan
Evan Whelan has refreshed the contents of this pull request, and previous
commits have been removed. The incremental views will show differences compared
to the previous content of the PR. The pull request contains one new commit
since the last revision:
Removed +open from jtreg modules
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5964/files
- new: https://git.openjdk.java.net/jdk/pull/5964/files/f5b0d2d8..13257929
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=05
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5964&range=04-05
Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/5964.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5964/head:pull/5964
PR: https://git.openjdk.java.net/jdk/pull/5964
Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v5]
On Wed, 20 Oct 2021 14:52:45 GMT, Evan Whelan wrote:
>> Hi,
>>
>> Please review my fix for JDK-8274779 which changes how HttpClient and
>> HttpsClient checks for equality when comparing request methods.
>>
>> When `HttpURLConnection.setRequestMethod` is passed `new String("POST")`
>> rather than the "POST" String literal, the old behaviour resulted in broken
>> HttpClients being reused from the `KeepAliveCache`.
>>
>> This is because a call to `HttpClient.available()` was never reachable due
>> to identity equality being used instead of logical equality.
>>
>> The test case uses an injected KeepAliveCache, to which we put a HttpClient
>> that is unavailable. By comparing the initial HttpClient's `connectTimeout`
>> value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we
>> can assert that these values should never be equal as a new HttpClient
>> should be created in cases where we can no longer use the cached one.
>>
>> All CI testing is green for this fix.
>>
>> Kind regards,
>> Evan
>
> Evan Whelan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed +open from @modules
The force push was just modifying the previous git commit message. Including an
ampersand accidentally tagged a github user with the username "modules"
-
PR: https://git.openjdk.java.net/jdk/pull/5964
Integrated: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST
On Fri, 15 Oct 2021 08:49:20 GMT, Evan Whelan wrote:
> Hi,
>
> Please review my fix for JDK-8274779 which changes how HttpClient and
> HttpsClient checks for equality when comparing request methods.
>
> When `HttpURLConnection.setRequestMethod` is passed `new String("POST")`
> rather than the "POST" String literal, the old behaviour resulted in broken
> HttpClients being reused from the `KeepAliveCache`.
>
> This is because a call to `HttpClient.available()` was never reachable due to
> identity equality being used instead of logical equality.
>
> The test case uses an injected KeepAliveCache, to which we put a HttpClient
> that is unavailable. By comparing the initial HttpClient's `connectTimeout`
> value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we
> can assert that these values should never be equal as a new HttpClient should
> be created in cases where we can no longer use the cached one.
>
> All CI testing is green for this fix.
>
> Kind regards,
> Evan
This pull request has now been integrated.
Changeset: 45ce06c9
Author:Evan Whelan
Committer: Sean Coffey
URL:
https://git.openjdk.java.net/jdk/commit/45ce06c9f3e9bee7d4bda313c38f0f0e8786a4db
Stats: 177 lines in 4 files changed: 175 ins; 0 del; 2 mod
8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check
request method when set to POST
Reviewed-by: dfuchs, coffeys, vtewari, michaelm
-
PR: https://git.openjdk.java.net/jdk/pull/5964
