On Tue, 21 Jan 2025 15:15:06 GMT, Daniel Fuchs <[email protected]> wrote:
>>> Perhaps we should also add a test which verifies that the returned
>>> InputStream reaches EOF (and the is.read() calls complete with -1
>>> eventually) when wrapped with a limiting BodyHandler with insufficient
>>> capacity.
>>
>> Looking at the implementation of `BodyHandlers.ofInputStream()` it appears
>> that such cases will result in an IOException being thrown from `read(...)`
>> calls and not `-1`. So I stand corrected - the new test that we add should
>> assert that the `IOException` gets thrown from `is.read(...)`.
>
> Yes - IOException should be thrown since the limiting subscriber will call
> onError() with an IOExeception.
Added tests against `ofInputStream()` in
70387718fabbd63ec94bb2b538e29931dce1fea2 for both happy and unhappy paths. The
latter is interesting. The `HttpResponseInputStream::current` method (used by
`read()`) starts as follows:
if (closed || failed != null) {
throw new IOException("closed", failed);
}
That is, I cannot verify the failure using
assertEquals(exception.getMessage(), "body exceeds capacity: " +
insufficientCapacity)
anymore. Instead, I need to verify against _the cause_:
assertNotNull(exception.getCause());
assertEquals(exception.getCause().getMessage(), "body exceeds capacity: " +
insufficientCapacity);
This made me think, maybe `HttpResponseInputStream::current` should have
started as follows:
if (failed) throw new IOException(failed);
if (closed) throw new IOException("closed");
So that the exception message of `failed` will be preserved.
@jaikiran, if the current resolution (i.e.,
70387718fabbd63ec94bb2b538e29931dce1fea2) addresses your concern, would you
mind resolving this conversation, please?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1924940045
