Re: [quicwg/base-drafts] Add OnPacketReceived to pseudocode (#3653)

[Martin Thomson <[e-mail]>]

@martinthomson commented on this pull request.

Thanks for doing this.  I have one tweak that I think you need and maybe a tweak that you want.

The first is about the argument and what information is passed.  I think that this is just a notification from the main packet processing code, so it doesn't need to include packet processing.

The second is more involved and would involve adding more state variables.

> @@ -1142,6 +1142,21 @@ Pseudocode for OnPacketSent follows:
      SetLossDetectionTimer()
 ~~~
 
+## On Receiving a Packet
+
+When a packet is received, it may allow the server to send
+if the server was previously amplification limited.
+
+Psuedocode for OnPacketReceived follows:
+
+~~~
+OnPacketReceived(packet):
+  // If this packet unblocks the server, arm the

This is fine.  The alternative I considered was:

```
  if (server and peer address is not validated):
    was_blocked = aa_bytes_sent >= aa_bytes_received * 3
    aa_bytes_received += datagram.size()
    if was_blocked:
        SetLossDetectionTimer()
```
But this is fine.  In order for that to work, you need to have global state variables for aa_bytes_sent and aa_bytes_received, which you don't otherwise have.

> @@ -1142,6 +1142,21 @@ Pseudocode for OnPacketSent follows:
      SetLossDetectionTimer()
 ~~~
 
+## On Receiving a Packet
+
+When a packet is received, it may allow the server to send
+if the server was previously amplification limited.
+
+Psuedocode for OnPacketReceived follows:
+
+~~~
+OnPacketReceived(packet):

I would drop the argument to this.  Because this needs to be a datagram not a packet.

That means the function at the end should be called ProcessPackets(datagram) if you keep it.  But I would suggest that you don't need to do packet processing in this logic: this is just a function where the loss recovery code is told about what is happening in the main code.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3653#pullrequestreview-411395226