From 3683763dc05748421c81746fef1b2cb8fb1d5b18 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 12 May 2016 17:43:48 +0700 Subject: [PATCH] send DelayTime in ACK frames fixes #81 --- ackhandler/interfaces.go | 2 +- ackhandler/received_packet_handler.go | 26 +++++++++++++++------- ackhandler/received_packet_handler_test.go | 22 ++++++++++++------ frames/ack_frame.go | 6 ++++- frames/ack_frame_test.go | 18 ++++++++++++++- packet_unpacker_test.go | 11 +++++---- session.go | 5 ++++- session_test.go | 6 +++-- 8 files changed, 71 insertions(+), 25 deletions(-) diff --git a/ackhandler/interfaces.go b/ackhandler/interfaces.go index e161bee8..d6a922de 100644 --- a/ackhandler/interfaces.go +++ b/ackhandler/interfaces.go @@ -23,7 +23,7 @@ type ReceivedPacketHandler interface { ReceivedPacket(packetNumber protocol.PacketNumber, entropyBit bool) error ReceivedStopWaiting(*frames.StopWaitingFrame) error - DequeueAckFrame() *frames.AckFrame + DequeueAckFrame() (*frames.AckFrame, error) } // StopWaitingManager manages StopWaitings for sent packets diff --git a/ackhandler/received_packet_handler.go b/ackhandler/received_packet_handler.go index 2561d183..d0a411d4 100644 --- a/ackhandler/received_packet_handler.go +++ b/ackhandler/received_packet_handler.go @@ -11,6 +11,8 @@ import ( // ErrDuplicatePacket occurres when a duplicate packet is received var ErrDuplicatePacket = errors.New("ReceivedPacketHandler: Duplicate Packet") +var errInvalidPacketNumber = errors.New("ReceivedPacketHandler: Invalid packet number") + type packetHistoryEntry struct { EntropyBit bool TimeReceived time.Time @@ -33,7 +35,7 @@ func NewReceivedPacketHandler() ReceivedPacketHandler { func (h *receivedPacketHandler) ReceivedPacket(packetNumber protocol.PacketNumber, entropyBit bool) error { if packetNumber == 0 { - return errors.New("Invalid packet number") + return errInvalidPacketNumber } _, ok := h.packetHistory[packetNumber] if packetNumber <= h.highestInOrderObserved || ok { @@ -98,17 +100,25 @@ func (h *receivedPacketHandler) getNackRanges() ([]frames.NackRange, EntropyAccu return ranges, entropy } -func (h *receivedPacketHandler) DequeueAckFrame() *frames.AckFrame { +func (h *receivedPacketHandler) DequeueAckFrame() (*frames.AckFrame, error) { if !h.stateChanged { - return nil + return nil, nil } h.stateChanged = false - nackRanges, entropy := h.getNackRanges() - return &frames.AckFrame{ - LargestObserved: h.largestObserved, - Entropy: byte(entropy), - NackRanges: nackRanges, + p, ok := h.packetHistory[h.largestObserved] + if !ok { + return nil, errors.New("bla") } + packetReceivedTime := p.TimeReceived + + nackRanges, entropy := h.getNackRanges() + ack := frames.AckFrame{ + LargestObserved: h.largestObserved, + Entropy: byte(entropy), + NackRanges: nackRanges, + PacketReceivedTime: packetReceivedTime, + } + return &ack, nil } diff --git a/ackhandler/received_packet_handler_test.go b/ackhandler/received_packet_handler_test.go index 21172ad6..883daad2 100644 --- a/ackhandler/received_packet_handler_test.go +++ b/ackhandler/received_packet_handler_test.go @@ -34,6 +34,12 @@ var _ = Describe("receivedPacketHandler", func() { Expect(handler.packetHistory).To(HaveKey(protocol.PacketNumber(3))) }) + It("rejects packets with packet number 0", func() { + err := handler.ReceivedPacket(protocol.PacketNumber(0), false) + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(errInvalidPacketNumber)) + }) + It("rejects a duplicate package with PacketNumber equal to LargestObserved", func() { for i := 1; i < 5; i++ { err := handler.ReceivedPacket(protocol.PacketNumber(i), false) @@ -243,7 +249,10 @@ var _ = Describe("receivedPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) err = handler.ReceivedPacket(protocol.PacketNumber(2), true) Expect(err).ToNot(HaveOccurred()) - Expect(handler.DequeueAckFrame()).To(Equal(&frames.AckFrame{LargestObserved: 2, Entropy: byte(entropy)})) + ack, err := handler.DequeueAckFrame() + Expect(err).ToNot(HaveOccurred()) + Expect(ack.LargestObserved).To(Equal(protocol.PacketNumber(2))) + Expect(ack.Entropy).To(Equal(byte(entropy))) }) It("generates an ACK frame with a NACK range", func() { @@ -254,12 +263,11 @@ var _ = Describe("receivedPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) err = handler.ReceivedPacket(protocol.PacketNumber(4), true) Expect(err).ToNot(HaveOccurred()) - expectedAck := frames.AckFrame{ - LargestObserved: 4, - Entropy: byte(entropy), - NackRanges: []frames.NackRange{frames.NackRange{FirstPacketNumber: 2, LastPacketNumber: 3}}, - } - Expect(handler.DequeueAckFrame()).To(Equal(&expectedAck)) + ack, err := handler.DequeueAckFrame() + Expect(err).ToNot(HaveOccurred()) + Expect(ack.LargestObserved).To(Equal(protocol.PacketNumber(4))) + Expect(ack.Entropy).To(Equal(byte(entropy))) + Expect(ack.NackRanges).To(Equal([]frames.NackRange{frames.NackRange{FirstPacketNumber: 2, LastPacketNumber: 3}})) }) It("does not generate an ACK if an ACK has already been sent for the largest Packet", func() { diff --git a/frames/ack_frame.go b/frames/ack_frame.go index 28c442c5..f9c43fb4 100644 --- a/frames/ack_frame.go +++ b/frames/ack_frame.go @@ -15,9 +15,11 @@ var errInvalidNackRanges = errors.New("AckFrame: ACK frame contains invalid NACK type AckFrame struct { LargestObserved protocol.PacketNumber Entropy byte - DelayTime time.Duration NackRanges []NackRange // has to be ordered. The NACK range with the highest FirstPacketNumber goes first, the NACK range with the lowest FirstPacketNumber goes last Truncated bool + + DelayTime time.Duration + PacketReceivedTime time.Time // only for received packets. Will not be modified for received ACKs frames } // Write writes an ACK frame. @@ -28,6 +30,8 @@ func (f *AckFrame) Write(b *bytes.Buffer, version protocol.VersionNumber) error typeByte |= (0x20 | 0x03) } + f.DelayTime = time.Now().Sub(f.PacketReceivedTime) + b.WriteByte(typeByte) b.WriteByte(f.Entropy) utils.WriteUint48(b, uint64(f.LargestObserved)) // TODO: send the correct length diff --git a/frames/ack_frame_test.go b/frames/ack_frame_test.go index ba317e2b..2cef07ac 100644 --- a/frames/ack_frame_test.go +++ b/frames/ack_frame_test.go @@ -5,6 +5,7 @@ import ( "time" "github.com/lucas-clemente/quic-go/protocol" + "github.com/lucas-clemente/quic-go/utils" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -251,7 +252,22 @@ var _ = Describe("AckFrame", func() { } err := frame.Write(b, 32) Expect(err).ToNot(HaveOccurred()) - Expect(b.Bytes()).To(Equal([]byte{0x4c, 0x02, 0x01, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0})) + // check all values except the DelayTime + Expect(b.Bytes()[0:8]).To(Equal([]byte{0x4c, 0x02, 0x01, 0, 0, 0, 0, 0})) + Expect(b.Bytes()[10:]).To(Equal([]byte{1, 0, 0, 0, 0, 0})) + }) + + It("calculates the DelayTime", func() { + frame := AckFrame{ + LargestObserved: 5, + PacketReceivedTime: time.Now().Add(-750 * time.Millisecond), + } + frame.Write(b, 32) + Expect(frame.DelayTime).To(BeNumerically("~", 750*time.Millisecond, 10*time.Millisecond)) + delayTime := frame.DelayTime + var b2 bytes.Buffer + utils.WriteUfloat16(&b2, uint64(delayTime/time.Microsecond)) + Expect(b.Bytes()[8:10]).To(Equal(b2.Bytes())) }) It("writes a frame with one NACK range", func() { diff --git a/packet_unpacker_test.go b/packet_unpacker_test.go index b37cb8f6..ca639ca1 100644 --- a/packet_unpacker_test.go +++ b/packet_unpacker_test.go @@ -2,10 +2,10 @@ package quic import ( "bytes" - "time" "github.com/lucas-clemente/quic-go/crypto" "github.com/lucas-clemente/quic-go/frames" + "github.com/lucas-clemente/quic-go/protocol" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -60,15 +60,18 @@ var _ = Describe("Packet unpacker", func() { It("unpacks ack frames", func() { f := &frames.AckFrame{ - LargestObserved: 1, - DelayTime: time.Microsecond, + LargestObserved: 0x13, + Entropy: 0x37, } err := f.Write(buf, 32) Expect(err).ToNot(HaveOccurred()) setReader(buf.Bytes()) packet, err := unpacker.Unpack(hdrBin, hdr, r) Expect(err).ToNot(HaveOccurred()) - Expect(packet.frames).To(Equal([]frames.Frame{f})) + Expect(len(packet.frames)).To(Equal(1)) + readFrame := packet.frames[0].(*frames.AckFrame) + Expect(readFrame.LargestObserved).To(Equal(protocol.PacketNumber(0x13))) + Expect(readFrame.Entropy).To(Equal(byte(0x37))) }) It("errors on CONGESTION_FEEDBACK frames", func() { diff --git a/session.go b/session.go index 45bcece4..ed00266e 100644 --- a/session.go +++ b/session.go @@ -397,7 +397,10 @@ func (s *Session) sendPacket() error { stopWaitingFrame := s.stopWaitingManager.GetStopWaitingFrame() - ack := s.receivedPacketHandler.DequeueAckFrame() + ack, err := s.receivedPacketHandler.DequeueAckFrame() + if err != nil { + return err + } if ack != nil { controlFrames = append(controlFrames, ack) } diff --git a/session_test.go b/session_test.go index 6693eb87..73694ace 100644 --- a/session_test.go +++ b/session_test.go @@ -321,7 +321,8 @@ var _ = Describe("Session", func() { err := session.sendPacket() Expect(err).NotTo(HaveOccurred()) Expect(conn.written).To(HaveLen(1)) - Expect(conn.written[0]).To(ContainSubstring(string([]byte{0x4c, 0x2, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0}))) + // test for the beginning of an ACK frame: TypeByte until LargestObserved + Expect(conn.written[0]).To(ContainSubstring(string([]byte{0x4c, 0x2, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0}))) }) It("sends queued stream frames", func() { @@ -333,7 +334,8 @@ var _ = Describe("Session", func() { err := session.sendPacket() Expect(err).NotTo(HaveOccurred()) Expect(conn.written).To(HaveLen(1)) - Expect(conn.written[0]).To(ContainSubstring(string([]byte{0x4c, 0x2, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0}))) + // test for the beginning of an ACK frame: TypeByte until LargestObserved + Expect(conn.written[0]).To(ContainSubstring(string([]byte{0x4c, 0x2, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0}))) Expect(conn.written[0]).To(ContainSubstring(string("foobar"))) })