Re: RFR: 8272702: Resolving URI relative path with no / may lead to incorrect toString [v2]
On Wed, 1 Jun 2022 14:15:31 GMT, Daniel Fuchs wrote:
>> KIRIYAMA Takuya has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8272702: Resolving URI relative path with no / may lead to incorrect
>> toString
>
> src/java.base/share/classes/java/net/URI.java line 2140:
>
>> 2138: } else {
>> 2139: sb.append("/");
>> 2140: }
>
> This is wrong as it will cause
> `URI.create("foo").resolve(URI.create("test"))` to return `"/test"` instead
> of `"test"`
Your comment is correct. The behavior of specifying a relative URI as the base
URI should not change, although rfc2396 recommended that the base URI be an
absolute URI.
I modified to add "/" only if the given base URI is an absolute URI.
-
PR: https://git.openjdk.java.net/jdk/pull/8899
Re: RFR: 8272702: Resolving URI relative path with no / may lead to incorrect toString [v2]
> Consider an authority component without trailing "/" as a base URI. When > resolving a relative path against this base URI, the resulting URI is a > concatenated URI without "/". > This behaviour should be fixed, which is rationalized by > rfc3986#section-5.2.3. > Could you review this fix? KIRIYAMA Takuya has updated the pull request incrementally with one additional commit since the last revision: 8272702: Resolving URI relative path with no / may lead to incorrect toString - Changes: - all: https://git.openjdk.java.net/jdk/pull/8899/files - new: https://git.openjdk.java.net/jdk/pull/8899/files/4bed962e..4d191430 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8899&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8899&range=00-01 Stats: 6 lines in 2 files changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8899.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8899/head:pull/8899 PR: https://git.openjdk.java.net/jdk/pull/8899
Integrated: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet
On Tue, 19 Apr 2022 18:00:19 GMT, XenoAmess wrote: > as title. This pull request has now been integrated. Changeset: e01cd7c3 Author:XenoAmess Committer: Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/e01cd7c3ed923cd19509fc972ba6e4aa2991289f Stats: 154 lines in 29 files changed: 107 ins; 7 del; 40 mod 8284780: Need methods to create pre-sized HashSet and LinkedHashSet Reviewed-by: naoto, bpb, dfuchs, ascarpino - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v19]
On Wed, 8 Jun 2022 17:49:38 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > clean up Calendar I gave a quick look at the security files touched and seems straightforward. I didn't see any problems - Marked as reviewed by ascarpino (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v19]
On Wed, 8 Jun 2022 17:49:38 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > clean up Calendar Running tests and awaiting review from security team. Our internal test system is backlogged and tests might not complete in time to get into JDK 19. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Wed, 1 Jun 2022 18:26:17 GMT, Naoto Sato wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> do it as naotoj said
>
> src/java.base/share/classes/java/util/Calendar.java line 2648:
>
>> 2646: set.add("gregory");
>> 2647: set.add("buddhist");
>> 2648: set.add("japanese");
>
> This can be replaced with `SET = Set.of("gregory", "buddhist", "japanese");`.
@naotoj Yes it can. I did a further clean up to it, please have a look.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v19]
> as title. XenoAmess has updated the pull request incrementally with one additional commit since the last revision: clean up Calendar - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/bacc9ca8..95d59b97 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=18 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=17-18 Stats: 8 lines in 1 file changed: 0 ins; 7 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v18]
> as title. XenoAmess has updated the pull request incrementally with two additional commits since the last revision: - remove null check for Capacitiable in WhiteBoxResizeTest - Rename type variable per CSR request; minor spec wording change. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/98bfb0e1..bacc9ca8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=16-17 Stats: 19 lines in 3 files changed: 0 ins; 13 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Wed, 1 Jun 2022 17:34:04 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> do it as naotoj said > > test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 441: > >> 439: } >> 440: } >> 441: > > This unifies the test cases between the Set and Map factories, which > accomplishes the primary goal. Good. > > The unification is achieved through classic object-oriented polymorphism, > which works fine, though which is rather verbose. This could probably be > reduced with some tinkering on the model, but it's probably reaching the > point where additional tinkering on the model isn't worth it. I'm ok with > sticking with this approach for now. Maybe we can clean it up later, or maybe > not -- it's at least fairly straightforward. > > One issue that contributes to the verbosity is the repeated null checking. > The null checking enables the test code to proceed and end up with -1 as the > capacity if there's a null in there somewhere. This will cause the assertion > to fail. This is good in that it will call attention to itself (as opposed to > silently passing or something). However, if the test cases are set up > properly, they should never run into null. If the null checking weren't done, > an unexpected null will throw NPE, which will be caught be the framework and > reported as an error. > > That seems perfectly fine to me, so I'd suggest simply removing the null > checking. That would also reduce the bulkiness of infrastructure. @stuart-marks done. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8276798: HttpURLConnection sends invalid HTTP request
On Mon, 6 Jun 2022 09:43:50 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to fix > https://bugs.openjdk.java.net/browse/JDK-8276798? > > `sun.net.www.protocol.http.HttpURLConnection` has a (private) `writeRequests` > method. This method is responsible for creating the standard HTTP request > headers (include the request line) and then writing it out to the > `OutputStream` which communicates with the HTTP server. While writing out > these request headers, if there's an IOException, then `HttpURLConnection` > marks a `failedOnce` flag to `true` and attempts to write these again afresh > (after creating new connection to the server). This re-attempt is done just > once. > > As noted in the linked JBS issue, specifically this comment > https://bugs.openjdk.java.net/browse/JDK-8276798?focusedCommentId=14500074&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14500074, > there's a specific case where creating and writing out these request headers > ends up skipping the request line, which causes the server to respond back > with a "Bad Request" response code. > > The commit in this PR removes the use of `failedOnce` flag that was being > used to decide whether or not to write the request line. The existing code > doesn't have any specific comments on why this check was there in first > place, nor does the commit history show anything about this check. However, > reading through that code, my guess is that, it was there to avoid writing > the request line twice when the same `requests` object gets reused during the > re-attempt. I think a better check would be the see if the `requests` already > has the request line and if not add it afresh. > While in this code, I also removed the check where the `failedOnce` flag was > being used to check if the `Connection: Keep-Alive`/`Proxy-Connection: > Keep-alive` header needs to be set. This part of the code already has a call > to `setIfNotSet`, so I don't think we need the `failedOnce` check here. > > tier1, tier2 and tier3 tests have passed without issues. However, given the > nature of this code, I'm not too confident that we have tests which can catch > this issue (and adding one isn't easy), so I would like inputs on whether > this change is good enough or whether it has the potential to cause any > non-obvious regressions. This looks reasonable to me but I would like to get a second opinion. @Michael-Mc-Mahon ? - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/9038
