Date: Wed, 06 Sep 2017 00:19:15 +0000 (UTC)
From: janaiyengar <notifications@github.com>
Reply-To: quicwg/base-drafts
 <reply+0166e4ab04d2110cab09074f74494b1b05fcd9df2296895d92cf0000000115c7010292a169ce0edf94fd@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/721/review/60774331@github.com>
In-Reply-To: <quicwg/base-drafts/pull/721@github.com>
References: <quicwg/base-drafts/pull/721@github.com>
Subject: Re: [quicwg/base-drafts] Refactor the section on connection
 termination (#721)
Mime-Version: 1.0
Content-Type: multipart/alternative;
 boundary="--==_mimepart_59af3f02ad76a_4c353f7fa0de1c3c46142";
 charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/5V5Sd5VbVZ25ubsE6lYojZdkoMw>

----==_mimepart_59af3f02ad76a_4c353f7fa0de1c3c46142
Content-Type: text/plain;
 charset=UTF-8
Content-Transfer-Encoding: 7bit

janaiyengar approved this pull request.

A few nits, mostly editorial. LGTM'ing -- please feel free to merge once you've addressed these.

> +
+* application close ({{application-close}})
+* idle timeout ({{idle-timeout}})
+* immediate close ({{immediate-close}})
+* stateless reset ({{stateless-reset}})
+
+
+### Draining Period {#draining}
+
+After a connection is closed for any reason, an endpoint might receive packets
+from its peer.  These packets might have been sent prior to receiving any close
+signal, or they might be retransmissions of packets for which acknowledgments
+were lost.
+
+The draining period persists for three times the maximum of minimum RTO
+(kMinRTOTimeout) or the round trip time (smoothed_rtt), see {{QUIC-RECOVERY}}

I'm sorry if I mis-spoke, I would simply say "three times the RTO, see {{QUIC-RECOVERY}}." The RTO value is basically a max(minRTO, f(smoothed_rtt, rtt_variance)), and 3 times the RTO value allows for two consecutive timeout periods.

> +* idle timeout ({{idle-timeout}})
+* immediate close ({{immediate-close}})
+* stateless reset ({{stateless-reset}})
+
+
+### Draining Period {#draining}
+
+After a connection is closed for any reason, an endpoint might receive packets
+from its peer.  These packets might have been sent prior to receiving any close
+signal, or they might be retransmissions of packets for which acknowledgments
+were lost.
+
+The draining period persists for three times the maximum of minimum RTO
+(kMinRTOTimeout) or the round trip time (smoothed_rtt), see {{QUIC-RECOVERY}}
+for descriptions of these values.  During this period, new packets can be
+attributed to the correct connection and acknowledged, but the connection is no

nit: remove phrase "attributed to the correct connection and". Seems unnecessary.

> +* immediate close ({{immediate-close}})
+* stateless reset ({{stateless-reset}})
+
+
+### Draining Period {#draining}
+
+After a connection is closed for any reason, an endpoint might receive packets
+from its peer.  These packets might have been sent prior to receiving any close
+signal, or they might be retransmissions of packets for which acknowledgments
+were lost.
+
+The draining period persists for three times the maximum of minimum RTO
+(kMinRTOTimeout) or the round trip time (smoothed_rtt), see {{QUIC-RECOVERY}}
+for descriptions of these values.  During this period, new packets can be
+attributed to the correct connection and acknowledged, but the connection is no
+longer considered active and usable.

nit: instead of "the connection is no longer considered active and usable", how about "no new data or retransmittable packets may be sent on the connection"? The allowable frames are elucidated below, so I'd be happy with just "no new data may be sent on the connection."

> +immediately.  A CONNECTION_CLOSE causes all open streams to immediately become
+closed; open streams can be assumed to be implicitly reset.  After sending or
+receiving a CONNECTION_CLOSE frame, endpoints immediately enter a draining
+period.
+
+During the draining period, an endpoint that sends a CONNECTION_CLOSE frame
+SHOULD respond to any subsequent packet that it receives with another packet
+containing a CONNECTION_CLOSE frame.  To reduce the state that an endpoint
+maintains in this case, it MAY send the exact same packet.  However, endpoints
+SHOULD limit the number of CONNECTION_CLOSE messages they generate.  For
+instance, an endpoint could progressively increase the number of packets that it
+receives before sending additional CONNECTION_CLOSE frames.
+
+Note:
+
+: Allowing retransmision of a packet contradicts other advice in this document

typo: retransmision --> retransmission

-- 
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/721#pullrequestreview-60774331
----==_mimepart_59af3f02ad76a_4c353f7fa0de1c3c46142
Content-Type: text/html;
 charset=UTF-8
Content-Transfer-Encoding: 7bit

<p><b>@janaiyengar</b> approved this pull request.</p>

<p>A few nits, mostly editorial. LGTM'ing -- please feel free to merge once you've addressed these.</p><hr>

<p>In <a href="https://github.com/quicwg/base-drafts/pull/721#discussion_r137144893">draft-ietf-quic-transport.md</a>:</p>
<pre style='color:#555'>&gt; +
+* application close ({{application-close}})
+* idle timeout ({{idle-timeout}})
+* immediate close ({{immediate-close}})
+* stateless reset ({{stateless-reset}})
+
+
+### Draining Period {#draining}
+
+After a connection is closed for any reason, an endpoint might receive packets
+from its peer.  These packets might have been sent prior to receiving any close
+signal, or they might be retransmissions of packets for which acknowledgments
+were lost.
+
+The draining period persists for three times the maximum of minimum RTO
+(kMinRTOTimeout) or the round trip time (smoothed_rtt), see {{QUIC-RECOVERY}}
</pre>
<p>I'm sorry if I mis-spoke, I would simply say "three times the RTO, see {{QUIC-RECOVERY}}." The RTO value is basically a max(minRTO, f(smoothed_rtt, rtt_variance)), and 3 times the RTO value allows for two consecutive timeout periods.</p>

<hr>

<p>In <a href="https://github.com/quicwg/base-drafts/pull/721#discussion_r137145024">draft-ietf-quic-transport.md</a>:</p>
<pre style='color:#555'>&gt; +* idle timeout ({{idle-timeout}})
+* immediate close ({{immediate-close}})
+* stateless reset ({{stateless-reset}})
+
+
+### Draining Period {#draining}
+
+After a connection is closed for any reason, an endpoint might receive packets
+from its peer.  These packets might have been sent prior to receiving any close
+signal, or they might be retransmissions of packets for which acknowledgments
+were lost.
+
+The draining period persists for three times the maximum of minimum RTO
+(kMinRTOTimeout) or the round trip time (smoothed_rtt), see {{QUIC-RECOVERY}}
+for descriptions of these values.  During this period, new packets can be
+attributed to the correct connection and acknowledged, but the connection is no
</pre>
<p>nit: remove phrase "attributed to the correct connection and". Seems unnecessary.</p>

<hr>

<p>In <a href="https://github.com/quicwg/base-drafts/pull/721#discussion_r137145368">draft-ietf-quic-transport.md</a>:</p>
<pre style='color:#555'>&gt; +* immediate close ({{immediate-close}})
+* stateless reset ({{stateless-reset}})
+
+
+### Draining Period {#draining}
+
+After a connection is closed for any reason, an endpoint might receive packets
+from its peer.  These packets might have been sent prior to receiving any close
+signal, or they might be retransmissions of packets for which acknowledgments
+were lost.
+
+The draining period persists for three times the maximum of minimum RTO
+(kMinRTOTimeout) or the round trip time (smoothed_rtt), see {{QUIC-RECOVERY}}
+for descriptions of these values.  During this period, new packets can be
+attributed to the correct connection and acknowledged, but the connection is no
+longer considered active and usable.
</pre>
<p>nit: instead of "the connection is no longer considered active and usable", how about "no new data or retransmittable packets may be sent on the connection"? The allowable frames are elucidated below, so I'd be happy with just "no new data may be sent on the connection."</p>

<hr>

<p>In <a href="https://github.com/quicwg/base-drafts/pull/721#discussion_r137146093">draft-ietf-quic-transport.md</a>:</p>
<pre style='color:#555'>&gt; +immediately.  A CONNECTION_CLOSE causes all open streams to immediately become
+closed; open streams can be assumed to be implicitly reset.  After sending or
+receiving a CONNECTION_CLOSE frame, endpoints immediately enter a draining
+period.
+
+During the draining period, an endpoint that sends a CONNECTION_CLOSE frame
+SHOULD respond to any subsequent packet that it receives with another packet
+containing a CONNECTION_CLOSE frame.  To reduce the state that an endpoint
+maintains in this case, it MAY send the exact same packet.  However, endpoints
+SHOULD limit the number of CONNECTION_CLOSE messages they generate.  For
+instance, an endpoint could progressively increase the number of packets that it
+receives before sending additional CONNECTION_CLOSE frames.
+
+Note:
+
+: Allowing retransmision of a packet contradicts other advice in this document
</pre>
<p>typo: retransmision --&gt; retransmission</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">&mdash;<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/quicwg/base-drafts/pull/721#pullrequestreview-60774331">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AWbkq4gbSaDH5ZkFnL2s4CvTDlJjH7Sfks5sfeUCgaJpZM4O0OWB">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AWbkq6Z1nCF3V8Kz4ToTiizX_QVGiHTAks5sfeUCgaJpZM4O0OWB.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
  <link itemprop="url" href="https://github.com/quicwg/base-drafts/pull/721#pullrequestreview-60774331"></link>
  <meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/quicwg/base-drafts","title":"quicwg/base-drafts","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/quicwg/base-drafts"}},"updates":{"snippets":[{"icon":"PERSON","message":"@janaiyengar approved #721"}],"action":{"name":"View Pull Request","url":"https://github.com/quicwg/base-drafts/pull/721#pullrequestreview-60774331"}}}</script>
----==_mimepart_59af3f02ad76a_4c353f7fa0de1c3c46142--