Subject: [quicwg/base-drafts] s/initial_max_stream_id_(uni|bidi)/initial_max_streams_$1/g (#1310)

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 20 Apr 2018 13:33:55 +0000 (UTC)

Fix the naming of initial maximum number of streams transport
parameters: these are not IDs, but rather counts.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * s/initial_max_stream_id_(uni|bidi)/initial_max_streams_$1/g

-- File Changes --

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

-- Patch Links --

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

---

From: Nick Banks <[email-masked]>
Date: Fri, 20 Apr 2018 13:36:56 +0000 (UTC)

nibanks approved this pull request.

LGTM

-- 
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/1310#pullrequestreview-113979397

---

From: Lucas Pardue <[email-masked]>
Date: Fri, 20 Apr 2018 13:38:45 +0000 (UTC)

How didn't I spot this in #1303?

Minor point that I won't argue strongly on: given its a count could we put "count" in the parameter name?

-- 
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/1310#issuecomment-383098767

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 20 Apr 2018 06:47:05 -0700

We could put "count" in the name, but then it would have to be "stream count" (no "s").  I think, however, that "streams" (plural) carries the same meaning.

-- 
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/1310#issuecomment-383101195

---

From: Lucas Pardue <[email-masked]>
Date: Fri, 20 Apr 2018 13:49:55 +0000 (UTC)

Agreed, its a hair splitting activity. ID is confusing currently, so perhaps a merge now and change later, if someone *really* has a 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/1310#issuecomment-383102080

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 20 Apr 2018 13:53:01 +0000 (UTC)

For my money, this is already long enough.  My concern here is variable names in my 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/1310#issuecomment-383103040

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 20 Apr 2018 07:44:02 -0700

As long as we are discussing this, two more thoughts:

To be consistent, the pair should be either _uni_ and _bi_ or _unidi_ and _bidi_.  I think the former is a better choice.

We could place _uni_ and _bi_ in front of "streams," resulting in `initial_max_uni_streams` and `initial_max_bi_streams`.  This makes it easier to read.

-- 
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/1310#issuecomment-383119146

---

From: Ryan Hamilton <[email-masked]>
Date: Fri, 20 Apr 2018 16:48:59 +0000 (UTC)

Something doesn't smell right about this change. Look at this language:

   initial_max_stream_id_bidi (0x0002):  
      ... Setting this parameter is
      equivalent to sending a MAX_STREAM_ID (Section 8.8) immediately

In other words, I would expect that I would be able to take the value from this transport parameter and hand it to the same machinery that handles MAX_STREAM_ID frames. If instead, this transport parameter tracks the *number* of open streams, that's no longer true. Instead we'd need to transform this parameter into a stream id. While that's obviously doable, it seems like there is value in being consistent.

What do you think?

-- 
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/1310#issuecomment-383156103

---

From: MikkelFJ <[email-masked]>
Date: Fri, 20 Apr 2018 09:58:54 -0700

> Instead we'd need to transform this parameter into a stream id. While that's obviously doable, it seems like there is value in being consistent.

I believe this was initially the case but it made it possible to have invalid arguments such as a uni stream id where a bidi was expected, and also because setting less than one stream of a given type would have to be zero, which is then an invalid stream type for some of the streams.

Even so, I agree. It makes more sense to have a max stream id and then close the connection on invalid stream id and make an excemption that a value of 0 indicates no stream other than stream 0 regardless of stream type.

The current dicussion shows that counts are highly confusing.

-- 
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/1310#issuecomment-383158737

---

From: MikkelFJ <[email-masked]>
Date: Fri, 20 Apr 2018 10:02:07 -0700

Another option is to rebase stream identifiers so the current stream 0 is stream 1 and stream 0 is always an invalid stream.

-- 
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/1310#issuecomment-383159596

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 20 Apr 2018 17:36:26 +0000 (UTC)

@RyanAtGoogle -- this is true, but the change is no worse than the original, which was already wrong in this regard:

```

-- 
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/1310#issuecomment-383168986

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 20 Apr 2018 17:38:03 +0000 (UTC)

> Another option is to rebase stream identifiers so the current stream 0 is stream 1 and stream 0 is always an invalid stream.

Let's not open this can of worms again 😃 

-- 
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/1310#issuecomment-383169444

---

From: Dmitri Tikhonov <[email-masked]>
Date: Fri, 20 Apr 2018 17:40:05 +0000 (UTC)

One way to think about these parameters is that making the values Stream IDs rather than simply counts would imply having to validate them, which client and server having different validation logic.  This also means additional error codes and so on.  The simpler transport parameters are, the better.

-- 
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/1310#issuecomment-383170042

---

From: Lucas Pardue <[email-masked]>
Date: Fri, 20 Apr 2018 17:42:44 +0000 (UTC)

Since Stream 0 has been identified as a problematic design, and there is work underway to think on a solution (iirc), I think maybe this and my PR #1303 should be parked.

-- 
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/1310#issuecomment-383170794

---

From: Mike Bishop <[email-masked]>
Date: Fri, 20 Apr 2018 14:19:31 -0700

MikeBishop commented on this pull request.

@RyanAtGoogle, the original text carried the same value as the frame, which required checking the type and failing on inappropriate values.  This *is* equivalent to receiving a frame -- it doesn't say a frame with the same value, after all.

(Though I wouldn't be averse to making the frame follow the same pattern, with stream type specified in the type byte and the payload being a count, that makes more sense in a world where the Stream ID spaces are separated rather than interleaved.  That ship has sailed, alas.)

> @@ -1213,7 +1213,7 @@ initial_max_streams_bidi (0x0002):
   equivalent to receiving a MAX_STREAM_ID containing 20 when received by a
   client or 17 when received by a server.
 
-initial_max_stream_id_uni (0x0008):
+initial_max_streams_uni (0x0008):
 
 : The initial maximum unidirectional streams parameter contains the initial

If the expansion is "initial maximum unidirectional streams", perhaps this should be "initial_max_uni_streams" instead, and likewise for bidi?

-- 
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/1310#pullrequestreview-114128946

---

From: Ryan Hamilton <[email-masked]>
Date: Fri, 20 Apr 2018 21:26:36 +0000 (UTC)

@MikeBishop Right. So the original text (before this PR) says that the transport param and the frame have the same semantics (and processing requirements). This seems great to me! :)

But this PR change the transport param to be different from the frame. That seems not great.

Am I misunderstanding?

-- 
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/1310#issuecomment-383227180

---

From: Victor Vasiliev <[email-masked]>
Date: Fri, 20 Apr 2018 21:29:06 +0000 (UTC)

> For example, a value of 0x05 would be equivalent to receiving a MAX_STREAM_ID containing 20 when received by a client or 17 when received by a server.

I'm not sure I understand why those need to be encoded differently in transport params and in the frame?

-- 
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/1310#issuecomment-383227671

---

From: Mike Bishop <[email-masked]>
Date: Fri, 20 Apr 2018 15:47:27 -0700

See #1023/#1065.  The very quick recap is that if you send the value in the frame, you have two key issues:

- Since the param is typed, you then have to validate that the supplied stream ID is actually of the appropriate type.
- Since 0 is a bidirectional stream, it's an invalid value for the other instances, yet it's the default.

By changing them to counts, there are no invalid values, which means fewer ways you can send the wrong thing or blow up inappropriately.

-- 
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/1310#issuecomment-383241478

---

From: Dmitri Tikhonov <[email-masked]>
Date: Sat, 21 Apr 2018 01:50:24 +0000 (UTC)

> Am I misunderstanding?

Yes, you are.  The current (11th) version of the draft describes the parameter as the count of streams, even though the name of the parameter implies otherwise:

```
   initial_max_stream_id_uni (0x0008):  The initial maximum
      unidirectional streams parameter contains the initial maximum
      number of application-owned unidirectional streams the peer may
      initiate, encoded as an unsigned 16-bit integer
```

This PR of mine means simply to change the parameter names.  The meaning remains unchanged.

-- 
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/1310#issuecomment-383258663

---

From: Marten Seemann <[email-masked]>
Date: Mon, 23 Apr 2018 01:15:55 +0000 (UTC)

marten-seemann commented on this pull request.

 

I agree. That sounds better.

-- 
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/1310#discussion_r183260104

---

From: Marten Seemann <[email-masked]>
Date: Mon, 23 Apr 2018 01:16:11 +0000 (UTC)

marten-seemann 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/1310#pullrequestreview-114223910

---

From: ianswett <[email-masked]>
Date: Mon, 23 Apr 2018 18:47:14 +0000 (UTC)

It'd be nice to have these consistent between the transport param and the MAX_STREAM_ID frame.  I'm fine with either approach, but I don't see a good argument for the current inconsistency.

-- 
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/1310#issuecomment-383680889

---

From: Lucas Pardue <[email-masked]>
Date: Mon, 23 Apr 2018 18:52:22 +0000 (UTC)

I agree but would changing MAX_FRAME_ID put us back onto the position we started in, with Max concurrent streams?

Rolling back the TPs reopen s their old problems too.

-- 
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/1310#issuecomment-383682887

---

From: ianswett <[email-masked]>
Date: Mon, 23 Apr 2018 18:56:01 +0000 (UTC)

We'd need to create two frames, ie: MAX_BIDI_STREAMS and MAX_UNI_STREAMS, but that seems ok.

-- 
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/1310#issuecomment-383684501

---

From: MikkelFJ <[email-masked]>
Date: Mon, 23 Apr 2018 12:00:01 -0700

@LPardue > with Max concurrent streams?

this might cause some race conditions - max is fine, but the id space could be separated via tagged frame types - I always wanted that,

-- 
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/1310#issuecomment-383685710

---

From: ianswett <[email-masked]>
Date: Mon, 23 Apr 2018 12:02:58 -0700

I realize once you head down the path of splitting the MAX_STREAM_ID frame, it seems natural to split a bunch of other frames(ie: flow control and STREAM frame), which starts becoming tedious, and not very interesting work, so it's probably an unwise direction.

-- 
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/1310#issuecomment-383686568

---

From: Lucas Pardue <[email-masked]>
Date: Mon, 23 Apr 2018 19:12:19 +0000 (UTC)

@ianswett the point I was trying to make was that the MAX_STREAM_ID was quite an elegant solution to the concurrency problem and moving back to the "old" way loses some of that (see https://tools.ietf.org/html/draft-ietf-quic-transport-02#section-10.3).

Given that the TPs are "initial stream count", would the proposed MAX_BIDI_STREAMS/MAX_UNI_STREAMS be a relative count, or an absolute count? 

-- 
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/1310#issuecomment-383689481

---

From: Dmitri Tikhonov <[email-masked]>
Date: Mon, 23 Apr 2018 19:19:25 +0000 (UTC)

I think we can separate the transport parameters from the frame handling.  This is a minor optimization in code versus simplicity of the specification.

-- 
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/1310#issuecomment-383691475

---

From: Mike Bishop <[email-masked]>
Date: Mon, 23 Apr 2018 20:40:27 +0000 (UTC)

@ianswett, I don't know how tedious the code is, but I think the split basically looks like chopping the last two bits off the Stream ID and embedding them in the frame type(s).  You'd need to do that for STREAM, MAX_STREAM_DATA, and MAX_STREAM_ID.  Each stream type has a continuously increasing stream ID space.  And ironically, these TPs would then be the actual maximum stream ID.

That gets especially appealing if the crypto stream is no longer eating the first item out of one space but instead is its own separate type that doesn't carry an ID.

However, this is a *total* bikeshed.  While I'm not wild about chartreuse and would willingly repaint, let's be clear that it is a bikeshed.

-- 
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/1310#issuecomment-383715422

---

From: janaiyengar <[email-masked]>
Date: Tue, 24 Apr 2018 22:49:42 +0000 (UTC)

>From editors' meeting: This is basically an editorial change. We agreed on changing the params to counts earlier (#1023/#1065), and folks would like to edit or add to that change substantively, let's do that on an issue first. This is in keeping with not discussing substantive things on PRs. I'm taking in nits from @MikeBishop and merging this PR as an editorial one, please open an issue for the rest of the discussion.

-- 
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/1310#issuecomment-384104053

---

From: janaiyengar <[email-masked]>
Date: Tue, 24 Apr 2018 17:13:20 -0700

@janaiyengar pushed 1 commit.

55b8da1  Mike's comment

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1310/files/fa63065aadc8b40b24f3f08f283e8ceb9f286082..55b8da192c9543c9b8f88a9e343fc239363d0e33

---

From: janaiyengar <[email-masked]>
Date: Wed, 25 Apr 2018 00:13:48 +0000 (UTC)

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/1310#pullrequestreview-115006682

---

From: janaiyengar <[email-masked]>
Date: Tue, 24 Apr 2018 17:14:50 -0700

Merged #1310.

-- 
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/1310#event-1592900757