diff --git a/packet_packer.go b/packet_packer.go index 5843e9c1..6ba4bc72 100644 --- a/packet_packer.go +++ b/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) { - return p.packPacket(stopWaitingFrame, controlFrames, largestObserved, false) +func (p *packetPacker) PackPacket(stopWaitingFrame *frames.StopWaitingFrame, controlFrames []frames.Frame, largestObserved protocol.PacketNumber, maySendOnlyAck bool) (*packedPacket, error) { + 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,9 +87,26 @@ 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) { - return nil, nil + } + + // 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 + } } } diff --git a/packet_packer_test.go b/packet_packer_test.go index 5c4cd99e..e6579637 100644 --- a/packet_packer_test.go +++ b/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()) + }) }) diff --git a/protocol/server_parameters.go b/protocol/server_parameters.go index 1e87d6cc..20a78414 100644 --- a/protocol/server_parameters.go +++ b/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 -// 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 SmallPacketPayloadSizeThreshold = MaxPacketSize / 2 - -// SmallPacketSendDelay is the time delay applied to small packets -const SmallPacketSendDelay = 500 * time.Microsecond +// AckSendDelay is the maximal time delay applied to packets containing only ACKs +const AckSendDelay = 5 * time.Millisecond // ReceiveStreamFlowControlWindow is the stream-level flow control window for receiving data // This is the value that Google servers are using diff --git a/session.go b/session.go index 55720f8b..434dc6d4 100644 --- a/session.go +++ b/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() { - // nextDeadline = utils.MinDuration(firstTimeout, s.smallPacketDelayedOccurranceTime.Add(protocol.SmallPacketSendDelay).Sub(now)) - nextDeadline = utils.MinTime(nextDeadline, s.smallPacketDelayedOccurranceTime.Add(protocol.SmallPacketSendDelay)) + if !s.delayedAckOriginTime.IsZero() { + nextDeadline = utils.MinTime(nextDeadline, s.delayedAckOriginTime.Add(protocol.AckSendDelay)) } 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 { diff --git a/session_test.go b/session_test.go index 4e109205..6e11bc8d 100644 --- a/session_test.go +++ b/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