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

[Jaikiran Pai]

On Wed, 30 Jul 2025 17:49:44 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> Hello Volkan, the specification of `HttpClient.send(...)` (and sendAsync()) is that it throws a checked `IOException`. So any other exceptions that we throw internally (like this one) need to be converted to an `IOException` when it reaches the application code.
>> 
>> We have code in `HttpClientImpl.send(...)` which currently does instanceof checks against these exceptions and converts them to an `IOException`. I'm guessing your test is currently passing, which suggests to me that `sendAsync()` is propagating a `UncheckedIOException` to the application code. Would it be possible to tweak the test a bit to replace that call of `sendAsync()` with a `send()` (even if those tweaked changes cannot be pushed to this PR) and see what gets propagated? I suspect it would be `IOException`.
>
> I've confirmed that updating the test to use `send()` results in `IOE` getting received. I have not updated the PR in this direction, since it adds more boilerplate, and I read your lines as, not a change, but a research request – let me know if you meant otherwise.
> 
>> the specification of `HttpClient.send(...)` (and `sendAsync()`) is that it throws a checked `IOException`
> 
> `HttpClient::send` has the following Javadoc:
> 
> 
>      * @throws IOException if an I/O error occurs when sending or receiving, or
>      *         the client has {@linkplain ##closing shut down}
>      * @throws InterruptedException ...
>      * @throws IllegalArgumentException ...
> 
> 
> whereas `::sendAsync` Javadoc has no details regarding I/O failures. Do you think the fact that `sendAsync()` does *not* have a to-`IOException`-translation is an oversight that should be improved, or an intentional slack for the implementation?

Hello Volkan,

> whereas ::sendAsync Javadoc has no details regarding I/O failures.

The `sendAsync()` has the same specification about IOException as that of `send()`. It says:

* <p> The returned completable future completes exceptionally with:
* <ul>
* <li>{@link IOException} - if an I/O error occurs when sending or receiving,
*      or the client has {@linkplain ##closing shut down}.</li>
* </ul>


> Do you think the fact that sendAsync() does not have a to-IOException-translation is an oversight that should be improved, or an intentional slack for the implementation?

My expectation is that both send() and sendAsync() should propagate the same exception to the application (of course for sendAsync() it would be propagated as a cause in the `ExecutionException`). So this is a surprise to me that we aren't currently doing that. We'll need inputs from Daniel (@dfuch) and Michael (@Michael-Mc-Mahon) whether this is intentional or an oversight.

> I have not updated the PR in this direction, since it adds more boilerplate, and I read your lines as, not a change, but a research request 

Yes, that's OK. Having said that, if it's easy to add a test which uses send() and causes an IOException (due to the FileChannel being closed mid way, for example), then it would be a good thing since that will then bring in the necessary coverage for the exception propagation for that method too.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26155#discussion_r2244469864