From 561acc08e45bcd5efb23eb31a73162ed224be1de Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 14 May 2016 18:07:24 +0700 Subject: [PATCH] pack control frames into separate packets if they don't fit into one --- packet_packer.go | 31 ++++++++++++++++++------------- packet_packer_test.go | 39 ++++++++++++++++++++++++++++++--------- 2 files changed, 48 insertions(+), 22 deletions(-) diff --git a/packet_packer.go b/packet_packer.go index 8bacbeaa..3d0d9485 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -28,6 +28,7 @@ type packetPacker struct { connectionParametersManager *handshake.ConnectionParametersManager streamFrameQueue StreamFrameQueue + controlFrames []frames.Frame lastPacketNumber protocol.PacketNumber } @@ -41,13 +42,15 @@ func (p *packetPacker) AddHighPrioStreamFrame(f frames.StreamFrame) { } func (p *packetPacker) PackPacket(stopWaitingFrame *frames.StopWaitingFrame, controlFrames []frames.Frame, includeStreamFrames bool) (*packedPacket, error) { - // TODO: save controlFrames as a member variable, makes it easier to handle in the unlikely event that there are more controlFrames than you can put into on packet - // don't send out packets that only contain a StopWaitingFrame if len(controlFrames) == 0 && (p.streamFrameQueue.Len() == 0 || !includeStreamFrames) { return nil, nil } + if len(controlFrames) > 0 { + p.controlFrames = append(p.controlFrames, controlFrames...) + } + currentPacketNumber := protocol.PacketNumber(atomic.AddUint64( (*uint64)(&p.lastPacketNumber), 1, @@ -71,7 +74,7 @@ func (p *packetPacker) PackPacket(stopWaitingFrame *frames.StopWaitingFrame, con stopWaitingFrame.PacketNumberLen = packetNumberLen } - payloadFrames, err := p.composeNextPacket(stopWaitingFrame, controlFrames, publicHeaderLength, includeStreamFrames) + payloadFrames, err := p.composeNextPacket(stopWaitingFrame, publicHeaderLength, includeStreamFrames) if err != nil { return nil, err } @@ -118,11 +121,10 @@ func (p *packetPacker) getPayload(frames []frames.Frame, currentPacketNumber pro return payload.Bytes(), nil } -func (p *packetPacker) composeNextPacket(stopWaitingFrame *frames.StopWaitingFrame, controlFrames []frames.Frame, publicHeaderLength protocol.ByteCount, includeStreamFrames bool) ([]frames.Frame, error) { +func (p *packetPacker) composeNextPacket(stopWaitingFrame *frames.StopWaitingFrame, publicHeaderLength protocol.ByteCount, includeStreamFrames bool) ([]frames.Frame, error) { payloadLength := protocol.ByteCount(0) var payloadFrames []frames.Frame - // TODO: handle the case where there are more controlFrames than we can put into one packet if stopWaitingFrame != nil { payloadFrames = append(payloadFrames, stopWaitingFrame) minLength, err := stopWaitingFrame.MinLength() @@ -132,16 +134,19 @@ func (p *packetPacker) composeNextPacket(stopWaitingFrame *frames.StopWaitingFra payloadLength += minLength } - for len(controlFrames) > 0 { - frame := controlFrames[0] - payloadFrames = append(payloadFrames, frame) - minLength, _ := frame.MinLength() // controlFrames does not contain any StopWaitingFrames. So it will *never* return an error - payloadLength += minLength - controlFrames = controlFrames[1:] - } - maxFrameSize := protocol.MaxFrameAndPublicHeaderSize - publicHeaderLength + for len(p.controlFrames) > 0 { + frame := p.controlFrames[0] + minLength, _ := frame.MinLength() // controlFrames does not contain any StopWaitingFrames. So it will *never* return an error + if payloadLength+minLength > maxFrameSize { + break + } + payloadFrames = append(payloadFrames, frame) + payloadLength += minLength + p.controlFrames = p.controlFrames[1:] + } + if payloadLength > maxFrameSize { panic("internal inconsistency: packet payload too large") } diff --git a/packet_packer_test.go b/packet_packer_test.go index 29538d4b..dc1a341e 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -137,14 +137,35 @@ var _ = Describe("Packet packer", func() { for i := 0; i < maxFramesPerPacket; i++ { controlFrames = append(controlFrames, f) } - payloadFrames, err := packer.composeNextPacket(nil, controlFrames, publicHeaderLen, true) + packer.controlFrames = controlFrames + payloadFrames, err := packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(Equal(maxFramesPerPacket)) - payloadFrames, err = packer.composeNextPacket(nil, []frames.Frame{}, publicHeaderLen, true) + payloadFrames, err = packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(BeZero()) }) + It("packs a lot of control frames into 2 packets if they don't fit into one", func() { + windowUpdateFrame := &frames.WindowUpdateFrame{ + StreamID: 0x1337, + ByteOffset: 0xDECAFBAD, + } + minLength, _ := windowUpdateFrame.MinLength() + maxFramesPerPacket := int(protocol.MaxFrameAndPublicHeaderSize-publicHeaderLen) / int(minLength) + var controlFrames []frames.Frame + for i := 0; i < maxFramesPerPacket+10; i++ { + controlFrames = append(controlFrames, windowUpdateFrame) + } + packer.controlFrames = controlFrames + payloadFrames, err := packer.composeNextPacket(nil, publicHeaderLen, true) + Expect(err).ToNot(HaveOccurred()) + Expect(len(payloadFrames)).To(Equal(maxFramesPerPacket)) + payloadFrames, err = packer.composeNextPacket(nil, publicHeaderLen, true) + Expect(err).ToNot(HaveOccurred()) + Expect(len(payloadFrames)).To(Equal(10)) + }) + It("only increases the packet number when there is an actual packet to send", func() { f := frames.StreamFrame{ StreamID: 5, @@ -176,11 +197,11 @@ var _ = Describe("Packet packer", func() { maxStreamFrameDataLen := protocol.MaxFrameAndPublicHeaderSize - publicHeaderLen - minLength f.Data = bytes.Repeat([]byte{'f'}, int(maxStreamFrameDataLen)) packer.AddStreamFrame(f) - payloadFrames, err := packer.composeNextPacket(nil, []frames.Frame{}, publicHeaderLen, true) + payloadFrames, err := packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(Equal(1)) Expect(payloadFrames[0].(*frames.StreamFrame).DataLenPresent).To(BeFalse()) - payloadFrames, err = packer.composeNextPacket(nil, []frames.Frame{}, publicHeaderLen, true) + payloadFrames, err = packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(Equal(0)) }) @@ -248,17 +269,17 @@ var _ = Describe("Packet packer", func() { maxStreamFrameDataLen := protocol.MaxFrameAndPublicHeaderSize - publicHeaderLen - minLength + 1 // + 1 since MinceLength is 1 bigger than the actual StreamFrame header f.Data = bytes.Repeat([]byte{'f'}, int(maxStreamFrameDataLen)+200) packer.AddStreamFrame(f) - payloadFrames, err := packer.composeNextPacket(nil, []frames.Frame{}, publicHeaderLen, true) + payloadFrames, err := packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(Equal(1)) Expect(payloadFrames[0].(*frames.StreamFrame).DataLenPresent).To(BeFalse()) Expect(protocol.ByteCount(len(payloadFrames[0].(*frames.StreamFrame).Data))).To(Equal(maxStreamFrameDataLen)) - payloadFrames, err = packer.composeNextPacket(nil, []frames.Frame{}, publicHeaderLen, true) + payloadFrames, err = packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(Equal(1)) Expect(len(payloadFrames[0].(*frames.StreamFrame).Data)).To(Equal(200)) Expect(payloadFrames[0].(*frames.StreamFrame).DataLenPresent).To(BeFalse()) - payloadFrames, err = packer.composeNextPacket(nil, []frames.Frame{}, publicHeaderLen, true) + payloadFrames, err = packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(Equal(0)) }) @@ -318,10 +339,10 @@ var _ = Describe("Packet packer", func() { f.Data = bytes.Repeat([]byte{'f'}, int(protocol.MaxFrameAndPublicHeaderSize-publicHeaderLen-minLength+2)) // + 2 since MinceLength is 1 bigger than the actual StreamFrame header packer.AddStreamFrame(f) - payloadFrames, err := packer.composeNextPacket(nil, []frames.Frame{}, publicHeaderLen, true) + payloadFrames, err := packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(Equal(1)) - payloadFrames, err = packer.composeNextPacket(nil, []frames.Frame{}, publicHeaderLen, true) + payloadFrames, err = packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(len(payloadFrames)).To(Equal(1)) })