remove session.maybeSendPacket, restrict small frame detection to ACKs

ref #217

packet_packer.go

@@ -41,14 +41,14 @@ func newPacketPacker(connectionID protocol.ConnectionID, cryptoSetup *handshake.
 }
 
 func (p *packetPacker) PackConnectionClose(frame *frames.ConnectionCloseFrame, largestObserved protocol.PacketNumber) (*packedPacket, error) {
-	return p.packPacket(nil, []frames.Frame{frame}, largestObserved, true)
+	return p.packPacket(nil, []frames.Frame{frame}, largestObserved, true, false)
 }
 
-func (p *packetPacker) PackPacket(stopWaitingFrame *frames.StopWaitingFrame, controlFrames []frames.Frame, largestObserved protocol.PacketNumber) (*packedPacket, error) {
+func (p *packetPacker) PackPacket(stopWaitingFrame *frames.StopWaitingFrame, controlFrames []frames.Frame, largestObserved protocol.PacketNumber, maySendOnlyAck bool) (*packedPacket, error) {
-	return p.packPacket(stopWaitingFrame, controlFrames, largestObserved, false)
+	return p.packPacket(stopWaitingFrame, controlFrames, largestObserved, false, maySendOnlyAck)
 }
 
-func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, controlFrames []frames.Frame, largestObserved protocol.PacketNumber, onlySendOneControlFrame bool) (*packedPacket, error) {
+func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, controlFrames []frames.Frame, largestObserved protocol.PacketNumber, onlySendOneControlFrame, maySendOnlyAck bool) (*packedPacket, error) {
 	if len(controlFrames) > 0 {
 		p.controlFrames = append(p.controlFrames, controlFrames...)
 	}
@@ -87,10 +87,27 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, con
 		if err != nil {
 			return nil, err
 		}
-		// don't send out packets that only contain a StopWaitingFrame
+	}
-		if len(payloadFrames) == 0 || (stopWaitingFrame != nil && len(payloadFrames) == 1) {
+
+	// Check if we have enough frames to send
+	if len(payloadFrames) == 0 {
 		return nil, nil
 	}
+	// Don't send out packets that only contain a StopWaitingFrame
+	if !onlySendOneControlFrame && len(payloadFrames) == 1 && stopWaitingFrame != nil {
+		return nil, nil
+	}
+	// Don't send out packets that only contain an ACK (plus optional STOP_WAITING), if requested
+	if !maySendOnlyAck {
+		if len(payloadFrames) == 1 {
+			if _, ok := payloadFrames[0].(*frames.AckFrameLegacy); ok {
+				return nil, nil
+			}
+		} else if len(payloadFrames) == 2 && stopWaitingFrame != nil {
+			if _, ok := payloadFrames[1].(*frames.AckFrameLegacy); ok {
+				return nil, nil
+			}
+		}
 	}
 
 	payload, err := p.getPayload(payloadFrames, currentPacketNumber)

packet_packer_test.go

@@ -40,7 +40,7 @@ var _ = Describe("Packet packer", func() {
 	})
 
 	It("returns nil when no packet is queued", func() {
-		p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+		p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 		Expect(p).To(BeNil())
 		Expect(err).ToNot(HaveOccurred())
 	})
@@ -55,13 +55,13 @@ var _ = Describe("Packet packer", func() {
 		// pack the packet for QUIC version 33
 		packer.version = protocol.Version33
 		streamFramer.AddFrameForRetransmission(f)
-		p33, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+		p33, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(p33).ToNot(BeNil())
 		// pack the packet for QUIC version 34
 		packer.version = protocol.Version34
 		streamFramer.AddFrameForRetransmission(f)
-		p34, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+		p34, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(p34).ToNot(BeNil())
 		Expect(p34.entropyBit).To(BeFalse())
@@ -74,7 +74,7 @@ var _ = Describe("Packet packer", func() {
 			Data:     []byte{0xDE, 0xCA, 0xFB, 0xAD},
 		}
 		streamFramer.AddFrameForRetransmission(f)
-		p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+		p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(p).ToNot(BeNil())
 		b := &bytes.Buffer{}
@@ -99,14 +99,14 @@ var _ = Describe("Packet packer", func() {
 			ErrorCode:    0x1337,
 			ReasonPhrase: "foobar",
 		}
-		p, err := packer.packPacket(&frames.StopWaitingFrame{LeastUnacked: 13}, []frames.Frame{&ccf, &frames.WindowUpdateFrame{StreamID: 37}}, 0, true)
+		p, err := packer.packPacket(&frames.StopWaitingFrame{LeastUnacked: 13}, []frames.Frame{&ccf, &frames.WindowUpdateFrame{StreamID: 37}}, 0, true, true)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(p.frames).To(HaveLen(1))
 		Expect(p.frames[0]).To(Equal(&ccf))
 	})
 
 	It("packs only control frames", func() {
-		p, err := packer.PackPacket(nil, []frames.Frame{&frames.ConnectionCloseFrame{}}, 0)
+		p, err := packer.PackPacket(nil, []frames.Frame{&frames.ConnectionCloseFrame{}}, 0, true)
 		Expect(p).ToNot(BeNil())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(p.frames).To(HaveLen(1))
@@ -115,7 +115,7 @@ var _ = Describe("Packet packer", func() {
 
 	It("packs a StopWaitingFrame first", func() {
 		swf := &frames.StopWaitingFrame{LeastUnacked: 10}
-		p, err := packer.PackPacket(swf, []frames.Frame{&frames.ConnectionCloseFrame{}}, 0)
+		p, err := packer.PackPacket(swf, []frames.Frame{&frames.ConnectionCloseFrame{}}, 0, true)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(p).ToNot(BeNil())
 		Expect(p.frames).To(HaveLen(2))
@@ -126,21 +126,21 @@ var _ = Describe("Packet packer", func() {
 		packetNumber := protocol.PacketNumber(0xDECAFB) // will result in a 4 byte packet number
 		packer.lastPacketNumber = packetNumber - 1
 		swf := &frames.StopWaitingFrame{LeastUnacked: packetNumber - 0x100}
-		p, err := packer.PackPacket(swf, []frames.Frame{&frames.ConnectionCloseFrame{}}, 0)
+		p, err := packer.PackPacket(swf, []frames.Frame{&frames.ConnectionCloseFrame{}}, 0, true)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(p.frames[0].(*frames.StopWaitingFrame).PacketNumberLen).To(Equal(protocol.PacketNumberLen4))
 	})
 
 	It("does not pack a packet containing only a StopWaitingFrame", func() {
 		swf := &frames.StopWaitingFrame{LeastUnacked: 10}
-		p, err := packer.PackPacket(swf, []frames.Frame{}, 0)
+		p, err := packer.PackPacket(swf, []frames.Frame{}, 0, true)
 		Expect(p).To(BeNil())
 		Expect(err).ToNot(HaveOccurred())
 	})
 
 	It("packs a packet if it has queued control frames, but no new control frames", func() {
 		packer.controlFrames = []frames.Frame{&frames.BlockedFrame{StreamID: 0}}
-		p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+		p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(p).ToNot(BeNil())
 	})
@@ -188,16 +188,16 @@ var _ = Describe("Packet packer", func() {
 			Data:     []byte{0xDE, 0xCA, 0xFB, 0xAD},
 		}
 		streamFramer.AddFrameForRetransmission(f)
-		p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+		p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 		Expect(p).ToNot(BeNil())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(packer.lastPacketNumber).To(Equal(protocol.PacketNumber(1)))
-		p, err = packer.PackPacket(nil, []frames.Frame{}, 0)
+		p, err = packer.PackPacket(nil, []frames.Frame{}, 0, true)
 		Expect(p).To(BeNil())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(packer.lastPacketNumber).To(Equal(protocol.PacketNumber(1)))
 		streamFramer.AddFrameForRetransmission(f)
-		p, err = packer.PackPacket(nil, []frames.Frame{}, 0)
+		p, err = packer.PackPacket(nil, []frames.Frame{}, 0, true)
 		Expect(p).ToNot(BeNil())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(packer.lastPacketNumber).To(Equal(protocol.PacketNumber(2)))
@@ -237,12 +237,12 @@ var _ = Describe("Packet packer", func() {
 			}
 			streamFramer.AddFrameForRetransmission(f1)
 			streamFramer.AddFrameForRetransmission(f2)
-			p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(p.raw).To(HaveLen(int(protocol.MaxPacketSize - 1)))
 			Expect(p.frames).To(HaveLen(1))
 			Expect(p.frames[0].(*frames.StreamFrame).DataLenPresent).To(BeFalse())
-			p, err = packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err = packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(p.frames).To(HaveLen(1))
 			Expect(p.frames[0].(*frames.StreamFrame).DataLenPresent).To(BeFalse())
@@ -264,7 +264,7 @@ var _ = Describe("Packet packer", func() {
 			streamFramer.AddFrameForRetransmission(f1)
 			streamFramer.AddFrameForRetransmission(f2)
 			streamFramer.AddFrameForRetransmission(f3)
-			p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(p).ToNot(BeNil())
 			Expect(err).ToNot(HaveOccurred())
 			b := &bytes.Buffer{}
@@ -288,7 +288,7 @@ var _ = Describe("Packet packer", func() {
 				Data:     bytes.Repeat([]byte{'f'}, int(protocol.MaxPacketSize)+100),
 			}
 			streamFramer.AddFrameForRetransmission(f)
-			p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(p.raw).To(HaveLen(int(protocol.MaxPacketSize)))
 		})
@@ -331,23 +331,23 @@ var _ = Describe("Packet packer", func() {
 			}
 			streamFramer.AddFrameForRetransmission(f1)
 			streamFramer.AddFrameForRetransmission(f2)
-			p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(p.frames).To(HaveLen(1))
 			Expect(p.frames[0].(*frames.StreamFrame).DataLenPresent).To(BeFalse())
 			Expect(p.raw).To(HaveLen(int(protocol.MaxPacketSize)))
-			p, err = packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err = packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(p.frames).To(HaveLen(2))
 			Expect(p.frames[0].(*frames.StreamFrame).DataLenPresent).To(BeTrue())
 			Expect(p.frames[1].(*frames.StreamFrame).DataLenPresent).To(BeFalse())
 			Expect(err).ToNot(HaveOccurred())
 			Expect(p.raw).To(HaveLen(int(protocol.MaxPacketSize)))
-			p, err = packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err = packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(p.frames).To(HaveLen(1))
 			Expect(p.frames[0].(*frames.StreamFrame).DataLenPresent).To(BeFalse())
 			Expect(err).ToNot(HaveOccurred())
 			Expect(p).ToNot(BeNil())
-			p, err = packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err = packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(p).To(BeNil())
 		})
@@ -360,7 +360,7 @@ var _ = Describe("Packet packer", func() {
 			minLength, _ := f.MinLength(0)
 			f.Data = bytes.Repeat([]byte{'f'}, int(protocol.MaxFrameAndPublicHeaderSize-publicHeaderLen-minLength+1)) // + 1 since MinceLength is 1 bigger than the actual StreamFrame header
 			streamFramer.AddFrameForRetransmission(f)
-			p, err := packer.PackPacket(nil, []frames.Frame{}, 0)
+			p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(p).ToNot(BeNil())
 			Expect(p.raw).To(HaveLen(int(protocol.MaxPacketSize)))
@@ -424,4 +424,28 @@ var _ = Describe("Packet packer", func() {
 			Expect(packer.controlFrames[0]).To(Equal(&frames.BlockedFrame{StreamID: 0}))
 		})
 	})
+
+	It("returns nil if we only have a single STOP_WAITING", func() {
+		p, err := packer.PackPacket(&frames.StopWaitingFrame{}, nil, 0, false)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(p).To(BeNil())
+	})
+
+	It("returns nil if we only have a single STOP_WAITING and an ACK", func() {
+		p, err := packer.PackPacket(&frames.StopWaitingFrame{}, []frames.Frame{&frames.AckFrameLegacy{}}, 0, false)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(p).To(BeNil())
+	})
+
+	It("returns nil if we only have a single ACK", func() {
+		p, err := packer.PackPacket(nil, []frames.Frame{&frames.AckFrameLegacy{}}, 0, false)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(p).To(BeNil())
+	})
+
+	It("does not return nil if we only have a single ACK but request it to be sent", func() {
+		p, err := packer.PackPacket(nil, []frames.Frame{&frames.AckFrameLegacy{}}, 0, true)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(p).ToNot(BeNil())
+	})
 })

protocol/server_parameters.go

@@ -17,12 +17,8 @@ const InitialCongestionWindow PacketNumber = 32
 // session queues for later until it sends a public reset.
 const MaxUndecryptablePackets = 10
 
-// SmallPacketPayloadSizeThreshold defines a threshold for small packets
+// AckSendDelay is the maximal time delay applied to packets containing only ACKs
-// if the packet payload size (i.e. the packet without public header and private header) is below SmallPacketSizeThreshold, sending will be delayed by SmallPacketSendDelay
+const AckSendDelay = 5 * time.Millisecond
-const SmallPacketPayloadSizeThreshold = MaxPacketSize / 2
-
-// SmallPacketSendDelay is the time delay applied to small packets
-const SmallPacketSendDelay = 500 * time.Microsecond
 
 // ReceiveStreamFlowControlWindow is the stream-level flow control window for receiving data
 // This is the value that Google servers are using

session.go

@@ -76,7 +76,7 @@ type Session struct {
 	undecryptablePackets []receivedPacket
 	aeadChanged          chan struct{}
 
-	smallPacketDelayedOccurranceTime time.Time
+	delayedAckOriginTime time.Time
 
 	connectionParametersManager *handshake.ConnectionParametersManager
 
@@ -187,6 +187,9 @@ func (s *Session) run() {
 				s.tryQueueingUndecryptablePacket(p)
 				continue
 			}
+			if s.delayedAckOriginTime.IsZero() {
+				s.delayedAckOriginTime = time.Now()
+			}
 		case <-s.aeadChanged:
 			s.tryDecryptingQueuedPackets()
 		}
@@ -195,7 +198,7 @@ func (s *Session) run() {
 			s.Close(err)
 		}
 
-		if err := s.maybeSendPacket(); err != nil {
+		if err := s.sendPacket(); err != nil {
 			s.Close(err)
 		}
 		if time.Now().Sub(s.lastNetworkActivityTime) >= s.connectionParametersManager.GetIdleConnectionStateLifetime() {
@@ -208,9 +211,8 @@ func (s *Session) run() {
 func (s *Session) maybeResetTimer() {
 	nextDeadline := s.lastNetworkActivityTime.Add(s.connectionParametersManager.GetIdleConnectionStateLifetime())
 
-	if !s.smallPacketDelayedOccurranceTime.IsZero() {
+	if !s.delayedAckOriginTime.IsZero() {
-		// nextDeadline = utils.MinDuration(firstTimeout, s.smallPacketDelayedOccurranceTime.Add(protocol.SmallPacketSendDelay).Sub(now))
+		nextDeadline = utils.MinTime(nextDeadline, s.delayedAckOriginTime.Add(protocol.AckSendDelay))
-		nextDeadline = utils.MinTime(nextDeadline, s.smallPacketDelayedOccurranceTime.Add(protocol.SmallPacketSendDelay))
 	}
 	if rtoTime := s.sentPacketHandler.TimeOfFirstRTO(); !rtoTime.IsZero() {
 		nextDeadline = utils.MinTime(nextDeadline, rtoTime)
@@ -447,61 +449,7 @@ func (s *Session) closeStreamWithError(str *stream, err error) {
 	str.RegisterError(err)
 }
 
-func (s *Session) maybeSendPacket() error {
-	if !s.smallPacketDelayedOccurranceTime.IsZero() && time.Now().Sub(s.smallPacketDelayedOccurranceTime) > protocol.SmallPacketSendDelay {
-		return s.sendPacket()
-	}
-
-	// always send out retransmissions immediately. No need to check the size of the packet
-	// in the edge cases where a belated ACK was received for a packet that was already queued for retransmission, we might send out a small packet. However, this shouldn't happen very often
-	if s.sentPacketHandler.ProbablyHasPacketForRetransmission() {
-		return s.sendPacket()
-	}
-
-	if !s.sentPacketHandler.CongestionAllowsSending() {
-		return nil
-	}
-
-	var maxPacketSize protocol.ByteCount // the maximum size of a packet we could send out at this moment
-
-	// we only estimate the size of the StopWaitingFrame here
-	stopWaitingFrame := s.stopWaitingManager.GetStopWaitingFrame()
-	if stopWaitingFrame != nil {
-		// The actual size of a StopWaitingFrame depends on the packet number of the packet it is sent with, and it's easier here to neglect the fact the StopWaitingFrame could be 5 bytes smaller than calculated here
-		maxPacketSize += 8
-	}
-
-	ack, err := s.receivedPacketHandler.GetAckFrame(false)
-	if err != nil {
-		return err
-	}
-
-	if ack != nil {
-		ackLength, _ := ack.MinLength(s.version) // MinLength never errors for an ACK frame
-		maxPacketSize += ackLength
-	}
-
-	// note that maxPacketSize can get (much) larger than protocol.MaxPacketSize if there is a long queue of StreamFrames
-	maxPacketSize += s.streamFramer.EstimatedDataLen()
-
-	if maxPacketSize > protocol.SmallPacketPayloadSizeThreshold {
-		return s.sendPacket()
-	}
-
-	if maxPacketSize == 0 {
-		return nil
-	}
-
-	if s.smallPacketDelayedOccurranceTime.IsZero() {
-		s.smallPacketDelayedOccurranceTime = time.Now()
-	}
-
-	return nil
-}
-
 func (s *Session) sendPacket() error {
-	s.smallPacketDelayedOccurranceTime = time.Time{} // zero
-
 	// Repeatedly try sending until we don't have any more data, or run out of the congestion window
 	for {
 		err := s.sentPacketHandler.CheckForError()
@@ -539,7 +487,7 @@ func (s *Session) sendPacket() error {
 			controlFrames = append(controlFrames, wuf)
 		}
 
-		ack, err := s.receivedPacketHandler.GetAckFrame(true)
+		ack, err := s.receivedPacketHandler.GetAckFrame(false)
 		if err != nil {
 			return err
 		}
@@ -547,8 +495,11 @@ func (s *Session) sendPacket() error {
 			controlFrames = append(controlFrames, ack)
 		}
 
+		// Check whether we are allowed to send a packet containing only an ACK
+		maySendOnlyAck := time.Now().Sub(s.delayedAckOriginTime) > protocol.AckSendDelay
+
 		stopWaitingFrame := s.stopWaitingManager.GetStopWaitingFrame()
-		packet, err := s.packer.PackPacket(stopWaitingFrame, controlFrames, s.sentPacketHandler.GetLargestAcked())
+		packet, err := s.packer.PackPacket(stopWaitingFrame, controlFrames, s.sentPacketHandler.GetLargestAcked(), maySendOnlyAck)
 		if err != nil {
 			return err
 		}
@@ -556,6 +507,12 @@ func (s *Session) sendPacket() error {
 			return nil
 		}
 
+		// Pop the ACK frame now that we are sure we're gonna send it
+		_, err = s.receivedPacketHandler.GetAckFrame(true)
+		if err != nil {
+			return err
+		}
+
 		for _, f := range windowUpdateFrames {
 			s.packer.QueueControlFrameForNextPacket(f)
 		}
@@ -569,10 +526,9 @@ func (s *Session) sendPacket() error {
 		if err != nil {
 			return err
 		}
-
 		s.stopWaitingManager.SentStopWaitingWithPacket(packet.number)
-
 		s.logPacket(packet)
+		s.delayedAckOriginTime = time.Time{}
 
 		err = s.conn.write(packet.raw)
 		if err != nil {

session_test.go

@@ -557,10 +557,10 @@ var _ = Describe("Session", func() {
 				go session.run()
 				go func() {
 					defer GinkgoRecover()
-					_, err2 := s1.Write(bytes.Repeat([]byte{'e'}, int(protocol.SmallPacketPayloadSizeThreshold+50)))
+					_, err2 := s1.Write(bytes.Repeat([]byte{'e'}, 1000))
 					Expect(err2).ToNot(HaveOccurred())
 				}()
-				_, err = s2.Write(bytes.Repeat([]byte{'e'}, int(protocol.SmallPacketPayloadSizeThreshold+50)))
+				_, err = s2.Write(bytes.Repeat([]byte{'e'}, 1000))
 				Expect(err).ToNot(HaveOccurred())
 				Eventually(func() [][]byte { return conn.written }).Should(HaveLen(2))
 			})
@@ -685,7 +685,7 @@ var _ = Describe("Session", func() {
 			PacketNumber: n,
 			Length:       1,
 			Frames: []frames.Frame{&frames.StreamFrame{
-				Data: bytes.Repeat([]byte{'a'}, int(protocol.SmallPacketPayloadSizeThreshold)+1),
+				Data: bytes.Repeat([]byte{'a'}, 1000),
 			}},
 		})
 		session.packer.lastPacketNumber = n