From 3f563d3b0dee482b8cc70bae68346d9a6d4609a6 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 9 Apr 2025 15:42:46 -0700 Subject: [PATCH] quic: use an enum for sentPacket state The sentPacket type tracks the state of a packet sent to the peer. Packets can be in progress, acknowledged, or lost. Track this state with an enum rather than a set of bools, to avoid the possibility of nonsensical states such as a packet being both acknowledged and lost. This also simplifies a following change to add an "unsent" state for intentionally skipped packet numbers. Change-Id: I87c8fc399c72337c033ab7ec5ec8db2c56c732f9 Reviewed-on: https://go-review.googlesource.com/c/net/+/664297 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam Auto-Submit: Damien Neil --- quic/loss.go | 18 +++++++++--------- quic/sent_packet.go | 11 +++++++++-- quic/sent_packet_list.go | 2 +- quic/sent_packet_list_test.go | 4 ++-- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/quic/loss.go b/quic/loss.go index 595e30f7..b89aabdf 100644 --- a/quic/loss.go +++ b/quic/loss.go @@ -246,20 +246,20 @@ func (c *lossState) receiveAckRange(now time.Time, space numberSpace, rangeIndex // If the latest packet in the ACK frame is newly-acked, // record the RTT in c.ackFrameRTT. sent := c.spaces[space].num(end - 1) - if !sent.acked { + if sent.state == sentPacketSent { c.ackFrameRTT = max(0, now.Sub(sent.time)) } } for pnum := start; pnum < end; pnum++ { sent := c.spaces[space].num(pnum) - if sent.acked || sent.lost { + if sent.state != sentPacketSent { continue } // This is a newly-acknowledged packet. if pnum > c.spaces[space].maxAcked { c.spaces[space].maxAcked = pnum } - sent.acked = true + sent.state = sentPacketAcked c.cc.packetAcked(now, sent) ackf(space, sent, packetAcked) if sent.ackEliciting { @@ -315,12 +315,12 @@ func (c *lossState) receiveAckEnd(now time.Time, log *slog.Logger, space numberS func (c *lossState) discardPackets(space numberSpace, log *slog.Logger, lossf func(numberSpace, *sentPacket, packetFate)) { for i := 0; i < c.spaces[space].size; i++ { sent := c.spaces[space].nth(i) - if sent.acked || sent.lost { + if sent.state != sentPacketSent { // This should not be possible, since we only discard packets // in spaces which have never received an ack, but check anyway. continue } - sent.lost = true + sent.state = sentPacketLost c.cc.packetDiscarded(sent) lossf(numberSpace(space), sent, packetLost) } @@ -335,7 +335,7 @@ func (c *lossState) discardKeys(now time.Time, log *slog.Logger, space numberSpa // https://www.rfc-editor.org/rfc/rfc9002.html#section-6.4 for i := 0; i < c.spaces[space].size; i++ { sent := c.spaces[space].nth(i) - if sent.acked || sent.lost { + if sent.state != sentPacketSent { continue } c.cc.packetDiscarded(sent) @@ -362,7 +362,7 @@ func (c *lossState) detectLoss(now time.Time, lossf func(numberSpace, *sentPacke for space := numberSpace(0); space < numberSpaceCount; space++ { for i := 0; i < c.spaces[space].size; i++ { sent := c.spaces[space].nth(i) - if sent.lost || sent.acked { + if sent.state != sentPacketSent { continue } // RFC 9002 Section 6.1 states that a packet is only declared lost if it @@ -378,13 +378,13 @@ func (c *lossState) detectLoss(now time.Time, lossf func(numberSpace, *sentPacke case sent.num <= c.spaces[space].maxAcked && !sent.time.After(lossTime): // Time threshold // https://www.rfc-editor.org/rfc/rfc9002.html#section-6.1.2 - sent.lost = true + sent.state = sentPacketLost lossf(space, sent, packetLost) if sent.inFlight { c.cc.packetLost(now, space, sent, &c.rtt) } } - if !sent.lost { + if sent.state != sentPacketLost { break } } diff --git a/quic/sent_packet.go b/quic/sent_packet.go index 06960276..457c50e6 100644 --- a/quic/sent_packet.go +++ b/quic/sent_packet.go @@ -19,10 +19,9 @@ type sentPacket struct { time time.Time // time sent ptype packetType + state sentPacketState ackEliciting bool // https://www.rfc-editor.org/rfc/rfc9002.html#section-2-3.4.1 inFlight bool // https://www.rfc-editor.org/rfc/rfc9002.html#section-2-3.6.1 - acked bool // ack has been received - lost bool // packet is presumed lost // Frames sent in the packet. // @@ -36,6 +35,14 @@ type sentPacket struct { n int // read offset into b } +type sentPacketState uint8 + +const ( + sentPacketSent = sentPacketState(iota) // sent but neither acked nor lost + sentPacketAcked // acked + sentPacketLost // declared lost +) + var sentPool = sync.Pool{ New: func() any { return &sentPacket{} diff --git a/quic/sent_packet_list.go b/quic/sent_packet_list.go index 38704616..04116f21 100644 --- a/quic/sent_packet_list.go +++ b/quic/sent_packet_list.go @@ -67,7 +67,7 @@ func (s *sentPacketList) num(num packetNumber) *sentPacket { func (s *sentPacketList) clean() { for s.size > 0 { sent := s.p[s.off] - if !sent.acked && !sent.lost { + if sent.state == sentPacketSent { return } sent.recycle() diff --git a/quic/sent_packet_list_test.go b/quic/sent_packet_list_test.go index 056ed06f..44c99c53 100644 --- a/quic/sent_packet_list_test.go +++ b/quic/sent_packet_list_test.go @@ -26,7 +26,7 @@ func TestSentPacketListSlidingWindow(t *testing.T) { if got := list.nth(0); got != sent { t.Fatalf("list.nth(0) != list.num(%v)", prev) } - sent.acked = true + sent.state = sentPacketAcked list.clean() if got := list.num(prev); got != nil { t.Fatalf("list.num(%v) = packet %v, expected it to be discarded", prev, got.num) @@ -82,7 +82,7 @@ func TestSentPacketListCleanAll(t *testing.T) { } // Mark all the packets as acked. for i := packetNumber(0); i < count; i++ { - list.num(i).acked = true + list.num(i).state = sentPacketAcked } list.clean() if got, want := list.size, 0; got != want {