Introduce an API that lets the user get the current datagram size

[nanokatze]

cc @marten-seemann @chungthuang @joliveirinha @tanghaowillow

[nanokatze]

I'm having trouble generating mocks using the latest https://github.com/uber-go/mock: running
mockgen -typed -build_flags=-tags=gomock -package quic -self_package github.com/quic-go/quic-go -destination mock_packer_test.go github.com/quic-go/quic-go Packer as it says in the comment on top of packet_packer_test.go, if I look at the diff, some types get a Mock prefix they previously didn't, e.g.:

-// PackerAppendPacketCall wrap *gomock.Call
-type PackerAppendPacketCall struct {
+// MockPackerAppendPacketCall wrap *gomock.Call
+type MockPackerAppendPacketCall struct {
 	*gomock.Call
 }

which breaks lots of other code. Is this expected, and the other code using current names should be adjusted to use new names with Mock prefix or am I invoking it incorrectly?

[marten-seemann]

You should only need to run go generate ./....

[codecov]

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (0582e93) 84.17% compared to head (48bc3df) 84.01%.

❗ Current head 48bc3df differs from pull request most recent head 3d7ebd4. Consider uploading reports for the commit 3d7ebd4 to get more accurate results

Files	Patch %	Lines
packet_packer.go	0.00%	9 Missing ⚠️
connection.go	57.14%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4294      +/-   ##
==========================================
- Coverage   84.17%   84.01%   -0.15%     
==========================================
  Files         149      150       +1     
  Lines       15397    15450      +53     
==========================================
+ Hits        12959    12980      +21     
- Misses       1939     1967      +28     
- Partials      499      503       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[chungthuang]

Have we consider using atomic uint32?

[nanokatze]

I'm not convinced focusing on this is worthwhile right now. There are more important things to address first (which would likely affect whatever performance impact of updateMaxDatagramDataSize has), and if connStateMutex contention turns out to be a problem in the end, I think we might want to revisit how all of ConnectionState is accessed, not just this field.

[marten-seemann]

Can we be smarter than running this once per runLoop iteration? This is the hot path.

[nanokatze]

Do you mean this should be moved to the end of runLoop body? I wasn't sure under which conditions end of the runLoop body is or isn't reachable.

[marten-seemann]

Not really. runLoop iterates once per (batch of) packets received and sent. Which is a lot on an active QUIC connection.

The available space in the packet however only changes under 2 conditions (I think):

1. When we update the connection ID.
2. When we discover a new MTU.

[nanokatze]

I think there's another reason: encoded packet number length increases, which IIUC grows with RTT and how fast we send packets (as in, packets per unit of time.)

I also just realized I misread and misunderstood your initial comment, oops.

The simplest way I can think of would be just to rate limit updateMaxDatagramSize. A more "precise" solution I can think of would be to run it in response to the 3 events that can change the available space: connection ID changed, PMTU changed, packet number length changed. Packet number length can change after every packet we send and on every ACK we receive. This reasoning is in fact why I initially went with running updateMaxDatagramSize every runLoop, as it runs in response to all of these events. In any case, do you think just rate limiting so that updateMaxDatagramSize runs once in a while, e.g. every 10 ms or every N iterations of runLoop, would be good enough solution, or should we instead try to run updateMaxDatagramSize in response to 3 events listed above? If we go with "every once in a while" solution, the users could be advised to underutilize the available space by 1-2 bytes to allow for growth if they start sending datagrams after a period of inactivity. We could also trigger updateMaxDatagramSize in response to discarding datagrams when sending.

[marten-seemann]

uint16?

[nanokatze]

I don't mind making it uint16, but I don't see what improvement this would have. At worst I believe this would, in some cases, cause the users to perform additional conversion in their code, as dealing with sizes of things that fit in core in Go conventionally is done in ints.