diff --git a/packet_packer.go b/packet_packer.go index 5d9b4c35..c9870e91 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -179,7 +179,6 @@ func (p *packetPacker) composeNextPacket(stopWaitingFrame *frames.StopWaitingFra } hasStreamFrames := false - lastFrameIsBlockedFrame := false // temporarily increase the maxFrameSize by 2 bytes // this leads to a properly sized packet in all cases, since we do all the packet length calculations with StreamFrames that have the DataLen set @@ -211,31 +210,25 @@ func (p *packetPacker) composeNextPacket(stopWaitingFrame *frames.StopWaitingFra payloadLength += protocol.ByteCount(len(frame.Data)) - 1 } - payloadLength += frameMinLength - payloadFrames = append(payloadFrames, frame) - hasStreamFrames = true - lastFrameIsBlockedFrame = false - blockedFrame := p.blockedManager.GetBlockedFrame(frame.StreamID, frame.Offset+protocol.ByteCount(len(frame.Data))) if blockedFrame != nil { - blockedMinLength, _ := blockedFrame.MinLength() // BlockedFrame.MinLength *never* returns an error - if payloadLength+blockedMinLength <= maxFrameSize { + blockedLength, _ := blockedFrame.MinLength() // BlockedFrame.MinLength *never* returns an error + if payloadLength+frameMinLength+blockedLength <= maxFrameSize { payloadFrames = append(payloadFrames, blockedFrame) - payloadLength += blockedMinLength - lastFrameIsBlockedFrame = true + payloadLength += blockedLength } else { p.controlFrames = append(p.controlFrames, blockedFrame) } } + + payloadLength += frameMinLength + payloadFrames = append(payloadFrames, frame) + hasStreamFrames = true } // remove the dataLen for the last StreamFrame in the packet if hasStreamFrames { - lastStreamFrameIndex := len(payloadFrames) - 1 - if lastFrameIsBlockedFrame { - lastStreamFrameIndex-- - } - lastStreamFrame, ok := payloadFrames[lastStreamFrameIndex].(*frames.StreamFrame) + lastStreamFrame, ok := payloadFrames[len(payloadFrames)-1].(*frames.StreamFrame) if !ok { return nil, errors.New("PacketPacker BUG: StreamFrame type assertion failed") } diff --git a/packet_packer_test.go b/packet_packer_test.go index 0de39e5b..21d98bec 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -352,10 +352,10 @@ var _ = Describe("Packet packer", func() { p, err := packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) Expect(p).To(HaveLen(2)) - Expect(p[1]).To(Equal(&frames.BlockedFrame{StreamID: 5})) + Expect(p[0]).To(Equal(&frames.BlockedFrame{StreamID: 5})) }) - It("removes the dataLen attribute from the last StreamFrame, even if the last frame is a BlockedFrame", func() { + It("removes the dataLen attribute from the last StreamFrame, even if it inserted a BlockedFrame before", func() { length := 100 packer.AddBlocked(5, protocol.ByteCount(length)) f := frames.StreamFrame{ @@ -365,27 +365,8 @@ var _ = Describe("Packet packer", func() { packer.AddStreamFrame(f) p, err := packer.composeNextPacket(nil, publicHeaderLen, true) Expect(err).ToNot(HaveOccurred()) - Expect(p[0].(*frames.StreamFrame).DataLenPresent).To(BeFalse()) - }) - - It("correctly removes the dataLen attribute from the last StreamFrame, when packing one StreamFrame, one BlockedFrame, and another StreamFrame", func() { - length := 10 - packer.AddBlocked(5, protocol.ByteCount(length)) - f := frames.StreamFrame{ - StreamID: 5, - Data: bytes.Repeat([]byte{'f'}, length), - } - packer.AddStreamFrame(f) - f = frames.StreamFrame{ - StreamID: 7, - Data: []byte("foobar"), - } - packer.AddStreamFrame(f) - p, err := packer.composeNextPacket(nil, publicHeaderLen, true) - Expect(err).ToNot(HaveOccurred()) - Expect(p).To(HaveLen(3)) - Expect(p[0].(*frames.StreamFrame).DataLenPresent).To(BeTrue()) - Expect(p[2].(*frames.StreamFrame).DataLenPresent).To(BeFalse()) + Expect(p).To(HaveLen(2)) + Expect(p[1].(*frames.StreamFrame).DataLenPresent).To(BeFalse()) }) It("packs a BlockedFrame in the next packet if the current packet doesn't have enough space", func() { @@ -405,6 +386,29 @@ var _ = Describe("Packet packer", func() { Expect(p[0]).To(Equal(&frames.BlockedFrame{StreamID: 5})) }) + It("packs a packet with the maximum size with a BlocedFrame", func() { + blockedFrame := &frames.BlockedFrame{StreamID: 0x1337} + blockedFrameLen, _ := blockedFrame.MinLength() + f1 := frames.StreamFrame{ + StreamID: 5, + Offset: 1, + } + streamFrameHeaderLen, _ := f1.MinLength() + streamFrameHeaderLen-- // - 1 since MinceLength is 1 bigger than the actual StreamFrame header + // this is the maximum dataLen of a StreamFrames that fits into one packet + dataLen := int(protocol.MaxFrameAndPublicHeaderSize - publicHeaderLen - streamFrameHeaderLen - blockedFrameLen) + packer.AddBlocked(5, protocol.ByteCount(dataLen)) + f1.Data = bytes.Repeat([]byte{'f'}, dataLen) + packer.AddStreamFrame(f1) + p, err := packer.PackPacket(nil, []frames.Frame{}, true) + Expect(err).ToNot(HaveOccurred()) + Expect(p).ToNot(BeNil()) + Expect(p.raw).To(HaveLen(int(protocol.MaxPacketSize))) + p, err = packer.PackPacket(nil, []frames.Frame{}, true) + Expect(err).ToNot(HaveOccurred()) + Expect(p).To(BeNil()) + }) + // TODO: fix this once connection-level BlockedFrames are sent out at the right time // see https://github.com/lucas-clemente/quic-go/issues/113 It("packs a connection-level BlockedFrame", func() {