http3: Trailling grease and push_promise frames do not close the stream

[G1gg1L3s]

Hi! First of all, thanks for your open-source work!

I used aioquic HTTP3 on a server and decided to test it with rust h3-quinn implementation. To my surprise, I encountered a bug with POST requests as the server didn't correctly close the response body.

After some investigation, I pinpointed the cause to a grease frame which h3 crate sends in its stream finish method. It turns out that aioquic HTTP3 connection does not properly close the request stream where there are grease frames after request body. Instead, the library just hangs and awaits more data despite the fact the stream is closed.

Schematically, it can be described as so:

QUIC DATA FRAME (stream_id, [h3_headers, h3_body, h3_grease], stream_ended=True)
        ↓
> HTTP3 Connection
        ↓
H3 HeadersReceived(stream_ended=False)
H3 DataReceived(stream_ended=False)
# grease frame is ignored, but no even with stream_end=True is generated

Hope that makes sense :)

---

Initially, I wanted to submit a PR with a fix, but after diving into the code, I think the problem is more serious.

It turns out that this issue is the same for PUSH_PROMISE frames. If these frames are at the end of stream, the stream is not properly closed by aioquic. You can see here that the PUSH_PROMISE branch doesn't handle stream_ended argument similarly to how it's handled in HEADERS or DATA frames.

I though that adding something like this may be sufficient:

if stream_ended:
    http_events.append(
        DataReceived(
            data=b"",
            stream_id=stream.stream_id,
            stream_ended=True,
        )
    )

But it's not because the PUSH_PROMISE frame may be sent after the trailers, in which case we cannot return more DataReceived (I think, correct me If I'm wrong):

H3 HeadersReceived(stream_ended=False)
H3 DataReceived(stream_ended=False, data=b"...")
H3 HeadersReceived(stream_ended=False) # trailers
H3 PushPromiseReceived
H3 DataReceived(stream_ended=True, data=b"") # probably not okay

For me it seems like we need to introduce another HTTP3 Event to mark an explicit stream end in cases like these. It wouldn't be enough to add stream_ended=False to PushPromiseReceived because it will not work with grease frames:

H3 HeadersReceived(stream_ended=False)
H3 DataReceived(stream_ended=False, data=b"...")
H3 HeadersReceived(stream_ended=False) # trailers
H3 GreaseFrame
H3 DataReceived(stream_ended=True, data=b"") # probably not okay

Or, it should be handled by the users of the API and they should explicitly handle situation when QUIC frame ends a stream, but HTTP3 Connection didn't return any stream_ended marks. But it sounds awful to be honest :)

I've written a couple of test cases so you can check and debug the problem:

diff --git a/tests/test_h3.py b/tests/test_h3.py
index 5c3e47b..1f3f9fb 100644
--- a/tests/test_h3.py
+++ b/tests/test_h3.py
@@ -1,6 +1,7 @@
 import binascii
 import contextlib
 import copy
+import random
 from unittest import TestCase
 
 from aioquic.buffer import Buffer, encode_uint_var
@@ -1332,6 +1333,246 @@ class H3ConnectionTest(TestCase):
             )
             self.assertEqual(h3_transfer(quic_client, h3_server), [])
 
+    def test_grease_after_request_body(self):
+        with h3_client_and_server() as (quic_client, quic_server):
+            h3_client = H3Connection(quic_client)
+            h3_server = H3Connection(quic_server)
+
+            stream_id = quic_client.get_next_available_stream_id()
+            h3_client.send_headers(
+                stream_id=stream_id,
+                headers=[
+                    (b":method", b"POST"),
+                    (b":scheme", b"https"),
+                    (b":authority", b"localhost"),
+                    (b":path", b"/"),
+                    (b"x-foo", b"client"),
+                ],
+                end_stream=False,
+            )
+            h3_client.send_data(
+                stream_id=stream_id, data=b"hello world", end_stream=False
+            )
+            grease_type = random.randint(0, 0x210842108421083) * 0x1F + 0x21
+
+            quic_client.send_stream_data(
+                stream_id=stream_id,
+                data=encode_frame(frame_type=grease_type, frame_data=b"grease"),
+                end_stream=True,
+            )
+
+            events = h3_transfer(quic_client, h3_server)
+            self.assertEqual(
+                events,
+                [
+                    HeadersReceived(
+                        headers=[
+                            (b":method", b"POST"),
+                            (b":scheme", b"https"),
+                            (b":authority", b"localhost"),
+                            (b":path", b"/"),
+                            (b"x-foo", b"client"),
+                        ],
+                        stream_id=stream_id,
+                        stream_ended=False,
+                    ),
+                    DataReceived(data=b"hello world", stream_id=0, stream_ended=False),
+                    DataReceived(data=b"", stream_id=0, stream_ended=True),
+                ],
+            )
+
+    def test_push_promise_after_response_body(self):
+        #
+        #  From RFC9114 4.1:
+        # > A server MAY send one or more PUSH_PROMISE frames before, after, or
+        # > interleaved with the frames of a response message.
+        #
+
+        with h3_client_and_server() as (quic_client, quic_server):
+            h3_client = H3Connection(quic_client)
+            h3_server = H3Connection(quic_server)
+
+            stream_id = quic_client.get_next_available_stream_id()
+            h3_client.send_headers(
+                stream_id=stream_id,
+                headers=[
+                    (b":method", b"GET"),
+                    (b":scheme", b"https"),
+                    (b":authority", b"localhost"),
+                    (b":path", b"/test-promise"),
+                    (b"x-foo", b"client"),
+                ],
+                end_stream=True,
+            )
+
+            events = h3_transfer(quic_client, h3_server)
+            self.assertEqual(
+                events,
+                [
+                    HeadersReceived(
+                        headers=[
+                            (b":method", b"GET"),
+                            (b":scheme", b"https"),
+                            (b":authority", b"localhost"),
+                            (b":path", b"/test-promise"),
+                            (b"x-foo", b"client"),
+                        ],
+                        stream_id=stream_id,
+                        stream_ended=True,
+                    ),
+                ],
+            )
+
+            # send response
+            h3_server.send_headers(
+                stream_id=stream_id,
+                headers=[
+                    (b":status", b"200"),
+                    (b"content-type", b"text/html; charset=utf-8"),
+                ],
+                end_stream=False,
+            )
+            h3_server.send_data(stream_id=stream_id, data=b"html", end_stream=False)
+
+            _push_stream_id = h3_server.send_push_promise(
+                stream_id=stream_id,
+                headers=[
+                    (b":method", b"GET"),
+                    (b":scheme", b"https"),
+                    (b":authority", b"localhost"),
+                    (b":path", b"/my/promise"),
+                ],
+                end_stream=True,
+            )
+
+            # receive response and push promise
+            events = h3_transfer(quic_server, h3_client)
+            self.assertEqual(
+                events,
+                [
+                    HeadersReceived(
+                        headers=[
+                            (b":status", b"200"),
+                            (b"content-type", b"text/html; charset=utf-8"),
+                        ],
+                        stream_id=0,
+                        stream_ended=False,
+                    ),
+                    DataReceived(data=b"html", stream_id=0, stream_ended=False),
+                    PushPromiseReceived(
+                        headers=[
+                            (b":method", b"GET"),
+                            (b":scheme", b"https"),
+                            (b":authority", b"localhost"),
+                            (b":path", b"/my/promise"),
+                        ],
+                        push_id=0,
+                        stream_id=stream_id,
+                    ),
+                    DataReceived(data=b"", stream_id=0, stream_ended=True),
+                ],
+            )
+
+    def test_grease_after_trailers(self):
+        with h3_client_and_server() as (quic_client, quic_server):
+            h3_client = H3Connection(quic_client)
+            h3_server = H3Connection(quic_server)
+
+            # send request with trailers
+            stream_id = quic_client.get_next_available_stream_id()
+            h3_client.send_headers(
+                stream_id=stream_id,
+                headers=[
+                    (b":method", b"GET"),
+                    (b":scheme", b"https"),
+                    (b":authority", b"localhost"),
+                    (b":path", b"/"),
+                ],
+                end_stream=False,
+            )
+            h3_client.send_headers(
+                stream_id=stream_id,
+                headers=[(b"x-some-trailer", b"foo")],
+                end_stream=True,
+            )
+
+            # receive request
+            events = h3_transfer(quic_client, h3_server)
+            self.assertEqual(
+                events,
+                [
+                    HeadersReceived(
+                        headers=[
+                            (b":method", b"GET"),
+                            (b":scheme", b"https"),
+                            (b":authority", b"localhost"),
+                            (b":path", b"/"),
+                        ],
+                        stream_id=stream_id,
+                        stream_ended=False,
+                    ),
+                    HeadersReceived(
+                        headers=[(b"x-some-trailer", b"foo")],
+                        stream_id=stream_id,
+                        stream_ended=True,
+                    ),
+                ],
+            )
+
+            # send response
+            h3_server.send_headers(
+                stream_id=stream_id,
+                headers=[
+                    (b":status", b"200"),
+                    (b"content-type", b"text/html; charset=utf-8"),
+                ],
+                end_stream=False,
+            )
+            h3_server.send_data(
+                stream_id=stream_id,
+                data=b"<html><body>hello</body></html>",
+                end_stream=False,
+            )
+            h3_server.send_headers(
+                stream_id=stream_id,
+                headers=[(b"x-some-trailer", b"bar")],
+                end_stream=False,
+            )
+            # Send grease frame as the last one
+            grease_type = random.randint(0, 0x210842108421083) * 0x1F + 0x21
+
+            quic_client.send_stream_data(
+                stream_id=stream_id,
+                data=encode_frame(frame_type=grease_type, frame_data=b"grease"),
+                end_stream=True,
+            )
+
+            # receive response
+            events = h3_transfer(quic_server, h3_client)
+            self.assertEqual(
+                events,
+                [
+                    HeadersReceived(
+                        headers=[
+                            (b":status", b"200"),
+                            (b"content-type", b"text/html; charset=utf-8"),
+                        ],
+                        stream_id=stream_id,
+                        stream_ended=False,
+                    ),
+                    DataReceived(
+                        data=b"<html><body>hello</body></html>",
+                        stream_id=stream_id,
+                        stream_ended=False,
+                    ),
+                    HeadersReceived(
+                        headers=[(b"x-some-trailer", b"bar")],
+                        stream_id=stream_id,
+                        stream_ended=True,
+                    ),
+                ],
+            )
+
     def test_request_with_trailers(self):
         with h3_client_and_server() as (quic_client, quic_server):
             h3_client = H3Connection(quic_client)