On Sun, 1 Feb 2026 11:56:11 GMT, Jaikiran Pai <[email protected]> wrote:
>> The issue here is that `HttpURLConnection` is automatically disconnected
>> (`HttpClient` is set to `null`, `connected` is set to `false`) when a
>> response with no response body bytes is received. This happens before a fake
>> empty body input stream is returned to the user. That behaviour also occurs
>> with any method for which `content-length: 0` is returned (GET, POST,
>> custom, anything), and with any status code (204, 304) for which there is no
>> body.
>>
>> In this case, the proposed fix is to store the `SSLSession` in the
>> `AbstractDelegateHttpsURLConnection` subclass until such a time where
>> `disconnect()` is explicitely closed. Information pertaining to SSL, such as
>> server certificates, can be extracted from the saved `SSLSession`.
>
> test/jdk/sun/net/www/protocol/https/HttpsURLConnection/GetServerCertificates.java
> line 140:
>
>> 138: if (!"GET".equals(method)) {
>> 139: uc.setRequestMethod(method);
>> 140: }
>
> Is the use of these conditionals on `GET` intentional? From what I can see,
> we could just call `uc.setRequestMethod(method);` without the `if` blocks.
> It's OK to keep it in this manner if you prefer doing so.
For GET the regular path would be to not set the method - I preferred to keep
the regular path.
> test/jdk/sun/net/www/protocol/https/HttpsURLConnection/GetServerCertificates.java
> line 151:
>
>> 149: .formatted(code, resp));
>> 150:
>> 151: uc.getServerCertificates();
>
> Here and a few other places in this test, should we assert that this returns
> non-null certificates?
I guess we could - but if the bug is not fixed an exception would be thrown
there. Checking the returned certificates didn't look to gain us much.
> test/jdk/sun/net/www/protocol/https/HttpsURLConnection/GetServerCertificates.java
> line 295:
>
>> 293: try {test(args);} catch (Throwable t) {unexpected(t);}
>> 294: System.out.printf("%nPassed = %d, failed = %d%n%n", passed,
>> failed);
>> 295: if (failed > 0) throw new AssertionError("Some tests failed");}
>
> I realize this was the old style of tests we have in some parts of the JDK,
> but maybe for this new test we can simplify this to just something like:
>
> public static void main(String[] args) throws Exception {
> tests(args);
> }
>
> and then let any `check()` calls throw and propagate an exception instead of
> counting the number of pass/fail?
This is copy paste from a nearby test - I first attempted to remove it but then
discovered that I would either have to hard code the name of the class to make
a new instance (new GetServerCertificates()) or make all instance variables and
methods static. In the end I decided to keep it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29489#discussion_r2759569145
PR Review Comment: https://git.openjdk.org/jdk/pull/29489#discussion_r2759573880
PR Review Comment: https://git.openjdk.org/jdk/pull/29489#discussion_r2759559796
