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

[Jaikiran Pai]

On Wed, 22 Jan 2025 15:04:24 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> 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?
>
>> Instead, I need to verify against the cause
> 
> Verifying against the cause is OK. Some of these tests, including this one are crafted based on what the implementation does. As long as we don't make the tests way too tied into the implementation (I believe this one doesn't).
> 
>> 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.
> 
> In its current form in mainline, the exception message is still preserved in the underlying cause. Having said that, I think what you suggest for the `HttpResponseInputStream::current` implementation make sense since the current message says "closed" even if it may not be. If we do considering doing that change, we should do it in a separate issue.

> @jaikiran, if the current resolution (i.e., https://github.com/openjdk/jdk/commit/70387718fabbd63ec94bb2b538e29931dce1fea2) addresses your concern, would you mind resolving this conversation, please?

The change you did does address my review. I however don't see a way to resolve this conversation in the GitHub UI (I anyway don't resolve conversations because I find it hard to follow the discussion in these PRs since GitHub hides resolved conversations).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23096#discussion_r1925487415