diff --git a/integrationtests/self/timeout_test.go b/integrationtests/self/timeout_test.go index 125b6674..fc0a3771 100644 --- a/integrationtests/self/timeout_test.go +++ b/integrationtests/self/timeout_test.go @@ -214,17 +214,30 @@ var _ = Describe("Timeout tests", func() { Expect(err).ToNot(HaveOccurred()) defer server.Close() + drop := utils.AtomicBool{} + proxy, err := quicproxy.NewQuicProxy("localhost:0", &quicproxy.Opts{ + RemoteAddr: fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port), + DropPacket: func(dir quicproxy.Direction, _ []byte) bool { + if dir == quicproxy.DirectionOutgoing { + return drop.Get() + } + return false + }, + }) + Expect(err).ToNot(HaveOccurred()) + defer proxy.Close() + serverSessionClosed := make(chan struct{}) go func() { defer GinkgoRecover() sess, err := server.Accept(context.Background()) Expect(err).ToNot(HaveOccurred()) - sess.AcceptStream(context.Background()) // blocks until the session is closed + <-sess.Context().Done() // block until the session is closed close(serverSessionClosed) }() sess, err := quic.DialAddr( - fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port), + fmt.Sprintf("localhost:%d", proxy.LocalPort()), getTLSClientConfig(), getQuicConfig(&quic.Config{MaxIdleTimeout: idleTimeout}), ) @@ -232,6 +245,7 @@ var _ = Describe("Timeout tests", func() { // wait half the idle timeout, then send a packet time.Sleep(idleTimeout / 2) + drop.Set(true) str, err := sess.OpenUniStream() Expect(err).ToNot(HaveOccurred()) _, err = str.Write([]byte("foobar")) @@ -281,7 +295,6 @@ var _ = Describe("Timeout tests", func() { }() drop := utils.AtomicBool{} - proxy, err := quicproxy.NewQuicProxy("localhost:0", &quicproxy.Opts{ RemoteAddr: fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port), DropPacket: func(quicproxy.Direction, []byte) bool { diff --git a/internal/ackhandler/received_packet_handler.go b/internal/ackhandler/received_packet_handler.go index fc74ba2b..f2124fe6 100644 --- a/internal/ackhandler/received_packet_handler.go +++ b/internal/ackhandler/received_packet_handler.go @@ -9,27 +9,6 @@ import ( "github.com/lucas-clemente/quic-go/internal/wire" ) -const ( - // initial maximum number of ack-eliciting packets received before sending an ack. - initialAckElicitingPacketsBeforeAck = 2 - // number of ack-eliciting that an ACK is sent for - ackElicitingPacketsBeforeAck = 10 - // 1/5 RTT delay when doing ack decimation - ackDecimationDelay = 1.0 / 4 - // 1/8 RTT delay when doing ack decimation - shortAckDecimationDelay = 1.0 / 8 - // Minimum number of packets received before ack decimation is enabled. - // This intends to avoid the beginning of slow start, when CWNDs may be - // rapidly increasing. - minReceivedBeforeAckDecimation = 100 - // Maximum number of packets to ack immediately after a missing packet for - // fast retransmission to kick in at the sender. This limit is created to - // reduce the number of acks sent that have no benefit for fast retransmission. - // Set to the number of nacks needed for fast retransmit plus one for protection - // against an ack loss - maxPacketsAfterNewMissing = 4 -) - type receivedPacketHandler struct { sentPackets sentPacketTracker diff --git a/internal/ackhandler/received_packet_tracker.go b/internal/ackhandler/received_packet_tracker.go index 3f4310e5..bc82c89f 100644 --- a/internal/ackhandler/received_packet_tracker.go +++ b/internal/ackhandler/received_packet_tracker.go @@ -8,6 +8,9 @@ import ( "github.com/lucas-clemente/quic-go/internal/wire" ) +// number of ack-eliciting packets received before sending an ack. +const packetsBeforeAck = 2 + type receivedPacketTracker struct { largestObserved protocol.PacketNumber ignoreBelow protocol.PacketNumber @@ -61,7 +64,7 @@ func (h *receivedPacketTracker) ReceivedPacket(packetNumber protocol.PacketNumbe h.maybeQueueAck(packetNumber, rcvTime, shouldInstigateAck, isMissing) } -// IgnoreBelow sets a lower limit for acking packets. +// IgnoreBelow sets a lower limit for acknowledging packets. // Packets with packet numbers smaller than p will not be acked. func (h *receivedPacketTracker) IgnoreBelow(p protocol.PacketNumber) { if p <= h.ignoreBelow { @@ -87,12 +90,10 @@ func (h *receivedPacketTracker) hasNewMissingPackets() bool { return false } highestRange := h.packetHistory.GetHighestAckRange() - return highestRange.Smallest >= h.lastAck.LargestAcked() && highestRange.Len() <= maxPacketsAfterNewMissing + return highestRange.Smallest > h.lastAck.LargestAcked()+1 && highestRange.Len() == 1 } // maybeQueueAck queues an ACK, if necessary. -// It is implemented analogously to Chrome's QuicConnection::MaybeQueueAck() -// in ACK_DECIMATION_WITH_REORDERING mode. func (h *receivedPacketTracker) maybeQueueAck(packetNumber protocol.PacketNumber, rcvTime time.Time, shouldInstigateAck, wasMissing bool) { // always ack the first packet if h.lastAck == nil { @@ -116,46 +117,22 @@ func (h *receivedPacketTracker) maybeQueueAck(packetNumber protocol.PacketNumber if !h.ackQueued && shouldInstigateAck { h.ackElicitingPacketsReceivedSinceLastAck++ - if packetNumber > minReceivedBeforeAckDecimation { - // ack up to 10 packets at once - if h.ackElicitingPacketsReceivedSinceLastAck >= ackElicitingPacketsBeforeAck { - h.ackQueued = true - if h.logger.Debug() { - h.logger.Debugf("\tQueueing ACK because packet %d packets were received after the last ACK (using threshold: %d).", h.ackElicitingPacketsReceivedSinceLastAck, ackElicitingPacketsBeforeAck) - } - } else if h.ackAlarm.IsZero() { - // wait for the minimum of the ack decimation delay or the delayed ack time before sending an ack - ackDelay := utils.MinDuration(h.maxAckDelay, time.Duration(float64(h.rttStats.MinRTT())*float64(ackDecimationDelay))) - h.ackAlarm = rcvTime.Add(ackDelay) - if h.logger.Debug() { - h.logger.Debugf("\tSetting ACK timer to min(1/4 min-RTT, max ack delay): %s (%s from now)", ackDelay, time.Until(h.ackAlarm)) - } + // send an ACK every 2 ack-eliciting packets + if h.ackElicitingPacketsReceivedSinceLastAck >= packetsBeforeAck { + if h.logger.Debug() { + h.logger.Debugf("\tQueueing ACK because packet %d packets were received after the last ACK (using initial threshold: %d).", h.ackElicitingPacketsReceivedSinceLastAck, packetsBeforeAck) } - } else { - // send an ACK every 2 ack-eliciting packets - if h.ackElicitingPacketsReceivedSinceLastAck >= initialAckElicitingPacketsBeforeAck { - if h.logger.Debug() { - h.logger.Debugf("\tQueueing ACK because packet %d packets were received after the last ACK (using initial threshold: %d).", h.ackElicitingPacketsReceivedSinceLastAck, initialAckElicitingPacketsBeforeAck) - } - h.ackQueued = true - } else if h.ackAlarm.IsZero() { - if h.logger.Debug() { - h.logger.Debugf("\tSetting ACK timer to max ack delay: %s", h.maxAckDelay) - } - h.ackAlarm = rcvTime.Add(h.maxAckDelay) + h.ackQueued = true + } else if h.ackAlarm.IsZero() { + if h.logger.Debug() { + h.logger.Debugf("\tSetting ACK timer to max ack delay: %s", h.maxAckDelay) } + h.ackAlarm = rcvTime.Add(h.maxAckDelay) } - // If there are new missing packets to report, set a short timer to send an ACK. + + // Queue an ACK if there are new missing packets to report. if h.hasNewMissingPackets() { - // wait the minimum of 1/8 min RTT and the existing ack time - ackDelay := time.Duration(float64(h.rttStats.MinRTT()) * float64(shortAckDecimationDelay)) - ackTime := rcvTime.Add(ackDelay) - if h.ackAlarm.IsZero() || h.ackAlarm.After(ackTime) { - h.ackAlarm = ackTime - if h.logger.Debug() { - h.logger.Debugf("\tSetting ACK timer to 1/8 min-RTT: %s (%s from now)", ackDelay, time.Until(h.ackAlarm)) - } - } + h.ackQueued = true } } diff --git a/internal/ackhandler/received_packet_tracker_test.go b/internal/ackhandler/received_packet_tracker_test.go index 7de9cda4..108f5aa0 100644 --- a/internal/ackhandler/received_packet_tracker_test.go +++ b/internal/ackhandler/received_packet_tracker_test.go @@ -58,14 +58,6 @@ var _ = Describe("Received Packet Tracker", func() { Expect(tracker.ackQueued).To(BeFalse()) } - receiveAndAckPacketsUntilAckDecimation := func() { - for i := 1; i <= minReceivedBeforeAckDecimation; i++ { - tracker.ReceivedPacket(protocol.PacketNumber(i), time.Time{}, true) - } - Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) - Expect(tracker.ackQueued).To(BeFalse()) - } - It("always queues an ACK for the first packet", func() { tracker.ReceivedPacket(1, time.Now(), true) Expect(tracker.ackQueued).To(BeTrue()) @@ -80,7 +72,7 @@ var _ = Describe("Received Packet Tracker", func() { Expect(tracker.GetAckFrame(true).DelayTime).To(BeNumerically("~", 0, time.Second)) }) - It("queues an ACK for every second ack-eliciting packet at the beginning", func() { + It("queues an ACK for every second ack-eliciting packet", func() { receiveAndAck10Packets() p := protocol.PacketNumber(11) for i := 0; i <= 20; i++ { @@ -106,20 +98,6 @@ var _ = Describe("Received Packet Tracker", func() { Expect(tracker.GetAckFrame(false)).ToNot(BeNil()) }) - It("queues an ACK for every 10 ack-eliciting packet, if they are arriving fast", func() { - receiveAndAck10Packets() - p := protocol.PacketNumber(10000) - for i := 0; i < 9; i++ { - tracker.ReceivedPacket(p, time.Now(), true) - Expect(tracker.ackQueued).To(BeFalse()) - p++ - } - Expect(tracker.GetAlarmTimeout()).NotTo(BeZero()) - tracker.ReceivedPacket(p, time.Now(), true) - Expect(tracker.ackQueued).To(BeTrue()) - Expect(tracker.GetAlarmTimeout()).To(BeZero()) - }) - It("only sets the timer when receiving a ack-eliciting packets", func() { receiveAndAck10Packets() tracker.ReceivedPacket(11, time.Now(), false) @@ -133,80 +111,84 @@ var _ = Describe("Received Packet Tracker", func() { It("queues an ACK if it was reported missing before", func() { receiveAndAck10Packets() - tracker.ReceivedPacket(11, time.Time{}, true) - tracker.ReceivedPacket(13, time.Time{}, true) + tracker.ReceivedPacket(11, time.Now(), true) + tracker.ReceivedPacket(13, time.Now(), true) ack := tracker.GetAckFrame(true) // ACK: 1-11 and 13, missing: 12 Expect(ack).ToNot(BeNil()) Expect(ack.HasMissingRanges()).To(BeTrue()) Expect(tracker.ackQueued).To(BeFalse()) - tracker.ReceivedPacket(12, time.Time{}, false) + tracker.ReceivedPacket(12, time.Now(), false) Expect(tracker.ackQueued).To(BeTrue()) }) It("doesn't queue an ACK if it was reported missing before, but is below the threshold", func() { receiveAndAck10Packets() // 11 is missing - tracker.ReceivedPacket(12, time.Time{}, true) - tracker.ReceivedPacket(13, time.Time{}, true) + tracker.ReceivedPacket(12, time.Now(), true) + tracker.ReceivedPacket(13, time.Now(), true) ack := tracker.GetAckFrame(true) // ACK: 1-10, 12-13 Expect(ack).ToNot(BeNil()) // now receive 11 tracker.IgnoreBelow(12) - tracker.ReceivedPacket(11, time.Time{}, false) + tracker.ReceivedPacket(11, time.Now(), false) ack = tracker.GetAckFrame(true) Expect(ack).To(BeNil()) }) - It("doesn't queue an ACK if the packet closes a gap that was not yet reported", func() { - receiveAndAckPacketsUntilAckDecimation() - p := protocol.PacketNumber(minReceivedBeforeAckDecimation + 1) - tracker.ReceivedPacket(p+1, time.Now(), true) // p is missing now - Expect(tracker.ackQueued).To(BeFalse()) - Expect(tracker.GetAlarmTimeout()).ToNot(BeZero()) - tracker.ReceivedPacket(p, time.Now(), true) // p is not missing any more + It("doesn't recognize in-order packets as out-of-order after raising the threshold", func() { + receiveAndAck10Packets() + Expect(tracker.lastAck.LargestAcked()).To(Equal(protocol.PacketNumber(10))) Expect(tracker.ackQueued).To(BeFalse()) + tracker.IgnoreBelow(11) + tracker.ReceivedPacket(11, time.Now(), true) + Expect(tracker.GetAckFrame(true)).To(BeNil()) }) - It("sets an ACK alarm after 1/4 RTT if it creates a new missing range", func() { - now := time.Now().Add(-time.Hour) - rtt := 80 * time.Millisecond - rttStats.UpdateRTT(rtt, 0, now) - receiveAndAckPacketsUntilAckDecimation() - p := protocol.PacketNumber(minReceivedBeforeAckDecimation + 1) - for i := p; i < p+6; i++ { - tracker.ReceivedPacket(i, now, true) - } - tracker.ReceivedPacket(p+10, now, true) // we now know that packets p+7, p+8 and p+9 - Expect(rttStats.MinRTT()).To(Equal(rtt)) - Expect(tracker.ackAlarm.Sub(now)).To(Equal(rtt / 8)) + It("recognizes out-of-order packets after raising the threshold", func() { + receiveAndAck10Packets() + Expect(tracker.lastAck.LargestAcked()).To(Equal(protocol.PacketNumber(10))) + Expect(tracker.ackQueued).To(BeFalse()) + tracker.IgnoreBelow(11) + tracker.ReceivedPacket(12, time.Now(), true) ack := tracker.GetAckFrame(true) - Expect(ack.HasMissingRanges()).To(BeTrue()) Expect(ack).ToNot(BeNil()) + Expect(ack.AckRanges).To(Equal([]wire.AckRange{{Smallest: 12, Largest: 12}})) + }) + + It("doesn't queue an ACK if for non-ack-eliciting packets arriving out-of-order", func() { + receiveAndAck10Packets() + tracker.ReceivedPacket(11, time.Now(), true) + Expect(tracker.GetAckFrame(true)).To(BeNil()) + tracker.ReceivedPacket(13, time.Now(), false) // receive a non-ack-eliciting packet out-of-order + Expect(tracker.GetAckFrame(true)).To(BeNil()) + }) + + It("doesn't queue an ACK if packets arrive out-of-order, but haven't been acknowledged yet", func() { + receiveAndAck10Packets() + Expect(tracker.lastAck).ToNot(BeNil()) + tracker.ReceivedPacket(12, time.Now(), false) + Expect(tracker.GetAckFrame(true)).To(BeNil()) + // 11 is received out-of-order, but this hasn't been reported in an ACK frame yet + tracker.ReceivedPacket(11, time.Now(), true) + Expect(tracker.GetAckFrame(true)).To(BeNil()) }) }) Context("ACK generation", func() { It("generates an ACK for an ack-eliciting packet, if no ACK is queued yet", func() { - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) // The first packet is always acknowledged. Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) - - tracker.ReceivedPacket(2, time.Time{}, true) - Expect(tracker.GetAckFrame(true)).To(BeNil()) - ack := tracker.GetAckFrame(false) - Expect(ack).ToNot(BeNil()) - Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(1))) - Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(2))) }) It("doesn't generate ACK for a non-ack-eliciting packet, if no ACK is queued yet", func() { - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) // The first packet is always acknowledged. Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) - tracker.ReceivedPacket(2, time.Time{}, false) + tracker.ReceivedPacket(2, time.Now(), false) Expect(tracker.GetAckFrame(false)).To(BeNil()) - tracker.ReceivedPacket(3, time.Time{}, true) + tracker.ReceivedPacket(3, time.Now(), true) ack := tracker.GetAckFrame(false) Expect(ack).ToNot(BeNil()) Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(1))) @@ -219,8 +201,8 @@ var _ = Describe("Received Packet Tracker", func() { }) It("generates a simple ACK frame", func() { - tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ReceivedPacket(2, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) + tracker.ReceivedPacket(2, time.Now(), true) ack := tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(2))) @@ -229,7 +211,7 @@ var _ = Describe("Received Packet Tracker", func() { }) It("generates an ACK for packet number 0", func() { - tracker.ReceivedPacket(0, time.Time{}, true) + tracker.ReceivedPacket(0, time.Now(), true) ack := tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(0))) @@ -238,7 +220,7 @@ var _ = Describe("Received Packet Tracker", func() { }) It("sets the delay time", func() { - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) tracker.ReceivedPacket(2, time.Now().Add(-1337*time.Millisecond), true) ack := tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) @@ -253,11 +235,11 @@ var _ = Describe("Received Packet Tracker", func() { }) It("saves the last sent ACK", func() { - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) ack := tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) Expect(tracker.lastAck).To(Equal(ack)) - tracker.ReceivedPacket(2, time.Time{}, true) + tracker.ReceivedPacket(2, time.Now(), true) tracker.ackQueued = true ack = tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) @@ -265,8 +247,8 @@ var _ = Describe("Received Packet Tracker", func() { }) It("generates an ACK frame with missing packets", func() { - tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ReceivedPacket(4, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) + tracker.ReceivedPacket(4, time.Now(), true) ack := tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(4))) @@ -278,9 +260,9 @@ var _ = Describe("Received Packet Tracker", func() { }) It("generates an ACK for packet number 0 and other packets", func() { - tracker.ReceivedPacket(0, time.Time{}, true) - tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ReceivedPacket(3, time.Time{}, true) + tracker.ReceivedPacket(0, time.Now(), true) + tracker.ReceivedPacket(1, time.Now(), true) + tracker.ReceivedPacket(3, time.Now(), true) ack := tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(3))) @@ -293,8 +275,8 @@ var _ = Describe("Received Packet Tracker", func() { It("doesn't add delayed packets to the packetHistory", func() { tracker.IgnoreBelow(7) - tracker.ReceivedPacket(4, time.Time{}, true) - tracker.ReceivedPacket(10, time.Time{}, true) + tracker.ReceivedPacket(4, time.Now(), true) + tracker.ReceivedPacket(10, time.Now(), true) ack := tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(10))) @@ -303,7 +285,7 @@ var _ = Describe("Received Packet Tracker", func() { It("deletes packets from the packetHistory when a lower limit is set", func() { for i := 1; i <= 12; i++ { - tracker.ReceivedPacket(protocol.PacketNumber(i), time.Time{}, true) + tracker.ReceivedPacket(protocol.PacketNumber(i), time.Now(), true) } tracker.IgnoreBelow(7) // check that the packets were deleted from the receivedPacketHistory by checking the values in an ACK frame @@ -317,14 +299,14 @@ var _ = Describe("Received Packet Tracker", func() { // TODO: remove this test when dropping support for STOP_WAITINGs It("handles a lower limit of 0", func() { tracker.IgnoreBelow(0) - tracker.ReceivedPacket(1337, time.Time{}, true) + tracker.ReceivedPacket(1337, time.Now(), true) ack := tracker.GetAckFrame(true) Expect(ack).ToNot(BeNil()) Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(1337))) }) It("resets all counters needed for the ACK queueing decision when sending an ACK", func() { - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) tracker.ackAlarm = time.Now().Add(-time.Minute) Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) Expect(tracker.GetAlarmTimeout()).To(BeZero()) @@ -333,21 +315,21 @@ var _ = Describe("Received Packet Tracker", func() { }) It("doesn't generate an ACK when none is queued and the timer is not set", func() { - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) tracker.ackQueued = false tracker.ackAlarm = time.Time{} Expect(tracker.GetAckFrame(true)).To(BeNil()) }) It("doesn't generate an ACK when none is queued and the timer has not yet expired", func() { - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) tracker.ackQueued = false tracker.ackAlarm = time.Now().Add(time.Minute) Expect(tracker.GetAckFrame(true)).To(BeNil()) }) It("generates an ACK when the timer has expired", func() { - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(1, time.Now(), true) tracker.ackQueued = false tracker.ackAlarm = time.Now().Add(-time.Minute) Expect(tracker.GetAckFrame(true)).ToNot(BeNil())