Daniel, > On 14 Apr 2020, at 16:37, Daniel Fuchs <daniel.fu...@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.
