On Fri, 27 Mar 2026 09:55:57 GMT, Daniel Fuchs <[email protected]> wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - add additional bad status lines
>> - Daniel's review - minor cleanup to the test
>
> test/jdk/java/net/HttpURLConnection/ProxyBadStatusLine.java line 54:
>
>> 52:
>> 53: static List<String> badStatusLines() {
>> 54: return List.of("HTTP/1.",
>
> should we add a blank line here too?
I have added it now. If the response status line doesn't start with the HTTP
protocol version part, then the internal implementation in HttpURLConnection
throws a different error message (it's still a IOException). So I've updated
the test accordingly to allow for expecting different exception messages
depending on the bad status line.
> test/jdk/java/net/HttpURLConnection/ProxyBadStatusLine.java line 107:
>
>> 105: private static final class BadProxyServer implements Runnable,
>> AutoCloseable {
>> 106: private static final int CR = 13;
>> 107: private static final int LF = 10;
>
> Any reason to not initialize the int constant with the corresponding char
> value?
>
> Suggestion:
>
> private static final int CR = '\r';
> private static final int LF = '\n';
Hello Daniel, I actually copied this over from the existing ProxyServer test
library. I have now updated this PR to use this suggestion.
> test/jdk/java/net/HttpURLConnection/ProxyBadStatusLine.java line 207:
>
>> 205: }
>> 206:
>> 207: private static byte[] readRequestLine(InputStream is) throws
>> IOException {
>
> So this does not simply read the request line, but the full request headers
> block, up to 4k. Maybe the name of the method should reflect that.
Good catch, I was experimenting with this method and the final (easier) version
ended up parsing the entire request, but I had forgotten to rename the method.
I have done that now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30466#discussion_r3000705376
PR Review Comment: https://git.openjdk.org/jdk/pull/30466#discussion_r3000694152
PR Review Comment: https://git.openjdk.org/jdk/pull/30466#discussion_r3000698660
