Subject: [quicwg/base-drafts] Fix for #3835 (#4014)

From: Lars Eggert <[email-masked]>
Date: Wed, 19 Aug 2020 06:17:31 -0700

You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Fix for #3835

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/4014.patch
https://github.com/quicwg/base-drafts/pull/4014.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/4014

---

From: Mike Bishop <[email-masked]>
Date: Wed, 19 Aug 2020 11:42:38 -0700

@MikeBishop approved this pull request.

I'm not convinced we need this, but it does no harm.

-- 
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/4014#pullrequestreview-470740510

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 19 Aug 2020 14:51:48 -0700

@janaiyengar requested changes on this pull request.

I don't see the point of this. The state transitions are currently only on things that move an endpoint out of that state. This transition is not one such.

I'll note that if this is to become an exhaustive FSM, that would be unreadable and not particularly useful.

-- 
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/4014#pullrequestreview-470990530

---

From: ianswett <[email-masked]>
Date: Wed, 19 Aug 2020 15:10:06 -0700

I also don't think this is necessary, but I'd like @martinthomson opinion

-- 
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/4014#issuecomment-676751705

---

From: Martin Thomson <[email-masked]>
Date: Wed, 19 Aug 2020 17:28:42 -0700

@martinthomson requested changes on this pull request.

I didn't want to do this.  I believe that more now that I've seen this.  These are both wrong, which demonstrates how this is non-trivial.  That's why this was originally limited to transitions.

> +              | Recv STREAM / STREAM_DATA_BLOCKED / RESET_STREAM
+              | Create Bidirectional Stream (Sending)
+              | Recv MAX_STREAM_DATA / STOP_SENDING (Bidirectional)
+              | Create Higher-Numbered Stream
+              v
+          +-------+
+          | Recv  | Recv RESET_STREAM
+       +->|       |-----------------------.
+Recv   |  +-------+                       |
+STREAM |    | |                           |
+       +----+ | Recv STREAM + FIN         |
+              v                           |
+          +-------+                       |
+          | Size  | Recv RESET_STREAM     |
+          | Known |---------------------->|
+          +-------+                       |

You receive STREAM frames here as well.

> +       +->|       |-----------------------.
+Recv   |  +-------+                       |
+STREAM |    | |                           |
+       +----+ | Recv STREAM + FIN         |
+              v                           |
+          +-------+                       |
+          | Size  | Recv RESET_STREAM     |
+          | Known |---------------------->|
+          +-------+                       |
+              |                           |
+              | Recv All Data             |
+              v                           v
+          +-------+ Recv RESET_STREAM +-------+
+          | Data  |--- (optional) --->| Reset |
+          | Recvd |  Recv All Data    | Recvd |
+          +-------+<-- (optional) ----+-------+

And here, in case of reordering or spurious retransmission.

> +               |      STREAM_DATA_BLOCKED  |
+               |                           |
+               | Peer Creates              |
+               |      Bidirectional Stream |
+               v                           |
+           +-------+                       |
+           | Send  | Send RESET_STREAM     |
+       +-->|       |---------------------->|
+Send   |   +-------+                       |
+STREAM |     | |                           |
+       +-----+ | Send STREAM + FIN         |
+               v                           v
+           +-------+                   +-------+
+           | Data  | Send RESET_STREAM | Reset |
+           | Sent  |------------------>| Sent  |
+           +-------+                   +-------+

You send STREAM frames here too when packets get declared lost.

-- 
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/4014#pullrequestreview-471049698

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 19 Aug 2020 20:41:24 -0700

Right, the problem is that this PR doesn't exhaustively list all loops, but doing so will make this state machine unreadable and useless. The existing FSM shows transitions out of a state, not all the ones that keep the endpoint within that state.

-- 
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/4014#issuecomment-676959260

---

From: Lars Eggert <[email-masked]>
Date: Wed, 19 Aug 2020 23:21:14 -0700

I'm fine with dropping this - I just wanted to move the resolution along by preparing a PR that demonstrated (part of, I guess) the changes needed. @ekr?

-- 
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/4014#issuecomment-677265402