Do not swallow FIN frames without data

[mhils]

This PR fixes #438. Thanks for your hard work on aioquic! 🍰

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ac18ff6) 100.00% compared to head (5c3b466) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #439   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         4868      4867    -1     
=========================================
- Hits          4868      4867    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jlaine]

Thanks so much for digging into this, it does look like a tricky situation! Double thanks for providing a unit test :)

I haven't looked at this part of the code, so my apologies if my comment is incorrect, but something doesn't feel right about the stream ending in response to use sending something as opposed to receiving an ACK.

[mhils]

See my answer above. One more clarification: This is the stream sender ending, not the entire stream. The stream itself is not necessarily finished (the stream receiver may still be active).

[jlaine]

See my answer above. One more clarification: This is the stream sender ending, not the entire stream. The stream itself is not necessarily finished (the stream receiver may still be active).

Right, I'm quite clear that we're only talking about the sender part, but let's take the case of a unidirectional stream on which we'll only be sending. In this case, when the sender part finishes, the whole stream finishes and we discard its state.

I agree with the additional condition you added (and not self._pending_eof) in on_data_delivery : this is indeed needed.

What I don't agree with is the change in get_frame: simply sending out the FIN doesn't mean we're done, we still want on_data_delivery to inform us the FIN has actually been acknowledged.

[jlaine]

If we remove the change to get_frame, this is how the test would look:

    def test_sender_fin_then_ack(self):
        stream = QuicStream()

        # send some data
        stream.sender.write(b"data")
        frame = stream.sender.get_frame(8)
        self.assertEqual(frame.data, b"data")

        # request EOF
        stream.sender.write(b"", end_stream=True)
        self.assertFalse(stream.sender.buffer_is_empty)

        # receive acknowledgement for data
        stream.sender.on_data_delivery(QuicDeliveryState.ACKED, 0, 4)
        self.assertFalse(stream.sender.is_finished)

        # send EOF
        frame = stream.sender.get_frame(8)
        self.assertEqual(frame.data, b"")
        self.assertTrue(frame.fin)
        self.assertEqual(frame.offset, 4)
        self.assertFalse(stream.sender.is_finished)

        # receive acknowledgement for EOF
        stream.sender.on_data_delivery(QuicDeliveryState.ACKED, 4, 4)
        self.assertTrue(stream.sender.is_finished)

[mhils]

What I don't agree with is the change in get_frame: simply sending out the FIN doesn't mean we're done, we still want on_data_delivery to inform us the FIN has actually been acknowledged.

D'oh! I totally missed that mechanic. Thanks for pointing that out, I've just pushed a fix that removes the premature is_finished assignment.

[jlaine]

If you don't mind I think I need to think some more about whether we're 100% OK now. I'm wondering whether there isn't yet another pitfall here where we have sent out the FIN, but the receiver only ACK'd the data but not the FIN. In this situation, simply looking at the offset which was acknowledged doesn't tell us whether the FIN was ACK'd, or am I wrong?

This might mean changing the on_data_delivery signature to explicitly convey whether the ACK'd frame had the FIN flag set, or even adding an on_fin_delivery method.

[mhils]

Right - I naively expected the connection to handle retransmission, but if that's handled by QuicStreamSender itself then we need a way to detect FIN acks + new state that tracks that. This also needs to handle the case that the ACK for the FIN is not the last ACK for the stream (i.e. there's other data still in flight).

[jlaine]

Superseeded by #441