ignore STOP_WAITINGs, derive lower bound for packets to include in ACK

Changelog.md

@@ -2,6 +2,8 @@
 
 ## v0.6.1 (unreleased)
 
+- The lower boundary for packets included in ACKs is now derived, and the value sent in STOP_WAITING frames is ignored.
+
 ## v0.6.0 (2017-12-12)
 
 - Add support for QUIC 39, drop support for QUIC 35 - 37

ackhandler/interfaces.go

@@ -16,6 +16,7 @@ type SentPacketHandler interface {
 
 	SendingAllowed() bool
 	GetStopWaitingFrame(force bool) *wire.StopWaitingFrame
+	GetLowestPacketNotConfirmedAcked() protocol.PacketNumber
 	ShouldSendRetransmittablePacket() bool
 	DequeuePacketForRetransmission() (packet *Packet)
 	GetLeastUnacked() protocol.PacketNumber

ackhandler/packet.go

@@ -15,7 +15,8 @@ type Packet struct {
 	Length          protocol.ByteCount
 	EncryptionLevel protocol.EncryptionLevel
 
-	SendTime time.Time
+	largestAcked protocol.PacketNumber // if the packet contains an ACK, the LargestAcked value of that ACK
+	sendTime     time.Time
 }
 
 // GetFramesForRetransmission gets all the frames for retransmission

ackhandler/sent_packet_handler.go

@@ -40,6 +40,10 @@ type sentPacketHandler struct {
 
 	largestAcked                 protocol.PacketNumber
 	largestReceivedPacketWithAck protocol.PacketNumber
+	// lowestPacketNotConfirmedAcked is the lowest packet number that we sent an ACK for, but haven't received confirmation, that this ACK actually arrived
+	// example: we send an ACK for packets 90-100 with packet number 20
+	// once we receive an ACK from the peer for packet 20, the lowestPacketNotConfirmedAcked is 101
+	lowestPacketNotConfirmedAcked protocol.PacketNumber
 
 	packetHistory      *PacketList
 	stopWaitingManager stopWaitingManager
@@ -114,11 +118,19 @@ func (h *sentPacketHandler) SentPacket(packet *Packet) error {
 	h.lastSentPacketNumber = packet.PacketNumber
 	now := time.Now()
 
+	var largestAcked protocol.PacketNumber
+	if len(packet.Frames) > 0 {
+		if ackFrame, ok := packet.Frames[0].(*wire.AckFrame); ok {
+			largestAcked = ackFrame.LargestAcked
+		}
+	}
+
 	packet.Frames = stripNonRetransmittableFrames(packet.Frames)
 	isRetransmittable := len(packet.Frames) != 0
 
 	if isRetransmittable {
-		packet.SendTime = now
+		packet.sendTime = now
+		packet.largestAcked = largestAcked
 		h.bytesInFlight += packet.Length
 		h.packetHistory.PushBack(*packet)
 		h.numNonRetransmittablePackets = 0
@@ -146,14 +158,12 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *wire.AckFrame, withPacketNumbe
 	// duplicate or out-of-order ACK
 	// if withPacketNumber <= h.largestReceivedPacketWithAck && withPacketNumber != 0 {
 	if withPacketNumber <= h.largestReceivedPacketWithAck {
-		utils.Debugf("ignoring ack because duplicate")
 		return ErrDuplicateOrOutOfOrderAck
 	}
 	h.largestReceivedPacketWithAck = withPacketNumber
 
 	// ignore repeated ACK (ACKs that don't have a higher LargestAcked than the last ACK)
 	if ackFrame.LargestAcked < h.lowestUnacked() {
-		utils.Debugf("ignoring ack because repeated")
 		return nil
 	}
 	h.largestAcked = ackFrame.LargestAcked
@@ -178,6 +188,12 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *wire.AckFrame, withPacketNumbe
 			if encLevel < p.Value.EncryptionLevel {
 				return fmt.Errorf("Received ACK with encryption level %s that acks a packet %d (encryption level %s)", encLevel, p.Value.PacketNumber, p.Value.EncryptionLevel)
 			}
+			// largestAcked == 0 either means that the packet didn't contain an ACK, or it just acked packet 0
+			// It is safe to ignore the corner case of packets that just acked packet 0, because
+			// the lowestPacketNotConfirmedAcked is only used to limit the number of ACK ranges we will send.
+			if p.Value.largestAcked != 0 {
+				h.lowestPacketNotConfirmedAcked = utils.MaxPacketNumber(h.lowestPacketNotConfirmedAcked, p.Value.largestAcked+1)
+			}
 			h.onPacketAcked(p)
 			h.congestion.OnPacketAcked(p.Value.PacketNumber, p.Value.Length, h.bytesInFlight)
 		}
@@ -192,6 +208,10 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *wire.AckFrame, withPacketNumbe
 	return nil
 }
 
+func (h *sentPacketHandler) GetLowestPacketNotConfirmedAcked() protocol.PacketNumber {
+	return h.lowestPacketNotConfirmedAcked
+}
+
 func (h *sentPacketHandler) determineNewlyAckedPackets(ackFrame *wire.AckFrame) ([]*PacketElement, error) {
 	var ackedPackets []*PacketElement
 	ackRangeIndex := 0
@@ -233,7 +253,7 @@ func (h *sentPacketHandler) maybeUpdateRTT(largestAcked protocol.PacketNumber, a
 	for el := h.packetHistory.Front(); el != nil; el = el.Next() {
 		packet := el.Value
 		if packet.PacketNumber == largestAcked {
-			h.rttStats.UpdateRTT(rcvTime.Sub(packet.SendTime), ackDelay, time.Now())
+			h.rttStats.UpdateRTT(rcvTime.Sub(packet.sendTime), ackDelay, time.Now())
 			return true
 		}
 		// Packets are sorted by number, so we can stop searching
@@ -278,7 +298,7 @@ func (h *sentPacketHandler) detectLostPackets() {
 			break
 		}
 
-		timeSinceSent := now.Sub(packet.SendTime)
+		timeSinceSent := now.Sub(packet.sendTime)
 		if timeSinceSent > delayUntilLost {
 			lostPackets = append(lostPackets, el)
 		} else if h.lossTime.IsZero() {

ackhandler/sent_packet_handler_test.go

@@ -143,7 +143,7 @@ var _ = Describe("SentPacketHandler", func() {
 			packet := Packet{PacketNumber: 1, Frames: []wire.Frame{&streamFrame}, Length: 1}
 			err := handler.SentPacket(&packet)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(handler.packetHistory.Front().Value.SendTime.Unix()).To(BeNumerically("~", time.Now().Unix(), 1))
+			Expect(handler.packetHistory.Front().Value.sendTime.Unix()).To(BeNumerically("~", time.Now().Unix(), 1))
 		})
 
 		It("does not store non-retransmittable packets", func() {
@@ -553,9 +553,9 @@ var _ = Describe("SentPacketHandler", func() {
 			It("computes the RTT", func() {
 				now := time.Now()
 				// First, fake the sent times of the first, second and last packet
-				getPacketElement(1).Value.SendTime = now.Add(-10 * time.Minute)
-				getPacketElement(2).Value.SendTime = now.Add(-5 * time.Minute)
-				getPacketElement(6).Value.SendTime = now.Add(-1 * time.Minute)
+				getPacketElement(1).Value.sendTime = now.Add(-10 * time.Minute)
+				getPacketElement(2).Value.sendTime = now.Add(-5 * time.Minute)
+				getPacketElement(6).Value.sendTime = now.Add(-1 * time.Minute)
 				// Now, check that the proper times are used when calculating the deltas
 				err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 1}, 1, protocol.EncryptionUnencrypted, time.Now())
 				Expect(err).NotTo(HaveOccurred())
@@ -570,12 +570,50 @@ var _ = Describe("SentPacketHandler", func() {
 
 			It("uses the DelayTime in the ack frame", func() {
 				now := time.Now()
-				getPacketElement(1).Value.SendTime = now.Add(-10 * time.Minute)
+				getPacketElement(1).Value.sendTime = now.Add(-10 * time.Minute)
 				err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 1, DelayTime: 5 * time.Minute}, 1, protocol.EncryptionUnencrypted, time.Now())
 				Expect(err).NotTo(HaveOccurred())
 				Expect(handler.rttStats.LatestRTT()).To(BeNumerically("~", 5*time.Minute, 1*time.Second))
 			})
 		})
+
+		Context("determinining, which ACKs we have received an ACK for", func() {
+			BeforeEach(func() {
+				morePackets := []*Packet{
+					&Packet{PacketNumber: 13, Frames: []wire.Frame{&wire.AckFrame{LowestAcked: 80, LargestAcked: 100}, &streamFrame}, Length: 1},
+					&Packet{PacketNumber: 14, Frames: []wire.Frame{&wire.AckFrame{LowestAcked: 50, LargestAcked: 200}, &streamFrame}, Length: 1},
+					&Packet{PacketNumber: 15, Frames: []wire.Frame{&streamFrame}, Length: 1},
+				}
+				for _, packet := range morePackets {
+					err := handler.SentPacket(packet)
+					Expect(err).NotTo(HaveOccurred())
+				}
+			})
+
+			It("determines which ACK we have received an ACK for", func() {
+				err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 15, LowestAcked: 12}, 1, protocol.EncryptionUnencrypted, time.Now())
+				Expect(err).ToNot(HaveOccurred())
+				Expect(handler.GetLowestPacketNotConfirmedAcked()).To(Equal(protocol.PacketNumber(201)))
+			})
+
+			It("doesn't do anything when the acked packet didn't contain an ACK", func() {
+				err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 13, LowestAcked: 13}, 1, protocol.EncryptionUnencrypted, time.Now())
+				Expect(err).ToNot(HaveOccurred())
+				Expect(handler.GetLowestPacketNotConfirmedAcked()).To(Equal(protocol.PacketNumber(101)))
+				err = handler.ReceivedAck(&wire.AckFrame{LargestAcked: 15, LowestAcked: 15}, 2, protocol.EncryptionUnencrypted, time.Now())
+				Expect(err).ToNot(HaveOccurred())
+				Expect(handler.GetLowestPacketNotConfirmedAcked()).To(Equal(protocol.PacketNumber(101)))
+			})
+
+			It("doesn't decrease the value", func() {
+				err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 14, LowestAcked: 14}, 1, protocol.EncryptionUnencrypted, time.Now())
+				Expect(err).ToNot(HaveOccurred())
+				Expect(handler.GetLowestPacketNotConfirmedAcked()).To(Equal(protocol.PacketNumber(201)))
+				err = handler.ReceivedAck(&wire.AckFrame{LargestAcked: 13, LowestAcked: 13}, 2, protocol.EncryptionUnencrypted, time.Now())
+				Expect(err).ToNot(HaveOccurred())
+				Expect(handler.GetLowestPacketNotConfirmedAcked()).To(Equal(protocol.PacketNumber(201)))
+			})
+		})
 	})
 
 	Context("Retransmission handling", func() {
@@ -606,7 +644,7 @@ var _ = Describe("SentPacketHandler", func() {
 		})
 
 		It("dequeues a packet for retransmission", func() {
-			getPacketElement(1).Value.SendTime = time.Now().Add(-time.Hour)
+			getPacketElement(1).Value.sendTime = time.Now().Add(-time.Hour)
 			handler.OnAlarm()
 			Expect(getPacketElement(1)).To(BeNil())
 			Expect(handler.retransmissionQueue).To(HaveLen(1))
@@ -662,7 +700,7 @@ var _ = Describe("SentPacketHandler", func() {
 		Expect(err).NotTo(HaveOccurred())
 		Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(2)))
 
-		handler.packetHistory.Front().Value.SendTime = time.Now().Add(-time.Hour)
+		handler.packetHistory.Front().Value.sendTime = time.Now().Add(-time.Hour)
 		handler.OnAlarm()
 
 		Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(0)))
@@ -799,7 +837,7 @@ var _ = Describe("SentPacketHandler", func() {
 			Expect(handler.lossTime.Sub(time.Now())).To(BeNumerically("~", time.Hour*9/8, time.Minute))
 			Expect(handler.GetAlarmTimeout().Sub(time.Now())).To(BeNumerically("~", time.Hour*9/8, time.Minute))
 
-			handler.packetHistory.Front().Value.SendTime = time.Now().Add(-2 * time.Hour)
+			handler.packetHistory.Front().Value.sendTime = time.Now().Add(-2 * time.Hour)
 			handler.OnAlarm()
 			Expect(handler.DequeuePacketForRetransmission()).NotTo(BeNil())
 		})

internal/mocks/sent_packet_handler.go

@@ -72,6 +72,18 @@ func (_mr *MockSentPacketHandlerMockRecorder) GetLeastUnacked() *gomock.Call {
 	return _mr.mock.ctrl.RecordCallWithMethodType(_mr.mock, "GetLeastUnacked", reflect.TypeOf((*MockSentPacketHandler)(nil).GetLeastUnacked))
 }
 
+// GetLowestPacketNotConfirmedAcked mocks base method
+func (_m *MockSentPacketHandler) GetLowestPacketNotConfirmedAcked() protocol.PacketNumber {
+	ret := _m.ctrl.Call(_m, "GetLowestPacketNotConfirmedAcked")
+	ret0, _ := ret[0].(protocol.PacketNumber)
+	return ret0
+}
+
+// GetLowestPacketNotConfirmedAcked indicates an expected call of GetLowestPacketNotConfirmedAcked
+func (_mr *MockSentPacketHandlerMockRecorder) GetLowestPacketNotConfirmedAcked() *gomock.Call {
+	return _mr.mock.ctrl.RecordCallWithMethodType(_mr.mock, "GetLowestPacketNotConfirmedAcked", reflect.TypeOf((*MockSentPacketHandler)(nil).GetLowestPacketNotConfirmedAcked))
+}
+
 // GetStopWaitingFrame mocks base method
 func (_m *MockSentPacketHandler) GetStopWaitingFrame(_param0 bool) *wire.StopWaitingFrame {
 	ret := _m.ctrl.Call(_m, "GetStopWaitingFrame", _param0)

packet_packer.go

@@ -212,15 +212,15 @@ func (p *packetPacker) composeNextPacket(
 	var payloadFrames []wire.Frame
 
 	// STOP_WAITING and ACK will always fit
-	if p.stopWaiting != nil {
-		payloadFrames = append(payloadFrames, p.stopWaiting)
-		payloadLength += p.stopWaiting.MinLength(p.version)
-	}
-	if p.ackFrame != nil {
+	if p.ackFrame != nil { // ACKs need to go first, so that the sentPacketHandler will recognize them
 		payloadFrames = append(payloadFrames, p.ackFrame)
 		l := p.ackFrame.MinLength(p.version)
 		payloadLength += l
 	}
+	if p.stopWaiting != nil {
+		payloadFrames = append(payloadFrames, p.stopWaiting)
+		payloadLength += p.stopWaiting.MinLength(p.version)
+	}
 
 	p.controlFrameMutex.Lock()
 	for len(p.controlFrames) > 0 {

session.go

@@ -524,8 +524,7 @@ func (s *session) handleFrames(fs []wire.Frame, encLevel protocol.EncryptionLeve
 			s.closeRemote(qerr.Error(frame.ErrorCode, frame.ReasonPhrase))
 		case *wire.GoawayFrame:
 			err = errors.New("unimplemented: handling GOAWAY frames")
-		case *wire.StopWaitingFrame:
-			s.receivedPacketHandler.IgnoreBelow(frame.LeastUnacked)
+		case *wire.StopWaitingFrame: // ignore STOP_WAITINGs
 		case *wire.RstStreamFrame:
 			err = s.handleRstStreamFrame(frame)
 		case *wire.MaxDataFrame:
@@ -617,7 +616,11 @@ func (s *session) handleRstStreamFrame(frame *wire.RstStreamFrame) error {
 }
 
 func (s *session) handleAckFrame(frame *wire.AckFrame, encLevel protocol.EncryptionLevel) error {
-	return s.sentPacketHandler.ReceivedAck(frame, s.lastRcvdPacketNumber, encLevel, s.lastNetworkActivityTime)
+	if err := s.sentPacketHandler.ReceivedAck(frame, s.lastRcvdPacketNumber, encLevel, s.lastNetworkActivityTime); err != nil {
+		return err
+	}
+	s.receivedPacketHandler.IgnoreBelow(s.sentPacketHandler.GetLowestPacketNotConfirmedAcked())
+	return nil
 }
 
 func (s *session) closeLocal(e error) {

session_test.go

@@ -276,13 +276,26 @@ var _ = Describe("Session", func() {
 
 		Context("handling ACK frames", func() {
 			It("informs the SentPacketHandler about ACKs", func() {
+				f := &wire.AckFrame{LargestAcked: 3, LowestAcked: 2}
 				sph := mocks.NewMockSentPacketHandler(mockCtrl)
+				sph.EXPECT().ReceivedAck(f, protocol.PacketNumber(42), protocol.EncryptionSecure, gomock.Any())
+				sph.EXPECT().GetLowestPacketNotConfirmedAcked()
 				sess.sentPacketHandler = sph
 				sess.lastRcvdPacketNumber = 42
-				f := &wire.AckFrame{LargestAcked: 3, LowestAcked: 2}
 				err := sess.handleAckFrame(f, protocol.EncryptionSecure)
 				Expect(err).ToNot(HaveOccurred())
-				sph.EXPECT().ReceivedAck(f, protocol.PacketNumber(42), protocol.EncryptionSecure, gomock.Any())
+			})
+
+			It("tells the ReceivedPacketHandler to ignore low ranges", func() {
+				sph := mocks.NewMockSentPacketHandler(mockCtrl)
+				sph.EXPECT().ReceivedAck(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
+				sph.EXPECT().GetLowestPacketNotConfirmedAcked().Return(protocol.PacketNumber(0x42))
+				sess.sentPacketHandler = sph
+				rph := mocks.NewMockReceivedPacketHandler(mockCtrl)
+				rph.EXPECT().IgnoreBelow(protocol.PacketNumber(0x42))
+				sess.receivedPacketHandler = rph
+				err := sess.handleAckFrame(&wire.AckFrame{LargestAcked: 3, LowestAcked: 2}, protocol.EncryptionUnencrypted)
+				Expect(err).ToNot(HaveOccurred())
 			})
 		})