place BlockedFrames before the corresponding StreamFrame in a packet

fixes #114

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
+			blockedLength, _ := blockedFrame.MinLength() // BlockedFrame.MinLength *never* returns an error
-			if payloadLength+blockedMinLength <= maxFrameSize {
+			if payloadLength+frameMinLength+blockedLength <= maxFrameSize {
 				payloadFrames = append(payloadFrames, blockedFrame)
-				payloadLength += blockedMinLength
+				payloadLength += blockedLength
-				lastFrameIsBlockedFrame = true
 			} 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
+		lastStreamFrame, ok := payloadFrames[len(payloadFrames)-1].(*frames.StreamFrame)
-		if lastFrameIsBlockedFrame {
-			lastStreamFrameIndex--
-		}
-		lastStreamFrame, ok := payloadFrames[lastStreamFrameIndex].(*frames.StreamFrame)
 		if !ok {
 			return nil, errors.New("PacketPacker BUG: StreamFrame type assertion failed")
 		}

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())
+			Expect(p).To(HaveLen(2))
-		})
+			Expect(p[1].(*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())
 		})
 
 		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() {