RFR: 8328919: Add BodyHandlers / BodySubscribers methods to handle excessive server input [v10]

[Volkan Yazici]

On Tue, 21 Jan 2025 15:15:06 GMT, Daniel Fuchs <dfuchs at openjdk.org> 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