Subject: [quicwg/base-drafts] Proposed fix for #3987 (#4071)

From: Lars Eggert <[email-masked]>
Date: Thu, 03 Sep 2020 05:51:05 -0700

In the interest of helping move things along, here&#39;s a proposal for #3987.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/4071

-- Commit Summary --

  * Proposed fix for #3987

-- File Changes --

    M draft-ietf-quic-transport.md (14)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/4071.patch
https://github.com/quicwg/base-drafts/pull/4071.diff

-- 
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/4071

---

From: ianswett <[email-masked]>
Date: Thu, 03 Sep 2020 11:58:58 -0700

@ianswett commented on this pull request.

I think we're using PTO in two cases because we want to allow time for the PTO to fire twice, but I could be wrong?

> @@ -2214,6 +2214,11 @@ defined in {{QUIC-RECOVERY}} is RECOMMENDED.  That is:
    validation_timeout = max(3*PTO, 6*kInitialRtt)
 ~~~
 
+The PTO includes the peer's maximum expected acknowledgement delay. This is not

In this case, we use PTO to allow for the PTO to fire twice before giving up, correct?  I think explaining it in terms of max ack delay is confusing.

> @@ -2693,6 +2698,10 @@ ensures that connections are not closed after new activity is initiated.
 
 To avoid excessively small idle timeout periods, endpoints MUST increase the
 idle timeout period to be at least three times the current Probe Timeout (PTO).
+As defined in {{QUIC-RECOVERY}}, the PTO includes the peer's maximum expected

Again, I think 3 * PTO is so there is sufficient opportunity for the PTO to fire before the connection idle timeouts?

-- 
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/4071#pullrequestreview-482118572

---

From: Lars Eggert <[email-masked]>
Date: Thu, 03 Sep 2020 23:15:28 -0700

The issue that this is intending to address isn't about "why use a multiple of PTO for the timer" but "why use PTO, which includes max ACK delay, and not e.g. derive from SRTT". At least that is how I understood it...

-- 
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/4071#issuecomment-686935677

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 04 Sep 2020 15:07:39 -0700

@janaiyengar requested changes on this pull request.

I don't think this is quite right. max ack delay is useful in idle and closing/draining timeouts. I've suggested text changes, PTAL. 

> +The PTO includes the peer's maximum expected acknowledgement delay. This is not
+strictly necessary for the purposes of path validation, but defining
+validation_timeout as a function of the PTO is convenient, since implementations
+need to maintain a PTO value anyway.

```suggestion
Note:

: Using the PTO value here implicitly includes the peer's maximum expected
acknowledgement delay in the validation timeout period; see Section 6.2.1
of {{QUIC-RECOVERY}}. This is not necessary for the purposes of path validation,
since the peer is expected to respond to a PATH_CHALLENGE without delay;
see {{path-response}}. However, defining the timeout period as a function of PTO
is convenient, since implementations will maintain a PTO value anyway.
```

> +As defined in {{QUIC-RECOVERY}}, the PTO includes the peer's maximum expected
+acknowledgement delay. This is not strictly necessary for the purposes of the
+idle timeout, but defining it in this way is convenient, since implementations
+need to maintain a PTO value anyway.

```suggestion

Note:

: Using the PTO value here implicitly includes the peer's maximum expected
acknowledgement delay in the idle timeout period; see Section 6.2.1
of {{QUIC-RECOVERY}}. This ensures that the endpoint accounts for this
known peer delay before declaring the connection as idle and closing it.
```

> +(PTO) interval as defined in {{QUIC-RECOVERY}}. Note that the PTO includes the
+peer's maximum expected acknowledgement delay. While not strictly necessary for
+this purpose, it is convenient, since implementations need to maintain a PTO
+value anyway.

```suggestion
(PTO) interval as defined in {{QUIC-RECOVERY}}.

Note:

: Using the PTO value here implicitly includes the peer's maximum expected
acknowledgement delay in the closing and draining periods; see Section 6.2.1
of {{QUIC-RECOVERY}}. This ensures that the endpoint takes this peer delay
into account when waiting to discard subsequent packets sent by the peer.
```

> +As defined in {{QUIC-RECOVERY}}, the PTO includes the peer's maximum expected
+acknowledgement delay. This is not strictly necessary for the purposes of the
+idle timeout, but defining it in this way is convenient, since implementations
+need to maintain a PTO value anyway.

This one is deliberate -- we want to account for max ack delay.

-- 
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/4071#pullrequestreview-482947265

---

From: Lars Eggert <[email-masked]>
Date: Mon, 07 Sep 2020 04:25:55 -0700

@larseggert pushed 1 commit.

47006c6eb805f83bb187938fc19515287db269ca  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4071/files/1866ab9d5a8cb01363e157ff63278c7c9b57e78f..47006c6eb805f83bb187938fc19515287db269ca

---

From: Lars Eggert <[email-masked]>
Date: Mon, 07 Sep 2020 04:26:06 -0700

89e30835e2681623bcbc605a7f7c681eb59c3569  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4071/files/47006c6eb805f83bb187938fc19515287db269ca..89e30835e2681623bcbc605a7f7c681eb59c3569

---

From: Lars Eggert <[email-masked]>
Date: Mon, 07 Sep 2020 04:26:15 -0700

30111ca1da15125685eb391d9488d97b0c4adc05  Update draft-ietf-quic-transport.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4071/files/89e30835e2681623bcbc605a7f7c681eb59c3569..30111ca1da15125685eb391d9488d97b0c4adc05

---

From: ianswett <[email-masked]>
Date: Mon, 07 Sep 2020 16:04:43 -0700

@larseggert in some cases, the reason for using PTO is to ensure there's time to PTO retransmit, so I don't think this change currently captures that reason in this cases.

I agree that's the basis of the issue as it stands, but I think the better question is why use PTO in different cases.  I think the max_ack_delay is not particularly interesting in any of these cases.

-- 
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/4071#issuecomment-688534747

---

From: ianswett <[email-masked]>
Date: Mon, 07 Sep 2020 16:05:25 -0700

@ianswett requested changes on this pull request.

I don't think we need all this extra text on max_ack_delay in these cases, even if that's how the issue was filed.

-- 
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/4071#pullrequestreview-483723046

---

From: Lars Eggert <[email-masked]>
Date: Tue, 08 Sep 2020 03:24:44 -0700

I'm completely fine with not making changes. @Ralith?

-- 
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/4071#issuecomment-688775146

---

From: Benjamin Saunders <[email-masked]>
Date: Tue, 08 Sep 2020 10:56:56 -0700

Maybe just reduce it to the first case? My goal with the issue was make it explicit when we use PTO, including max_ack_delay, for the sake of simplicity even though max_ack_delay isn't actually pertinent. If it serves a legitimate purpose for idle/close timers, then there's less need to justify its involvement, because that involvement is less likely to produce confusion and give the impression of a spec bug.

-- 
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/4071#issuecomment-689041057

---

From: Jana Iyengar <[email-masked]>
Date: Tue, 08 Sep 2020 16:04:24 -0700

@janaiyengar approved this pull request.

-- 
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/4071#pullrequestreview-484551538

---

From: Martin Thomson <[email-masked]>
Date: Tue, 08 Sep 2020 16:07:28 -0700

@martinthomson commented on this pull request.

I think that the document would be fine without any of these.  Certainly the second two are a distraction.

-- 
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/4071#pullrequestreview-484552637

---

From: Lars Eggert <[email-masked]>
Date: Wed, 09 Sep 2020 01:29:50 -0700

OK, I rev'ed the PR to only include the first note. Do folks think this is OK, do we want changes, or do we want to drop if altogether?

-- 
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/4071#issuecomment-689412938

---

From: Lars Eggert <[email-masked]>
Date: Wed, 09 Sep 2020 01:41:36 -0700

a9786f27413bb8a2bd8711a8a1d1b80dfe82bc2d  Add missing anchor

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4071/files/02869a7b94e0e27f2bf25e4118388e208696ce0c..a9786f27413bb8a2bd8711a8a1d1b80dfe82bc2d

---

From: ianswett <[email-masked]>
Date: Wed, 09 Sep 2020 14:15:53 -0700

I sent out an alternate PR: https://github.com/quicwg/base-drafts/pull/4084

-- 
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/4071#issuecomment-689826161

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 09 Sep 2020 15:52:18 -0700

@ianswett pointed out that the point here is not so much about including max_ack_delay, it is about using PTO values. Thinking about this, I agree. The idea here is to basically allow for the sender to retransmit the probe multiple times, and that happens on PTOs. Basically, I think what needs to be written is an explanation of the expression. My suggested explanation and text:

`6 x InitialRTT` ensures that the period is large enough for sending multiple PATH_CHALLENGE frames on initial timeout (with exponential backoff). `3 x PTO` ensures that _if the current path PTO is longer than `2 x InitialRTT`_  then we allow for the new path to be as long. If we want to clarify something here, that would be the point to clarify.

-- 
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/4071#issuecomment-689864326

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 09 Sep 2020 15:53:30 -0700

Uh, sorry, @ianswett : I posted my message and then saw your message (and PR) above. Looking now.

-- 
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/4071#issuecomment-689864738

---

From: Martin Thomson <[email-masked]>
Date: Wed, 09 Sep 2020 18:50:21 -0700

Closed #4071 via #4084.

-- 
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/4071#event-3748526062