8238270: java.net HTTP/2 client does not decrease stream count when receives 204 response

[Chris Hegarty]

Daniel,

> On 14 Apr 2020, at 16:37, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> Hi,
> 
> Please find below a fix for:
> 
> 8238270: java.net HTTP/2 client does not decrease stream count
>         when receives 204 response
> https://bugs.openjdk.java.net/browse/JDK-8238270
> 
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8238270/webrev.00/index.html
> 
> For convenience reason, the Http2Connection handles header's frame
> END_STREAM flag by generating a fake dummy and empty data frame
> with the END_STREAM flag set.

Which seems reasonable.

> The empty data frame is later handled by the code that reads the
> body - so there is no difference between the case where the
> END_STREAM flag is carried by the last header frame, and the
> case where it is carried by the (possibly empty and first) data
> frame.

Again, seems like a reasonable implementation.

> This usually work as expected, except in the case of 204, because
> in that case, no body is expected, and the exchange will not
> register a body subscriber, resulting in that that data frame
> never being processed, and leaking the stream…

Ah, good find. This explains the issue.

> The fix ensures that any pending data frame will still be processed
> after 204 has been received - thus triggering the logic that
> eventually closes the stream.

I suppose that a 204 response MUST have an END_STREAM 
in its final HEADERS / CONTINUATION frame, right?

> A few notes:
> 
> I am not sure the changes to HeaderFrame are 100% correct:
> Should that be an error if END_STREAM was set before END_HEADER?
> Or does END_STREAM implies END_HEADER?

It is allowable for a HEADERS frame to carry an END_STREAM,
but not an END_HEADERS. If this happens, then CONTINUATION
frames must follow, the last of which will carry END_HEADERS.
That probably explains why the END_STREAM handling is done
the way that it is.

> Changes to the HTTP/2 test server might also not have been needed.
> But they do ensure that END_STREAM will always be carried by the
> header frame in case of 204 response.

We may want to test with CONTINUATION frames too.

I remember adding BadHeadersTest, which can produce
CONTINUATION frames. Not sure if that could be used as
a template here or not.

> The test will fail without the fix because the TRACKER finds several
> HTTP/2 streams still open in @AfterClass and throws an exception
> at that point.

The new test is good, but has an unnecessary reference to
AbstractThrowingSubscribers.TestExecutor.

-Chris.