On Mon, 5 Dec 2022 14:06:10 GMT, Daniel Jeliński <[email protected]> wrote:
>> Please review this patch that removes progress monitoring classes used by
>> UrlConnection.
>> Since Java 9 these classes are not used in the JDK, and are not exported
>> from java.base. If anyone was still using them, reimplementing them in user
>> code should be pretty straightforward.
>>
>> This PR also fixes the issue where MeteredStream finalizer could resurrect
>> an unusable connection, causing unexpected exceptions in other parts of the
>> code.
>>
>> No new regression test; since we are removing the finalizer, I don't believe
>> we will see the issue again. I can add a test for that if you think it still
>> makes sense.
>>
>> I had to adjust `ProxyModuleMapping.java` test which used
>> `sun.net.ProgressListener` as an example of a module-private interface; I
>> replaced it with another public interface from the same package.
>>
>> Existing tier 1-3 tests continue to pass.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add test
test/jdk/sun/net/www/http/KeepAliveStream/KeepAliveStreamFinalizer.java line 28:
> 26: * @bug 8240275
> 27: * @library /test/lib
> 28: * @run main/othervm KeepAliveStreamFinalizer
Hello Daniel, it doesn't look like we are doing anything JVM specific in this
test. Is the `othervm` intentional here?
test/jdk/sun/net/www/http/KeepAliveStream/KeepAliveStreamFinalizer.java line
175:
> 173: public InputStream getInputStream() throws IOException {
> 174: if (finalized) {
> 175: System.out.println(failureReason = "getInputStream
> called after finalize");
For ease of debugging, perhaps use `System.err.println` instead of
`System.out`? That way the `Thread.dumpStack()` which is done on the next line
will appear in the same jtreg section as this message.
Same comment in few other places where we have this construct.
-------------
PR: https://git.openjdk.org/jdk/pull/11474
