From 875692ea108ec6cf9e557ab077b711a3224c22fd Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 2 Apr 2021 11:24:54 +0700 Subject: [PATCH 1/2] add a function to trace acknowledged packets --- integrationtests/self/self_suite_test.go | 1 + integrationtests/self/tracer_test.go | 1 + internal/ackhandler/sent_packet_handler.go | 3 +++ internal/mocks/logging/connection_tracer.go | 12 ++++++++++++ logging/interface.go | 1 + logging/mock_connection_tracer_test.go | 12 ++++++++++++ logging/multiplex.go | 6 ++++++ logging/multiplex_test.go | 6 ++++++ qlog/qlog.go | 2 ++ 9 files changed, 44 insertions(+) diff --git a/integrationtests/self/self_suite_test.go b/integrationtests/self/self_suite_test.go index 81956cbfa..429bcffab 100644 --- a/integrationtests/self/self_suite_test.go +++ b/integrationtests/self/self_suite_test.go @@ -354,6 +354,7 @@ func (t *connTracer) DroppedPacket(logging.PacketType, logging.ByteCount, loggin func (t *connTracer) UpdatedMetrics(rttStats *logging.RTTStats, cwnd, bytesInFlight logging.ByteCount, packetsInFlight int) { } +func (t *connTracer) AcknowledgedPacket(logging.EncryptionLevel, logging.PacketNumber) {} func (t *connTracer) LostPacket(logging.EncryptionLevel, logging.PacketNumber, logging.PacketLossReason) { } func (t *connTracer) UpdatedCongestionState(logging.CongestionState) {} diff --git a/integrationtests/self/tracer_test.go b/integrationtests/self/tracer_test.go index 5ac2f6410..5bca78d5a 100644 --- a/integrationtests/self/tracer_test.go +++ b/integrationtests/self/tracer_test.go @@ -57,6 +57,7 @@ func (t *customConnTracer) DroppedPacket(logging.PacketType, logging.ByteCount, func (t *customConnTracer) UpdatedMetrics(rttStats *logging.RTTStats, cwnd, bytesInFlight logging.ByteCount, packetsInFlight int) { } +func (t *customConnTracer) AcknowledgedPacket(logging.EncryptionLevel, logging.PacketNumber) {} func (t *customConnTracer) LostPacket(logging.EncryptionLevel, logging.PacketNumber, logging.PacketLossReason) { } func (t *customConnTracer) UpdatedCongestionState(logging.CongestionState) {} diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 91ff05a3c..4e34478c7 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -399,6 +399,9 @@ func (h *sentPacketHandler) detectAndRemoveAckedPackets(ack *wire.AckFrame, encL if err := pnSpace.history.Remove(p.PacketNumber); err != nil { return nil, err } + if h.tracer != nil && !p.skippedPacket { + h.tracer.AcknowledgedPacket(encLevel, p.PacketNumber) + } } return h.ackedPackets, err diff --git a/internal/mocks/logging/connection_tracer.go b/internal/mocks/logging/connection_tracer.go index 64d774ca9..cbfc253b4 100644 --- a/internal/mocks/logging/connection_tracer.go +++ b/internal/mocks/logging/connection_tracer.go @@ -39,6 +39,18 @@ func (m *MockConnectionTracer) EXPECT() *MockConnectionTracerMockRecorder { return m.recorder } +// AcknowledgedPacket mocks base method. +func (m *MockConnectionTracer) AcknowledgedPacket(arg0 protocol.EncryptionLevel, arg1 protocol.PacketNumber) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "AcknowledgedPacket", arg0, arg1) +} + +// AcknowledgedPacket indicates an expected call of AcknowledgedPacket. +func (mr *MockConnectionTracerMockRecorder) AcknowledgedPacket(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AcknowledgedPacket", reflect.TypeOf((*MockConnectionTracer)(nil).AcknowledgedPacket), arg0, arg1) +} + // BufferedPacket mocks base method. func (m *MockConnectionTracer) BufferedPacket(arg0 protocol.PacketType) { m.ctrl.T.Helper() diff --git a/logging/interface.go b/logging/interface.go index cf52cc156..d03b66c80 100644 --- a/logging/interface.go +++ b/logging/interface.go @@ -115,6 +115,7 @@ type ConnectionTracer interface { BufferedPacket(PacketType) DroppedPacket(PacketType, ByteCount, PacketDropReason) UpdatedMetrics(rttStats *RTTStats, cwnd, bytesInFlight ByteCount, packetsInFlight int) + AcknowledgedPacket(EncryptionLevel, PacketNumber) LostPacket(EncryptionLevel, PacketNumber, PacketLossReason) UpdatedCongestionState(CongestionState) UpdatedPTOCount(value uint32) diff --git a/logging/mock_connection_tracer_test.go b/logging/mock_connection_tracer_test.go index 4ec76d6b6..27fb8bdf1 100644 --- a/logging/mock_connection_tracer_test.go +++ b/logging/mock_connection_tracer_test.go @@ -38,6 +38,18 @@ func (m *MockConnectionTracer) EXPECT() *MockConnectionTracerMockRecorder { return m.recorder } +// AcknowledgedPacket mocks base method. +func (m *MockConnectionTracer) AcknowledgedPacket(arg0 protocol.EncryptionLevel, arg1 protocol.PacketNumber) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "AcknowledgedPacket", arg0, arg1) +} + +// AcknowledgedPacket indicates an expected call of AcknowledgedPacket. +func (mr *MockConnectionTracerMockRecorder) AcknowledgedPacket(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AcknowledgedPacket", reflect.TypeOf((*MockConnectionTracer)(nil).AcknowledgedPacket), arg0, arg1) +} + // BufferedPacket mocks base method. func (m *MockConnectionTracer) BufferedPacket(arg0 protocol.PacketType) { m.ctrl.T.Helper() diff --git a/logging/multiplex.go b/logging/multiplex.go index f06eaef48..fd529ef63 100644 --- a/logging/multiplex.go +++ b/logging/multiplex.go @@ -139,6 +139,12 @@ func (m *connTracerMultiplexer) UpdatedMetrics(rttStats *RTTStats, cwnd, bytesIn } } +func (m *connTracerMultiplexer) AcknowledgedPacket(encLevel EncryptionLevel, pn PacketNumber) { + for _, t := range m.tracers { + t.AcknowledgedPacket(encLevel, pn) + } +} + func (m *connTracerMultiplexer) LostPacket(encLevel EncryptionLevel, pn PacketNumber, reason PacketLossReason) { for _, t := range m.tracers { t.LostPacket(encLevel, pn, reason) diff --git a/logging/multiplex_test.go b/logging/multiplex_test.go index 28856aeca..1f01f4025 100644 --- a/logging/multiplex_test.go +++ b/logging/multiplex_test.go @@ -190,6 +190,12 @@ var _ = Describe("Tracing", func() { tracer.UpdatedMetrics(rttStats, 1337, 42, 13) }) + It("traces the AcknowledgedPacket event", func() { + tr1.EXPECT().AcknowledgedPacket(EncryptionHandshake, PacketNumber(42)) + tr2.EXPECT().AcknowledgedPacket(EncryptionHandshake, PacketNumber(42)) + tracer.AcknowledgedPacket(EncryptionHandshake, 42) + }) + It("traces the LostPacket event", func() { tr1.EXPECT().LostPacket(EncryptionHandshake, PacketNumber(42), PacketLossReorderingThreshold) tr2.EXPECT().LostPacket(EncryptionHandshake, PacketNumber(42), PacketLossReorderingThreshold) diff --git a/qlog/qlog.go b/qlog/qlog.go index 866c56f05..0d04a1015 100644 --- a/qlog/qlog.go +++ b/qlog/qlog.go @@ -348,6 +348,8 @@ func (t *connectionTracer) UpdatedMetrics(rttStats *utils.RTTStats, cwnd, bytesI t.mutex.Unlock() } +func (t *connectionTracer) AcknowledgedPacket(protocol.EncryptionLevel, protocol.PacketNumber) {} + func (t *connectionTracer) LostPacket(encLevel protocol.EncryptionLevel, pn protocol.PacketNumber, lossReason logging.PacketLossReason) { t.mutex.Lock() t.recordEvent(time.Now(), &eventPacketLost{ From c4073fba8a5a657481cb983317bea6a6a1fe63a6 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 2 Apr 2021 11:29:19 +0700 Subject: [PATCH 2/2] simplify detection of acknowledgements for skipped packets --- internal/ackhandler/sent_packet_handler.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 4e34478c7..59ed3e417 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -311,9 +311,6 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En return err } for _, p := range ackedPackets { - if p.skippedPacket { - return fmt.Errorf("received an ACK for skipped packet number: %d (%s)", p.PacketNumber, encLevel) - } if p.includedInBytesInFlight && !p.declaredLost { h.congestion.OnPacketAcked(p.PacketNumber, p.Length, priorInFlight, rcvTime) } @@ -367,15 +364,17 @@ func (h *sentPacketHandler) detectAndRemoveAckedPackets(ack *wire.AckFrame, encL ackRange = ack.AckRanges[len(ack.AckRanges)-1-ackRangeIndex] } - if p.PacketNumber >= ackRange.Smallest { // packet i contained in ACK range - if p.PacketNumber > ackRange.Largest { - return false, fmt.Errorf("BUG: ackhandler would have acked wrong packet %d, while evaluating range %d -> %d", p.PacketNumber, ackRange.Smallest, ackRange.Largest) - } - h.ackedPackets = append(h.ackedPackets, p) + if p.PacketNumber < ackRange.Smallest { // packet not contained in ACK range + return true, nil + } + if p.PacketNumber > ackRange.Largest { + return false, fmt.Errorf("BUG: ackhandler would have acked wrong packet %d, while evaluating range %d -> %d", p.PacketNumber, ackRange.Smallest, ackRange.Largest) } - } else { - h.ackedPackets = append(h.ackedPackets, p) } + if p.skippedPacket { + return false, fmt.Errorf("received an ACK for skipped packet number: %d (%s)", p.PacketNumber, encLevel) + } + h.ackedPackets = append(h.ackedPackets, p) return true, nil }) if h.logger.Debug() && len(h.ackedPackets) > 0 { @@ -399,7 +398,7 @@ func (h *sentPacketHandler) detectAndRemoveAckedPackets(ack *wire.AckFrame, encL if err := pnSpace.history.Remove(p.PacketNumber); err != nil { return nil, err } - if h.tracer != nil && !p.skippedPacket { + if h.tracer != nil { h.tracer.AcknowledgedPacket(encLevel, p.PacketNumber) } }