Subject: Re: [quicwg/base-drafts] Send after receiving an ACK (#3047)

From: Jana Iyengar <[email-masked]>
Date: Thu, 24 Oct 2019 15:43:35 -0700

janaiyengar commented on this pull request.

> @@ -1031,9 +1031,8 @@ OnAckReceived(ack, pn_space):
     OnPacketAcked(acked_packet.packet_number, pn_space)
 
   DetectLostPackets(pn_space)

I propose we change this to be as follows, keeping with the discussion on the issue.
```
if (DetectLostPackets(pn_space)):
    SendOnePacket()
SendPacketsAsCongestionControllerAllows()
```

-- 
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/3047#pullrequestreview-306903829

---

From: ianswett <[email-masked]>
Date: Thu, 24 Oct 2019 16:51:06 -0700

ianswett commented on this pull request.

Q: Why would you always send one packet?  Depending on how ACKs were grouped, this could result in an unnecessary burst of packets that could defeat the congestion controller's intent.  Based on my experience, I don't think this is a purely theoretical problem.

-- 
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/3047#discussion_r338837625

---

From: martinduke <[email-masked]>
Date: Tue, 29 Oct 2019 14:46:14 -0700

martinduke approved this pull request.

>    pto_count = 0
-
   SetLossDetectionTimer()

we should delete this. OnPacketSent() already sets the timer, and SendPacketsAsCongestionControllerAllows may not actually send a packet.

-- 
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/3047#pullrequestreview-308854923

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 14:49:13 -0700

-
   SetLossDetectionTimer()

The ack may update the RTT, so I think we should  keep this unless we add a return value to SendPacketsAsCongestionControllerAllows() indicating whether a packet was sent?

-- 
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/3047#discussion_r340342713

---

From: martinduke <[email-masked]>
Date: Tue, 29 Oct 2019 15:04:38 -0700

martinduke commented on this pull request.

-
   SetLossDetectionTimer()

Ah, yes, we might have to reset the timer to reflect the new RTT. But then shouldn't the RTT update code go above this?

-- 
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/3047#discussion_r340348083

---

From: ianswett <[email-masked]>
Date: Tue, 29 Oct 2019 15:11:24 -0700

-
   SetLossDetectionTimer()

It's above on line 1024.

-- 
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/3047#discussion_r340350272

---

From: martinduke <[email-masked]>
Date: Tue, 29 Oct 2019 15:16:04 -0700

-
   SetLossDetectionTimer()

My mistake. I saw the UpdateRTT definition below and got confused.

-- 
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/3047#discussion_r340351853