On Wed, 30 Jul 2025 17:49:44 GMT, Volkan Yazici <[email protected]> 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
