RFR: 8329829: HttpClient: Add a BodyPublishers.ofFileChannel method [v7]

[Volkan Yazici]

On Sat, 9 Aug 2025 13:00:50 GMT, Shaojin Wen <swen at openjdk.org> wrote:

>> Volkan Yazici has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Revert the last commit: faa5d24d831
>>   
>>   The `send()`-vs-`sendAsync()` discrepancy will be addressed separately.
>
> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 755:
> 
>> 753:          */
>> 754:         public static BodyPublisher ofFileChannel(FileChannel channel, long offset, long length) throws IOException {
>> 755:             Objects.requireNonNull(channel, "channel");
> 
> public FileChannelPublisher(FileChannel channel, long offset, long length) throws IOException {
>             this.channel = Objects.requireNonNull(channel, "channel");
>             // ...
> 
> The FileChannelPublisher constructor already has a null value check for the parameter channel, which is redundant

We should raise an exception at the API boundary, hence this check. Why didn't I validate `offset` and `length` here too? Because doing so would expose implementation details in the interface. Why did I add an extra null check in `FCP::new`? That's also an API boundary, but an internal one. Naturally, these practices can vary depending on performance considerations.

> src/java.net.http/share/classes/jdk/internal/net/http/common/Utils.java line 368:
> 
>> 366: 
>> 367:     public static IllegalArgumentException newIAE(String message, Object... args) {
>> 368:         return new IllegalArgumentException(format(message, args));
> 
> Suggestion:
> 
>         return new IllegalArgumentException(message.formatted(args));

While I agree with your reasoning, it is out of the scope of this work.

For the record, I really like `String::formatted`! Yet one should try to avoid it if the associated changes might need to be backported. 😉

> test/jdk/java/net/httpclient/FileChannelPublisherTest.java line 105:
> 
>> 103:     private static final ServerRequestPair HTTP2 = ServerRequestPair.of(Version.HTTP_2, false);
>> 104: 
>> 105:     private static final ServerRequestPair HTTPS2 = ServerRequestPair.of(Version.HTTP_2, true);
> 
> Suggestion:
> 
>     private static final ServerRequestPair 
>             HTTP1  = ServerRequestPair.of(Version.HTTP_1_1, false), 
>             HTTPS1 = ServerRequestPair.of(Version.HTTP_1_1, true),
>             HTTP2  = ServerRequestPair.of(Version.HTTP_2, false),
>             HTTPS2 = ServerRequestPair.of(Version.HTTP_2, true);
> 
> This coding style can be considered for declaring multiple consecutive fields of the same type.

Applied in 91d3422. (Did not commit your suggestion due to the _"vertical alignment"_ you employed in the variable names. It is impossible to adhere to in the long run, see `vmIntrinsics.hpp` for an example.)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2265922312
PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2265925853
PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2265925993