only increase packet number when sending a packet in PacketPacker

fixes #288

packet_number_generator.go

@@ -7,30 +7,39 @@ import (
 	"github.com/lucas-clemente/quic-go/protocol"
 )
 
+// The packetNumberGenerator generates the packet number for the next packet
+// it randomly skips a packet number every averagePeriod packets (on average)
+// it is guarantued to never skip two consecutive packet numbers
 type packetNumberGenerator struct {
-	last          protocol.PacketNumber
-	nextToSkip    protocol.PacketNumber
 	averagePeriod protocol.PacketNumber
+
+	next       protocol.PacketNumber
+	nextToSkip protocol.PacketNumber
 }
 
 func newPacketNumberGenerator(averagePeriod protocol.PacketNumber) *packetNumberGenerator {
 	return &packetNumberGenerator{
+		next:          1,
 		averagePeriod: averagePeriod,
 	}
 }
 
-func (p *packetNumberGenerator) GetNextPacketNumber() protocol.PacketNumber {
-	p.last++
+func (p *packetNumberGenerator) Peek() protocol.PacketNumber {
+	return p.next
+}
 
-	if p.last == p.nextToSkip {
-		return p.GetNextPacketNumber()
-	}
+func (p *packetNumberGenerator) Pop() protocol.PacketNumber {
+	next := p.next
 
-	if p.last > p.nextToSkip {
+	// generate a new packet number for the next packet
+	p.next++
+
+	if p.next == p.nextToSkip {
+		p.next++
 		p.generateNewSkip()
 	}
 
-	return p.last
+	return next
 }
 
 func (p *packetNumberGenerator) generateNewSkip() error {
@@ -41,7 +50,7 @@ func (p *packetNumberGenerator) generateNewSkip() error {
 
 	skip := protocol.PacketNumber(num) * (p.averagePeriod - 1) / (math.MaxUint16 / 2)
 	// make sure that there are never two consecutive packet numbers that are skipped
-	p.nextToSkip = p.last + 2 + skip
+	p.nextToSkip = p.next + 2 + skip
 
 	return nil
 }

packet_number_generator_test.go

@@ -12,27 +12,38 @@ var _ = Describe("Packet Number Generator", func() {
 	var png packetNumberGenerator
 
 	BeforeEach(func() {
-		png = packetNumberGenerator{}
+		png = *newPacketNumberGenerator(100)
 	})
 
 	It("gets 1 as the first packet number", func() {
-		num := png.GetNextPacketNumber()
+		num := png.Pop()
 		Expect(num).To(Equal(protocol.PacketNumber(1)))
 	})
 
+	It("allows peeking", func() {
+		png.nextToSkip = 1000
+		Expect(png.Peek()).To(Equal(protocol.PacketNumber(1)))
+		Expect(png.Peek()).To(Equal(protocol.PacketNumber(1)))
+		num := png.Pop()
+		Expect(num).To(Equal(protocol.PacketNumber(1)))
+		Expect(png.Peek()).To(Equal(protocol.PacketNumber(2)))
+		Expect(png.Peek()).To(Equal(protocol.PacketNumber(2)))
+	})
+
 	It("skips a packet number", func() {
 		png.nextToSkip = 2
-		num := png.GetNextPacketNumber()
+		num := png.Pop()
 		Expect(num).To(Equal(protocol.PacketNumber(1)))
-		num = png.GetNextPacketNumber()
+		Expect(png.Peek()).To(Equal(protocol.PacketNumber(3)))
+		num = png.Pop()
 		Expect(num).To(Equal(protocol.PacketNumber(3)))
 	})
 
 	It("generates a new packet number to skip", func() {
-		png.last = 100
+		png.next = 100
 		png.averagePeriod = 100
 
-		rep := 1000
+		rep := 5000
 		var sum protocol.PacketNumber
 
 		for i := 0; i < rep; i++ {
@@ -42,7 +53,7 @@ var _ = Describe("Packet Number Generator", func() {
 		}
 
 		average := sum / protocol.PacketNumber(rep)
-		Expect(average).To(BeNumerically("==", protocol.PacketNumber(200), 10))
+		Expect(average).To(BeNumerically("==", protocol.PacketNumber(200), 4))
 	})
 
 	It("uses random numbers", func() {

packet_packer.go

@@ -65,7 +65,7 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, con
 	if p.version <= protocol.Version33 {
 		currentPacketNumber = p.lastPacketNumber + 1
 	} else {
-		currentPacketNumber = p.packetNumberGenerator.GetNextPacketNumber()
+		currentPacketNumber = p.packetNumberGenerator.Peek()
 	}
 
 	// cryptoSetup needs to be locked here, so that the AEADs are not changed between
@@ -158,7 +158,15 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, con
 	p.cryptoSetup.Seal(raw[payloadStartIndex:payloadStartIndex], raw[payloadStartIndex:], currentPacketNumber, raw[:payloadStartIndex])
 	raw = raw[0 : buffer.Len()+12]
 
-	p.lastPacketNumber++
+	if p.version <= protocol.Version33 {
+		p.lastPacketNumber++
+	} else {
+		num := p.packetNumberGenerator.Pop()
+		if num != currentPacketNumber {
+			return nil, errors.New("PacketPacker BUG: Peeked and Popped packet numbers do not match.")
+		}
+	}
+
 	return &packedPacket{
 		number:     currentPacketNumber,
 		entropyBit: entropyBit,

packet_packer_test.go

@@ -193,7 +193,9 @@ var _ = Describe("Packet packer", func() {
 		Expect(payloadFrames).To(HaveLen(10))
 	})
 
-	It("only increases the packet number when there is an actual packet to send", func() {
+	// TODO: remove once we drop support for QUIC 33
+	It("only increases the packet number when there is an actual packet to send, in QUIC 33", func() {
+		packer.version = protocol.Version33
 		f := &frames.StreamFrame{
 			StreamID: 5,
 			Data:     []byte{0xDE, 0xCA, 0xFB, 0xAD},
@@ -214,6 +216,24 @@ var _ = Describe("Packet packer", func() {
 		Expect(packer.lastPacketNumber).To(Equal(protocol.PacketNumber(2)))
 	})
 
+	It("only increases the packet number when there is an actual packet to send, in QUIC 34", func() {
+		packer.packetNumberGenerator.nextToSkip = 1000
+		p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true)
+		Expect(p).To(BeNil())
+		Expect(err).ToNot(HaveOccurred())
+		Expect(packer.packetNumberGenerator.Peek()).To(Equal(protocol.PacketNumber(1)))
+		f := &frames.StreamFrame{
+			StreamID: 5,
+			Data:     []byte{0xDE, 0xCA, 0xFB, 0xAD},
+		}
+		streamFramer.AddFrameForRetransmission(f)
+		p, err = packer.PackPacket(nil, []frames.Frame{}, 0, true)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(p).ToNot(BeNil())
+		Expect(p.number).To(Equal(protocol.PacketNumber(1)))
+		Expect(packer.packetNumberGenerator.Peek()).To(Equal(protocol.PacketNumber(2)))
+	})
+
 	Context("Stream Frame handling", func() {
 		It("does not splits a stream frame with maximum size", func() {
 			f := &frames.StreamFrame{

session_test.go

@@ -757,7 +757,7 @@ var _ = Describe("Session", func() {
 				if version == protocol.Version33 {
 					session.packer.lastPacketNumber = n
 				} else {
-					session.packer.packetNumberGenerator.last = n
+					session.packer.packetNumberGenerator.next = n + 1
 				}
 				// Now, we send a single packet, and expect that it was retransmitted later
 				err := session.sentPacketHandler.SentPacket(&ackhandlerlegacy.Packet{