ackhandler: simplify packet number generation#5191
ackhandler: simplify packet number generation#5191marten-seemann wants to merge 3 commits intomasterfrom
Conversation
No functional change expected.
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the old PeekPacketNumber/PopPacketNumber calls with a unified NextPacketNumber API across the packet packer and sent-packet handler, simplifying packet number generation without changing behavior.
- Swap
PeekPacketNumber+PopPacketNumberfor a singleNextPacketNumbermethod. - Update mocks and interfaces to reflect the new API.
- Refactor the sent-packet handler to track and issue packet numbers internally.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packet_packer_test.go | Remove PopPacketNumber calls and leave only PeekPacketNumber (now needs updating) |
| packet_packer.go | Replace peek/pop with NextPacketNumber in packet generation |
| internal/mocks/ackhandler/sent_packet_handler.go | Update mock generation to support NextPacketNumber |
| internal/ackhandler/sent_packet_handler.go | Refactor handler to use NextPacketNumber and track next numbers |
| internal/ackhandler/interfaces.go | Change interface to expose NextPacketNumber only |
| connection.go | Switch to calling NextPacketNumber on version negotiation |
Comments suppressed due to low confidence (2)
packet_packer_test.go:135
- After switching the interface to
NextPacketNumber, tests must mockNextPacketNumber(...)instead ofPeekPacketNumber(...)to avoid compilation failures and ensure correct expectations.
tp.pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x24), protocol.PacketNumberLen3)
internal/ackhandler/sent_packet_handler.go:63
- [nitpick] The abbreviation
PNGis ambiguous; consider renamingappDataPNGtoappDataPacketNumberGeneratorfor greater clarity.
appDataPNG *skippingPacketNumberGenerator
| ECNMode(isShortHeaderPacket bool) protocol.ECN // isShortHeaderPacket should only be true for non-coalesced 1-RTT packets | ||
| PeekPacketNumber(protocol.EncryptionLevel) (protocol.PacketNumber, protocol.PacketNumberLen) | ||
| PopPacketNumber(protocol.EncryptionLevel) protocol.PacketNumber | ||
| NextPacketNumber(protocol.EncryptionLevel) (protocol.PacketNumber, protocol.PacketNumberLen) |
There was a problem hiding this comment.
Update the interface documentation comment to reflect removal of PeekPacketNumber/PopPacketNumber and explain the semantics of NextPacketNumber.
c9fbcd0 to
42b2d7c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5191 +/- ##
==========================================
+ Coverage 84.00% 84.02% +0.02%
==========================================
Files 152 152
Lines 17350 17350
==========================================
+ Hits 14574 14578 +4
+ Misses 2216 2212 -4
Partials 560 560 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No functional change expected.