From dba964f7355f859e9dbb327b68f970a49701feaa Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 27 Apr 2019 00:06:13 +0900 Subject: [PATCH] fix packing of maximum-size packets --- framer.go | 15 +++++++++-- framer_test.go | 63 ++++++++++++++++++++++++++++++++++++++++--- packet_packer.go | 10 ------- packet_packer_test.go | 10 +++---- 4 files changed, 75 insertions(+), 23 deletions(-) diff --git a/framer.go b/framer.go index d5be6fc9..2f4d131b 100644 --- a/framer.go +++ b/framer.go @@ -4,6 +4,7 @@ import ( "sync" "github.com/lucas-clemente/quic-go/internal/protocol" + "github.com/lucas-clemente/quic-go/internal/utils" "github.com/lucas-clemente/quic-go/internal/wire" ) @@ -75,11 +76,12 @@ func (f *framerI) AddActiveStream(id protocol.StreamID) { func (f *framerI) AppendStreamFrames(frames []wire.Frame, maxLen protocol.ByteCount) ([]wire.Frame, protocol.ByteCount) { var length protocol.ByteCount + var frameAdded bool f.mutex.Lock() // pop STREAM frames, until less than MinStreamFrameSize bytes are left in the packet numActiveStreams := len(f.streamQueue) for i := 0; i < numActiveStreams; i++ { - if maxLen-length < protocol.MinStreamFrameSize { + if protocol.MinStreamFrameSize+length > maxLen { break } id := f.streamQueue[0] @@ -92,7 +94,12 @@ func (f *framerI) AppendStreamFrames(frames []wire.Frame, maxLen protocol.ByteCo delete(f.activeStreams, id) continue } - frame, hasMoreData := str.popStreamFrame(maxLen - length) + remainingLen := maxLen - length + // For the last STREAM frame, we'll remove the DataLen field later. + // Therefore, we can pretend to have more bytes avaibalbe when popping + // the STREAM frame (which will always have the DataLen set). + remainingLen += utils.VarIntLen(uint64(remainingLen)) + frame, hasMoreData := str.popStreamFrame(remainingLen) if hasMoreData { // put the stream back in the queue (at the end) f.streamQueue = append(f.streamQueue, id) } else { // no more data to send. Stream is not active any more @@ -103,7 +110,11 @@ func (f *framerI) AppendStreamFrames(frames []wire.Frame, maxLen protocol.ByteCo } frames = append(frames, frame) length += frame.Length(f.version) + frameAdded = true } f.mutex.Unlock() + if frameAdded { + frames[len(frames)-1].(*wire.StreamFrame).DataLenPresent = false + } return frames, length } diff --git a/framer_test.go b/framer_test.go index 075bb6d2..536bc64b 100644 --- a/framer_test.go +++ b/framer_test.go @@ -7,11 +7,12 @@ import ( "github.com/lucas-clemente/quic-go/internal/protocol" "github.com/lucas-clemente/quic-go/internal/wire" + . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) -var _ = Describe("Stream Framer", func() { +var _ = Describe("Framer", func() { const ( id1 = protocol.StreamID(10) id2 = protocol.StreamID(11) @@ -216,9 +217,63 @@ var _ = Describe("Stream Framer", func() { Expect(length).To(BeZero()) }) - It("pops frames that have the minimum size", func() { + It("pops maximum size STREAM frames", func() { + for i := protocol.MinStreamFrameSize; i < 2000; i++ { + streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil) + stream1.EXPECT().popStreamFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) (*wire.StreamFrame, bool) { + f := &wire.StreamFrame{ + StreamID: id1, + DataLenPresent: true, + } + f.Data = make([]byte, f.MaxDataLen(size, version)) + Expect(f.Length(version)).To(Equal(size)) + return f, false + }) + framer.AddActiveStream(id1) + frames, _ := framer.AppendStreamFrames(nil, i) + Expect(frames).To(HaveLen(1)) + f := frames[0].(*wire.StreamFrame) + Expect(f.DataLenPresent).To(BeFalse()) + Expect(f.Length(version)).To(Equal(i)) + } + }) + + It("pops multiple STREAM frames", func() { + for i := 2 * protocol.MinStreamFrameSize; i < 2000; i++ { + streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil) + streamGetter.EXPECT().GetOrOpenSendStream(id2).Return(stream2, nil) + stream1.EXPECT().popStreamFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) (*wire.StreamFrame, bool) { + f := &wire.StreamFrame{ + StreamID: id2, + DataLenPresent: true, + } + f.Data = make([]byte, f.MaxDataLen(protocol.MinStreamFrameSize, version)) + return f, false + }) + stream2.EXPECT().popStreamFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) (*wire.StreamFrame, bool) { + f := &wire.StreamFrame{ + StreamID: id2, + DataLenPresent: true, + } + f.Data = make([]byte, f.MaxDataLen(size, version)) + Expect(f.Length(version)).To(Equal(size)) + return f, false + }) + framer.AddActiveStream(id1) + framer.AddActiveStream(id2) + frames, _ := framer.AppendStreamFrames(nil, i) + Expect(frames).To(HaveLen(2)) + f1 := frames[0].(*wire.StreamFrame) + f2 := frames[1].(*wire.StreamFrame) + Expect(f1.DataLenPresent).To(BeTrue()) + Expect(f2.DataLenPresent).To(BeFalse()) + Expect(f1.Length(version) + f2.Length(version)).To(Equal(i)) + } + }) + + It("pops frames that when asked for the the minimum STREAM frame size", func() { streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil) - stream1.EXPECT().popStreamFrame(protocol.MinStreamFrameSize).Return(&wire.StreamFrame{Data: []byte("foobar")}, false) + stream1.EXPECT().popStreamFrame(gomock.Any()).Return(&wire.StreamFrame{Data: []byte("foobar")}, false) framer.AddActiveStream(id1) framer.AppendStreamFrames(nil, protocol.MinStreamFrameSize) }) @@ -235,7 +290,7 @@ var _ = Describe("Stream Framer", func() { StreamID: id1, Data: bytes.Repeat([]byte("f"), int(500-protocol.MinStreamFrameSize)), } - stream1.EXPECT().popStreamFrame(protocol.ByteCount(500)).Return(f, false) + stream1.EXPECT().popStreamFrame(gomock.Any()).Return(f, false) framer.AddActiveStream(id1) fs, length := framer.AppendStreamFrames(nil, 500) Expect(fs).To(Equal([]wire.Frame{f})) diff --git a/packet_packer.go b/packet_packer.go index b52a29c4..79ea80cb 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -353,18 +353,8 @@ func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount) (paylo frames, lengthAdded := p.framer.AppendControlFrames(payload.frames, maxFrameSize-payload.length) payload.length += lengthAdded - // temporarily increase the maxFrameSize by the (minimum) length of the DataLen field - // this leads to a properly sized packet in all cases, since we do all the packet length calculations with STREAM frames that have the DataLen set - // however, for the last STREAM frame in the packet, we can omit the DataLen, thus yielding a packet of exactly the correct size - // the length is encoded to either 1 or 2 bytes - maxFrameSize++ - frames, lengthAdded = p.framer.AppendStreamFrames(frames, maxFrameSize-payload.length) if len(frames) > 0 { - lastFrame := frames[len(frames)-1] - if sf, ok := lastFrame.(*wire.StreamFrame); ok { - sf.DataLenPresent = false - } payload.frames = append(payload.frames, frames...) payload.length += lengthAdded } diff --git a/packet_packer_test.go b/packet_packer_test.go index b5a9f615..5f4c355c 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -316,7 +316,7 @@ var _ = Describe("Packet packer", func() { return fs, 444 }), framer.EXPECT().AppendStreamFrames(gomock.Any(), gomock.Any()).Do(func(fs []wire.Frame, maxLen protocol.ByteCount) ([]wire.Frame, protocol.ByteCount) { - Expect(maxLen).To(Equal(maxSize - 444 + 1 /* data length of the STREAM frame */)) + Expect(maxLen).To(Equal(maxSize - 444)) return fs, 0 }), ) @@ -435,9 +435,8 @@ var _ = Describe("Packet packer", func() { sealingManager.EXPECT().GetSealer().Return(protocol.Encryption1RTT, sealer) expectAppendControlFrames() sf := &wire.StreamFrame{ - Offset: 1, - StreamID: 5, - DataLenPresent: true, + Offset: 1, + StreamID: 5, } framer.EXPECT().AppendStreamFrames(gomock.Any(), gomock.Any()).DoAndReturn(func(_ []wire.Frame, maxSize protocol.ByteCount) ([]wire.Frame, protocol.ByteCount) { sf.Data = bytes.Repeat([]byte{'f'}, int(maxSize-sf.Length(packer.version))) @@ -478,11 +477,8 @@ var _ = Describe("Packet packer", func() { Expect(err).ToNot(HaveOccurred()) Expect(p.frames).To(HaveLen(3)) Expect(p.frames[0].(*wire.StreamFrame).Data).To(Equal([]byte("frame 1"))) - Expect(p.frames[0].(*wire.StreamFrame).DataLenPresent).To(BeTrue()) Expect(p.frames[1].(*wire.StreamFrame).Data).To(Equal([]byte("frame 2"))) - Expect(p.frames[1].(*wire.StreamFrame).DataLenPresent).To(BeTrue()) Expect(p.frames[2].(*wire.StreamFrame).Data).To(Equal([]byte("frame 3"))) - Expect(p.frames[2].(*wire.StreamFrame).DataLenPresent).To(BeFalse()) }) })