RFR: 8367067: Improve exception handling in HttpRequest.BodyPublishers [v3]

[Volkan Yazici]

On Thu, 18 Sep 2025 16:40:02 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Volkan Yazici has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix `FileChannelPublisherTest` failures
>
> src/java.net.http/share/classes/jdk/internal/net/http/PullPublisher.java line 46:
> 
>> 44:     private final Throwable throwable;
>> 45: 
>> 46:     PullPublisher(CheckedIterable<T> iterable, Throwable throwable) {
> 
> Looking at the usages of this constructor, would it be better if we got rid of this 2-arg constructor and introduced a new
> 
> PullPublisher(Throwable throwable) {
> }
> 
> 
> That way all we have to do is verify that the incoming argument is non-null, both in this constructor and the other existing `PullPublisher(CheckedIterable<T> iterable)`

I agree with your sentiment, which is something I raised [earlier]. I intend to carry out this improvement in a follow-up ticket to avoid bloating this PR – created [JDK-8368077].

[earlier]: https://github.com/openjdk/jdk/pull/26876/files#r2329782120
[JDK-8368077]: https://bugs.openjdk.org/browse/JDK-8368077

> test/jdk/java/net/httpclient/HttpRequestBodyPublishers/OfByteArraysTest.java line 263:
> 
>> 261:      * Initiates tests that depend on a custom-configured JVM.
>> 262:      */
>> 263:     public static void main(String[] args) throws Exception {
> 
> Hello Volkan, I am not completely certain whether having a test class which has both a `@run main` and a `@run junit` is guaranteed to always work. What I mean is, I don't know if jtreg will setup the (runtime) classpath to always include junit libraries which will be required by this class (since it references those junit classes) when running the `@run main` action. It might be better to separate out the junit testing and the main() testing into 2 separate test classes.

You have a very valid point. `@run main` executions should indeed fail due to missing JUnit classes – yet it works, which is, IMHO, unexpected. Agreeing with your sentiment, and putting aside the fact that _"it works"_, I've tried to refactor these `@run main` tests out to separate classes, but I ended up duplicating quite some code. Later on I've checked this issue with @sormuras and we've found that, except `@run java`, all `@run` executions in the same `@test` block get JUnit/TestNG added to their classpath in the presence of a `@run junit`/`@run testng`. Since this is an established JTreg ~bug~ feature, and helps with DRY, I will keep the code as is for the time being.

> test/jdk/java/net/httpclient/HttpRequestBodyPublishers/OfFileTest.java line 63:
> 
>> 61: public class OfFileTest {
>> 62: 
>> 63:     private static final Path DEFAULT_FS_DIR = Path.of(System.getProperty("java.io.tmpdir"));
> 
> Is it intentional to use `java.io.tmpdir` as the Path instead of the test's scratch dir - `Path.of(".")`?

Fixed in 6e46e61eb9. Adopted the code in `jdk.test.lib.Utils::createTempFile`.

> test/jdk/java/net/httpclient/HttpRequestBodyPublishers/OfFileTest.java line 72:
> 
>> 70:         try {
>> 71:             Path zipFile = DEFAULT_FS_DIR.resolve("file.zip");
>> 72:             FileSystem zipFS = FileSystems.newFileSystem(zipFile, Map.of("create", "true"));
> 
> This can lead to the `FileSystem` not being closed even after the test completes. I think we should move out the `FileSystem` instance out of this method into a class level field which can then be closed when the test is done.

Fixed in 11a74dde85.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2362172857
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2362381864
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2362233541
PR Review Comment: https://git.openjdk.org/jdk/pull/26876#discussion_r2362290580