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

[Jaikiran Pai]

On Wed, 22 Jan 2025 08:56:32 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> 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?

> 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.

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

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