diff --git a/internal/ackhandler/packet.go b/internal/ackhandler/packet.go index 9781ff7c..9673a85c 100644 --- a/internal/ackhandler/packet.go +++ b/internal/ackhandler/packet.go @@ -18,7 +18,10 @@ type Packet struct { largestAcked protocol.PacketNumber // if the packet contains an ACK, the LargestAcked value of that ACK - queuedForRetransmission bool + // There are two reasons why a packet cannot be retransmitted: + // * it was already retransmitted + // * this packet is a retransmission, and we already received an ACK for the original packet + canBeRetransmitted bool includedInBytesInFlight bool retransmittedAs []protocol.PacketNumber isRetransmission bool // we need a separate bool here because 0 is a valid packet number diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 31a4e3f0..90f1d292 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -144,10 +144,9 @@ func (h *sentPacketHandler) sentPacketImpl(packet *Packet) bool /* isRetransmitt h.lastSentPacketNumber = packet.PacketNumber - var largestAcked protocol.PacketNumber if len(packet.Frames) > 0 { if ackFrame, ok := packet.Frames[0].(*wire.AckFrame); ok { - largestAcked = ackFrame.LargestAcked + packet.largestAcked = ackFrame.LargestAcked } } @@ -155,10 +154,10 @@ func (h *sentPacketHandler) sentPacketImpl(packet *Packet) bool /* isRetransmitt isRetransmittable := len(packet.Frames) != 0 if isRetransmittable { - packet.largestAcked = largestAcked h.lastSentRetransmittablePacketTime = packet.SendTime packet.includedInBytesInFlight = true h.bytesInFlight += packet.Length + packet.canBeRetransmitted = true } h.congestion.OnPacketSent(packet.SendTime, h.bytesInFlight, packet.PacketNumber, packet.Length, isRetransmittable) @@ -205,9 +204,6 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *wire.AckFrame, withPacketNumbe if err := h.onPacketAcked(p); err != nil { return err } - if len(p.retransmittedAs) == 0 { - h.congestion.OnPacketAcked(p.PacketNumber, p.Length, h.bytesInFlight) - } } if err := h.detectLostPackets(rcvTime); err != nil { @@ -298,9 +294,6 @@ func (h *sentPacketHandler) detectLostPackets(now time.Time) error { if packet.PacketNumber > h.largestAcked { return false, nil } - if packet.queuedForRetransmission { // don't retransmit packets twice - return true, nil - } timeSinceSent := now.Sub(packet.SendTime) if timeSinceSent > delayUntilLost { @@ -313,13 +306,19 @@ func (h *sentPacketHandler) detectLostPackets(now time.Time) error { }) for _, p := range lostPackets { - h.logger.Debugf("\tQueueing packet %#x because it was detected lost", p.PacketNumber) - if err := h.queuePacketForRetransmission(p); err != nil { - return err + // the bytes in flight need to be reduced no matter if this packet will be retransmitted + if p.includedInBytesInFlight { + h.bytesInFlight -= p.Length } - h.bytesInFlight -= p.Length - p.includedInBytesInFlight = false - h.congestion.OnPacketLost(p.PacketNumber, p.Length, h.bytesInFlight) + if p.canBeRetransmitted { + // queue the packet for retransmission, and report the loss to the congestion controller + h.logger.Debugf("\tQueueing packet %#x because it was detected lost", p.PacketNumber) + if err := h.queuePacketForRetransmission(p); err != nil { + return err + } + h.congestion.OnPacketLost(p.PacketNumber, p.Length, h.bytesInFlight) + } + h.packetHistory.Remove(p.PacketNumber) } return nil } @@ -362,55 +361,53 @@ func (h *sentPacketHandler) onPacketAcked(p *Packet) error { h.handshakeCount = 0 // TODO(#497): h.tlpCount = 0 - // find the first packet, from which on we can delete all retransmissions - // example: packet 10 was retransmitted as packet 11 and 12, and - // packet 12 was then retransmitted as 13. - // When receiving an ACK for packet 13, we can remove packets 12 and 13, - // but still need to keep 10 and 11. - first := p - for first.isRetransmission { - previous := h.packetHistory.GetPacket(first.retransmissionOf) - if previous == nil { - return fmt.Errorf("sent packet handler BUG: retransmitted packet for %d not found (should have been %d)", first.PacketNumber, first.retransmissionOf) - } - // if the retransmission of a packet was split, we can't remove it yet - if len(previous.retransmittedAs) > 1 { - break - } - first = previous - } - if first.isRetransmission { - root := h.packetHistory.GetPacket(first.retransmissionOf) - retransmittedAs := make([]protocol.PacketNumber, 0, len(root.retransmittedAs)-1) - for _, pn := range root.retransmittedAs { - if pn != first.PacketNumber { - retransmittedAs = append(retransmittedAs, pn) + // only report the acking of this packet to the congestion controller if: + // * it is a retransmittable packet + // * this packet wasn't retransmitted yet + if p.isRetransmission { + // that the parent doesn't exist is expected to happen every time the original packet was already acked + if parent := h.packetHistory.GetPacket(p.retransmissionOf); parent != nil { + if len(parent.retransmittedAs) == 1 { + parent.retransmittedAs = nil + } else { + // remove this packet from the slice of retransmission + retransmittedAs := make([]protocol.PacketNumber, 0, len(parent.retransmittedAs)-1) + for _, pn := range parent.retransmittedAs { + if pn != p.PacketNumber { + retransmittedAs = append(retransmittedAs, pn) + } + } + parent.retransmittedAs = retransmittedAs } } - root.retransmittedAs = retransmittedAs } - return h.removeAllRetransmissions(first) -} - -func (h *sentPacketHandler) removeAllRetransmissions(p *Packet) error { if p.includedInBytesInFlight { h.bytesInFlight -= p.Length - p.includedInBytesInFlight = false } - if p.queuedForRetransmission { - for _, r := range p.retransmittedAs { - packet := h.packetHistory.GetPacket(r) - if packet == nil { - return fmt.Errorf("sent packet handler BUG: removing packet %d (retransmission of %d) not found in history", r, p.PacketNumber) - } - if err := h.removeAllRetransmissions(packet); err != nil { - return err - } - } + // TODO: this will need to change once we implement sending of probe packets + if p.canBeRetransmitted { + h.congestion.OnPacketAcked(p.PacketNumber, p.Length, h.bytesInFlight) + } + if err := h.stopRetransmissionsFor(p); err != nil { + return err } return h.packetHistory.Remove(p.PacketNumber) } +func (h *sentPacketHandler) stopRetransmissionsFor(p *Packet) error { + if err := h.packetHistory.MarkCannotBeRetransmitted(p.PacketNumber); err != nil { + return err + } + for _, r := range p.retransmittedAs { + packet := h.packetHistory.GetPacket(r) + if packet == nil { + return fmt.Errorf("sent packet handler BUG: marking packet as not retransmittable %d (retransmission of %d) not found in history", r, p.PacketNumber) + } + h.stopRetransmissionsFor(packet) + } + return nil +} + func (h *sentPacketHandler) DequeuePacketForRetransmission() *Packet { if len(h.retransmissionQueue) == 0 { return nil @@ -472,9 +469,13 @@ func (h *sentPacketHandler) ShouldSendNumPackets() int { // retransmit the oldest two packets func (h *sentPacketHandler) queueRTOs() error { + // Queue the first two outstanding packets for retransmission. + // This does NOT declare this packets as lost: + // They are still tracked in the packet history and count towards the bytes in flight. + // TODO: don't report them as lost to the congestion controller for i := 0; i < 2; i++ { if p := h.packetHistory.FirstOutstanding(); p != nil { - h.logger.Debugf("\tQueueing packet %#x for retransmission (RTO), %d outstanding", p.PacketNumber, h.packetHistory.Len()) + h.logger.Debugf("\tQueueing packet %#x for retransmission (RTO)", p.PacketNumber) if err := h.queuePacketForRetransmission(p); err != nil { return err } @@ -488,7 +489,7 @@ func (h *sentPacketHandler) queueRTOs() error { func (h *sentPacketHandler) queueHandshakePacketsForRetransmission() error { var handshakePackets []*Packet h.packetHistory.Iterate(func(p *Packet) (bool, error) { - if !p.queuedForRetransmission && p.EncryptionLevel < protocol.EncryptionForwardSecure { + if p.canBeRetransmitted && p.EncryptionLevel < protocol.EncryptionForwardSecure { handshakePackets = append(handshakePackets, p) } return true, nil @@ -503,7 +504,10 @@ func (h *sentPacketHandler) queueHandshakePacketsForRetransmission() error { } func (h *sentPacketHandler) queuePacketForRetransmission(p *Packet) error { - if _, err := h.packetHistory.QueuePacketForRetransmission(p.PacketNumber); err != nil { + if !p.canBeRetransmitted { + return fmt.Errorf("sent packet handler BUG: packet %d already queued for retransmission", p.PacketNumber) + } + if err := h.packetHistory.MarkCannotBeRetransmitted(p.PacketNumber); err != nil { return err } h.retransmissionQueue = append(h.retransmissionQueue, p) diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index 5c67f97d..1245a13a 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -77,6 +77,19 @@ var _ = Describe("SentPacketHandler", func() { return nil } + losePacket := func(pn protocol.PacketNumber) { + p := getPacket(pn) + ExpectWithOffset(1, p).ToNot(BeNil()) + handler.queuePacketForRetransmission(p) + if p.includedInBytesInFlight { + p.includedInBytesInFlight = false + handler.bytesInFlight -= p.Length + } + r := handler.DequeuePacketForRetransmission() + ExpectWithOffset(1, r).ToNot(BeNil()) + ExpectWithOffset(1, r.PacketNumber).To(Equal(pn)) + } + expectInPacketHistory := func(expected []protocol.PacketNumber) { ExpectWithOffset(1, handler.packetHistory.Len()).To(Equal(len(expected))) for _, p := range expected { @@ -120,6 +133,7 @@ var _ = Describe("SentPacketHandler", func() { handler.SentPacket(nonRetransmittablePacket(&Packet{PacketNumber: 1})) Expect(handler.packetHistory.Len()).To(BeZero()) Expect(handler.lastSentRetransmittablePacketTime).To(BeZero()) + Expect(handler.bytesInFlight).To(BeZero()) }) Context("skipped packet numbers", func() { @@ -268,12 +282,13 @@ var _ = Describe("SentPacketHandler", func() { }) Context("acks and nacks the right packets", func() { - It("adjusts the LargestAcked", func() { + It("adjusts the LargestAcked, and adjusts the bytes in flight", func() { ack := createAck([]wire.AckRange{{First: 0, Last: 5}}) err := handler.ReceivedAck(ack, 1, protocol.EncryptionForwardSecure, time.Now()) Expect(err).ToNot(HaveOccurred()) Expect(handler.largestAcked).To(Equal(protocol.PacketNumber(5))) expectInPacketHistory([]protocol.PacketNumber{6, 7, 8, 9}) + Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(4))) }) It("acks packet 0", func() { @@ -423,19 +438,6 @@ var _ = Describe("SentPacketHandler", func() { }) Context("ACK processing, for retransmitted packets", func() { - losePacket := func(pn protocol.PacketNumber) { - p := getPacket(pn) - ExpectWithOffset(1, p).ToNot(BeNil()) - handler.queuePacketForRetransmission(p) - if p.includedInBytesInFlight { - p.includedInBytesInFlight = false - handler.bytesInFlight -= p.Length - } - r := handler.DequeuePacketForRetransmission() - ExpectWithOffset(1, r).ToNot(BeNil()) - ExpectWithOffset(1, r.PacketNumber).To(Equal(pn)) - } - It("sends a packet as retransmission", func() { // packet 5 was retransmitted as packet 6 handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, Length: 10})) @@ -446,66 +448,17 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(11))) }) - It("removes all retransmissions when the original packet is acked", func() { - // packet 5 was retransmitted as packet 6, which was then retransmitted as packet 8 + It("removes a packet when it is acked", func() { + // packet 5 was retransmitted as packet 6 handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, Length: 10})) losePacket(5) handler.SentPacketsAsRetransmission([]*Packet{retransmittablePacket(&Packet{PacketNumber: 6, Length: 11})}, 5) - losePacket(6) - handler.SentPacketsAsRetransmission([]*Packet{retransmittablePacket(&Packet{PacketNumber: 8, Length: 12})}, 6) - Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(12))) + Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(11))) // ack 5 err := handler.ReceivedAck(&wire.AckFrame{LowestAcked: 5, LargestAcked: 5}, 1, protocol.EncryptionForwardSecure, time.Now()) Expect(err).ToNot(HaveOccurred()) - Expect(handler.packetHistory.Len()).To(BeZero()) - Expect(handler.bytesInFlight).To(BeZero()) - }) - - It("removes all retransmissions when the retransmission is acked", func() { - // the retransmission for packet 5 was split into packets 6 and 8 - handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, Length: 10})) - losePacket(5) - handler.SentPacketsAsRetransmission([]*Packet{retransmittablePacket(&Packet{PacketNumber: 6, Length: 11})}, 5) - losePacket(6) - handler.SentPacketsAsRetransmission([]*Packet{retransmittablePacket(&Packet{PacketNumber: 8, Length: 12})}, 6) - Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(12))) - // ack 8 - err := handler.ReceivedAck(&wire.AckFrame{LowestAcked: 8, LargestAcked: 8}, 1, protocol.EncryptionForwardSecure, time.Now()) - Expect(err).ToNot(HaveOccurred()) - Expect(handler.packetHistory.Len()).To(BeZero()) - Expect(handler.bytesInFlight).To(BeZero()) - }) - - It("removes split retransmissions when the original packet is acked", func() { - // the retransmission for packet 5 was split into 8 and 9 - handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, Length: 10})) - losePacket(5) - handler.SentPacketsAsRetransmission([]*Packet{ - retransmittablePacket(&Packet{PacketNumber: 8, Length: 6}), - retransmittablePacket(&Packet{PacketNumber: 9, Length: 7}), - }, 5) - Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(6 + 7))) - // ack 5 - err := handler.ReceivedAck(&wire.AckFrame{LowestAcked: 5, LargestAcked: 5}, 1, protocol.EncryptionForwardSecure, time.Now()) - Expect(err).ToNot(HaveOccurred()) - Expect(handler.packetHistory.Len()).To(BeZero()) - Expect(handler.bytesInFlight).To(BeZero()) - }) - - It("doesn't remove the original packet if a split retransmission is acked", func() { - // the retransmission for packet 5 was split into 10 and 12 - handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, Length: 10})) - losePacket(5) - handler.SentPacketsAsRetransmission([]*Packet{ - retransmittablePacket(&Packet{PacketNumber: 10, Length: 6}), - retransmittablePacket(&Packet{PacketNumber: 12, Length: 7})}, - 5) - Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(6 + 7))) - // ack 10 - err := handler.ReceivedAck(&wire.AckFrame{LowestAcked: 10, LargestAcked: 10}, 1, protocol.EncryptionForwardSecure, time.Now()) - Expect(err).ToNot(HaveOccurred()) - Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(7))) - expectInPacketHistory([]protocol.PacketNumber{5, 12}) + expectInPacketHistory([]protocol.PacketNumber{6}) + Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(11))) }) It("handles ACKs that ack the original packet as well as the retransmission", func() { @@ -608,6 +561,24 @@ var _ = Describe("SentPacketHandler", func() { handler.OnAlarm() // RTO, meaning 2 lost packets }) + It("doesn't call OnPacketAcked when a retransmitted packet is acked", func() { + cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(2) + cong.EXPECT().TimeUntilSend(gomock.Any()).Times(2) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 2})) + // lose packet 1 + gomock.InOrder( + cong.EXPECT().MaybeExitSlowStart(), + cong.EXPECT().OnPacketAcked(protocol.PacketNumber(2), protocol.ByteCount(1), protocol.ByteCount(1)), + cong.EXPECT().OnPacketLost(protocol.PacketNumber(1), protocol.ByteCount(1), protocol.ByteCount(0)), + ) + err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 2, LowestAcked: 2}, 1, protocol.EncryptionForwardSecure, time.Now()) + Expect(err).ToNot(HaveOccurred()) + // don't EXPECT any further calls to the congestion controller + err = handler.ReceivedAck(&wire.AckFrame{LargestAcked: 2, LowestAcked: 1}, 2, protocol.EncryptionForwardSecure, time.Now()) + Expect(err).ToNot(HaveOccurred()) + }) + It("only allows sending of ACKs when congestion limited", func() { handler.bytesInFlight = 100 cong.EXPECT().GetCongestionWindow().Return(protocol.ByteCount(200)) @@ -711,7 +682,8 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.lossTime.IsZero()).To(BeTrue()) Expect(time.Until(handler.GetAlarmTimeout())).To(BeNumerically("~", handler.computeRTOTimeout(), time.Minute)) - handler.OnAlarm() + err := handler.OnAlarm() + Expect(err).ToNot(HaveOccurred()) p := handler.DequeuePacketForRetransmission() Expect(p).ToNot(BeNil()) Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(1))) @@ -722,6 +694,47 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.rtoCount).To(BeEquivalentTo(1)) }) + + It("doesn't delete packets transmitted as RTO from the history", func() { + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 2, SendTime: time.Now().Add(-time.Hour)})) + handler.rttStats.UpdateRTT(time.Second, 0, time.Now()) + err := handler.OnAlarm() + Expect(err).ToNot(HaveOccurred()) + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + expectInPacketHistory([]protocol.PacketNumber{1, 2}) + Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(2))) + // send another packet and receive an ACK for it + // This will trigger ack-based loss detection, and declare 1 and 2 lost, and remove them from bytes in flight + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 3})) + err = handler.ReceivedAck(&wire.AckFrame{LargestAcked: 3, LowestAcked: 3}, 1, protocol.EncryptionForwardSecure, time.Now()) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.packetHistory.Len()).To(BeZero()) + Expect(handler.bytesInFlight).To(BeZero()) + }) + + It("handles ACKs for the original packet", func() { + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, SendTime: time.Now().Add(-time.Hour)})) + handler.rttStats.UpdateRTT(time.Second, 0, time.Now()) + err := handler.OnAlarm() + Expect(err).ToNot(HaveOccurred()) + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + handler.SentPacketsAsRetransmission([]*Packet{retransmittablePacket(&Packet{PacketNumber: 6})}, 5) + err = handler.ReceivedAck(&wire.AckFrame{LargestAcked: 5, LowestAcked: 5}, 1, protocol.EncryptionForwardSecure, time.Now()) + Expect(err).ToNot(HaveOccurred()) + err = handler.OnAlarm() + Expect(err).ToNot(HaveOccurred()) + }) + + It("handles ACKs for the original packet", func() { + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, SendTime: time.Now().Add(-time.Hour)})) + handler.rttStats.UpdateRTT(time.Second, 0, time.Now()) + err := handler.OnAlarm() + Expect(err).ToNot(HaveOccurred()) + err = handler.OnAlarm() + Expect(err).ToNot(HaveOccurred()) + }) }) Context("Delay-based loss detection", func() { @@ -734,8 +747,10 @@ var _ = Describe("SentPacketHandler", func() { err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 2, LowestAcked: 2}, 1, protocol.EncryptionForwardSecure, now) Expect(err).NotTo(HaveOccurred()) Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) // no need to set an alarm, since packet 1 was already declared lost Expect(handler.lossTime.IsZero()).To(BeTrue()) + Expect(handler.bytesInFlight).To(BeZero()) }) It("sets the early retransmit alarm", func() { @@ -754,7 +769,8 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.lossTime.Sub(getPacket(1).SendTime)).To(Equal(time.Second * 9 / 8)) // Expect(time.Until(handler.GetAlarmTimeout())).To(BeNumerically("~", time.Hour*9/8, time.Minute)) - handler.OnAlarm() + err = handler.OnAlarm() + Expect(err).ToNot(HaveOccurred()) Expect(handler.DequeuePacketForRetransmission()).NotTo(BeNil()) // make sure this is not an RTO: only packet 1 is retransmissted Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) @@ -784,7 +800,8 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.lossTime.IsZero()).To(BeTrue()) Expect(handler.GetAlarmTimeout().Sub(lastHandshakePacketSendTime)).To(Equal(2 * time.Minute)) - handler.OnAlarm() + err = handler.OnAlarm() + Expect(err).ToNot(HaveOccurred()) p := handler.DequeuePacketForRetransmission() Expect(p).ToNot(BeNil()) Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(2))) diff --git a/internal/ackhandler/sent_packet_history.go b/internal/ackhandler/sent_packet_history.go index f815b854..38a2a0e5 100644 --- a/internal/ackhandler/sent_packet_history.go +++ b/internal/ackhandler/sent_packet_history.go @@ -87,26 +87,23 @@ func (h *sentPacketHistory) FirstOutstanding() *Packet { // QueuePacketForRetransmission marks a packet for retransmission. // A packet can only be queued once. -func (h *sentPacketHistory) QueuePacketForRetransmission(pn protocol.PacketNumber) (*Packet, error) { +func (h *sentPacketHistory) MarkCannotBeRetransmitted(pn protocol.PacketNumber) error { el, ok := h.packetMap[pn] if !ok { - return nil, fmt.Errorf("sent packet history: packet %d not found", pn) + return fmt.Errorf("sent packet history: packet %d not found", pn) } - if el.Value.queuedForRetransmission { - return nil, fmt.Errorf("sent packet history BUG: packet %d already queued for retransmission", pn) - } - el.Value.queuedForRetransmission = true + el.Value.canBeRetransmitted = false if el == h.firstOutstanding { h.readjustFirstOutstanding() } - return &el.Value, nil + return nil } // readjustFirstOutstanding readjusts the pointer to the first outstanding packet. // This is necessary every time the first outstanding packet is deleted or retransmitted. func (h *sentPacketHistory) readjustFirstOutstanding() { el := h.firstOutstanding.Next() - for el != nil && el.Value.queuedForRetransmission { + for el != nil && !el.Value.canBeRetransmitted { el = el.Next() } h.firstOutstanding = el diff --git a/internal/ackhandler/sent_packet_history_test.go b/internal/ackhandler/sent_packet_history_test.go index 746bf70f..fae82c2a 100644 --- a/internal/ackhandler/sent_packet_history_test.go +++ b/internal/ackhandler/sent_packet_history_test.go @@ -55,15 +55,15 @@ var _ = Describe("SentPacketHistory", func() { }) It("gets the second packet if the first one is retransmitted", func() { - hist.SentPacket(&Packet{PacketNumber: 1}) - hist.SentPacket(&Packet{PacketNumber: 3}) - hist.SentPacket(&Packet{PacketNumber: 4}) + hist.SentPacket(&Packet{PacketNumber: 1, canBeRetransmitted: true}) + hist.SentPacket(&Packet{PacketNumber: 3, canBeRetransmitted: true}) + hist.SentPacket(&Packet{PacketNumber: 4, canBeRetransmitted: true}) front := hist.FirstOutstanding() Expect(front).ToNot(BeNil()) Expect(front.PacketNumber).To(Equal(protocol.PacketNumber(1))) // Queue the first packet for retransmission. // The first outstanding packet should now be 3. - _, err := hist.QueuePacketForRetransmission(1) + err := hist.MarkCannotBeRetransmitted(1) Expect(err).ToNot(HaveOccurred()) front = hist.FirstOutstanding() Expect(front).ToNot(BeNil()) @@ -71,22 +71,22 @@ var _ = Describe("SentPacketHistory", func() { }) It("gets the third packet if the first two are retransmitted", func() { - hist.SentPacket(&Packet{PacketNumber: 1}) - hist.SentPacket(&Packet{PacketNumber: 3}) - hist.SentPacket(&Packet{PacketNumber: 4}) + hist.SentPacket(&Packet{PacketNumber: 1, canBeRetransmitted: true}) + hist.SentPacket(&Packet{PacketNumber: 3, canBeRetransmitted: true}) + hist.SentPacket(&Packet{PacketNumber: 4, canBeRetransmitted: true}) front := hist.FirstOutstanding() Expect(front).ToNot(BeNil()) Expect(front.PacketNumber).To(Equal(protocol.PacketNumber(1))) // Queue the second packet for retransmission. // The first outstanding packet should still be 3. - _, err := hist.QueuePacketForRetransmission(3) + err := hist.MarkCannotBeRetransmitted(3) Expect(err).ToNot(HaveOccurred()) front = hist.FirstOutstanding() Expect(front).ToNot(BeNil()) Expect(front.PacketNumber).To(Equal(protocol.PacketNumber(1))) // Queue the first packet for retransmission. // The first outstanding packet should still be 4. - _, err = hist.QueuePacketForRetransmission(1) + err = hist.MarkCannotBeRetransmitted(1) Expect(err).ToNot(HaveOccurred()) front = hist.FirstOutstanding() Expect(front).ToNot(BeNil()) @@ -168,26 +168,8 @@ var _ = Describe("SentPacketHistory", func() { } }) - It("gets packets for retransmission", func() { - p, err := hist.QueuePacketForRetransmission(3) - Expect(err).ToNot(HaveOccurred()) - Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(3))) - Expect(p.queuedForRetransmission).To(BeTrue()) - Expect(p.retransmissionOf).To(BeZero()) - Expect(p.isRetransmission).To(BeFalse()) - Expect(p.retransmittedAs).To(BeNil()) - }) - - It("errors if the packet was already queued for retransmission", func() { - p, err := hist.QueuePacketForRetransmission(5) - Expect(err).ToNot(HaveOccurred()) - Expect(p).ToNot(BeNil()) - _, err = hist.QueuePacketForRetransmission(5) - Expect(err).To(MatchError("sent packet history BUG: packet 5 already queued for retransmission")) - }) - It("errors if the packet doesn't exist", func() { - _, err := hist.QueuePacketForRetransmission(100) + err := hist.MarkCannotBeRetransmitted(100) Expect(err).To(MatchError("sent packet history: packet 100 not found")) })