Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial suggestions on the Streams section #2730

Merged
merged 7 commits into from Jun 25, 2019

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented May 20, 2019

Feel free to take the suggestions you like and revert or fix the others.

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, some of which are important enough to fix.

@@ -187,8 +187,9 @@ QUIC:

QUIC packet:

: The smallest unit of QUIC that can be encapsulated in a UDP datagram. Multiple
QUIC packets can be encapsulated in a single UDP datagram.
: A complete processable payload with a packet type, encryption level and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this. I agree that the existing definition isn't good, but "payload" means something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Payload was my best suggestion here, so I'm open to others.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structure? Sequence of octets? Or drop it entirely and define a packet as a header, payload, and authentication tag in that order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mike, I updated it to header, payload, and authentication tag.

abstraction.
application. Streams can be unidirectional or bidirecational. An alternative
view of QUIC unidirectional streams is a "message" abstraction of unlimited
length.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Streams aren't unlimited length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

practically unlimited?

has all stream data. Any STREAM or STREAM_DATA_BLOCKED frames it receives for
the stream can be discarded.
frame that causes the transition to "Size Known". Any STREAM or
STREAM_DATA_BLOCKED frames it receives for the stream can be discarded.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize here that the last sentence here, if taken out of context, might be misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, PTAL

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
If the STOP_SENDING frame is received on a stream that is already in the
"Data Sent" state, an endpoint that wishes to cease retransmission of
previously-sent STREAM frames on that stream MUST first send a RESET_STREAM
frame.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon reading, I thought it was redundant with the surrounding text, but I'll restore it for this PR.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Show resolved Hide resolved
@@ -861,13 +855,12 @@ If either is received, the connection MUST be closed immediately with a
connection error of type STREAM_LIMIT_ERROR (see {{immediate-close}}).

Endpoints MUST NOT exceed the limit set by their peer. An endpoint that
receives a STREAM frame with a stream ID exceeding the limit it has sent MUST
receives a frame with a stream ID exceeding the limit it has sent MUST
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are talking here about trying to include STREAM_BLOCKED and RESET_STREAM as well as STREAM? But by making it generic you might accidentally capture STOP_SENDING.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but I think that's a feature, not a bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, but wouldn't this then be a design change? This is an editorial PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I'll revert that and if we think it's worth changing, I'll open a design issue for it.

Copy link
Contributor Author

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Martin, I made some updates based on your suggestions.

@@ -187,8 +187,9 @@ QUIC:

QUIC packet:

: The smallest unit of QUIC that can be encapsulated in a UDP datagram. Multiple
QUIC packets can be encapsulated in a single UDP datagram.
: A complete processable payload with a packet type, encryption level and
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Payload was my best suggestion here, so I'm open to others.

abstraction.
application. Streams can be unidirectional or bidirecational. An alternative
view of QUIC unidirectional streams is a "message" abstraction of unlimited
length.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

practically unlimited?

has all stream data. Any STREAM or STREAM_DATA_BLOCKED frames it receives for
the stream can be discarded.
frame that causes the transition to "Size Known". Any STREAM or
STREAM_DATA_BLOCKED frames it receives for the stream can be discarded.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, PTAL

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
@@ -861,13 +855,12 @@ If either is received, the connection MUST be closed immediately with a
connection error of type STREAM_LIMIT_ERROR (see {{immediate-close}}).

Endpoints MUST NOT exceed the limit set by their peer. An endpoint that
receives a STREAM frame with a stream ID exceeding the limit it has sent MUST
receives a frame with a stream ID exceeding the limit it has sent MUST
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but I think that's a feature, not a bug.

If the STOP_SENDING frame is received on a stream that is already in the
"Data Sent" state, an endpoint that wishes to cease retransmission of
previously-sent STREAM frames on that stream MUST first send a RESET_STREAM
frame.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon reading, I thought it was redundant with the surrounding text, but I'll restore it for this PR.

QUIC packets can be encapsulated in a single UDP datagram.
: A complete processable payload with a packet type, encryption level and
packet number. Multiple QUIC packets can be encapsulated in a single UDP
datagram.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An authenticated and encrypted container of payload usually carrying a packet number and sometimes a connection identifier and version information in its header.

it relies on receiving priority information from the application that uses QUIC.
QUIC does not provide a mechanism for exchanging prioritization information.
Instead it relies on receiving priority information from the application that
uses QUIC.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uses QUIC.
uses QUIC in an implementation specific manner.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementation-specific, with the hyphen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this, then removed it because it read somewhat redundantly.

@@ -187,8 +187,9 @@ QUIC:

QUIC packet:

: The smallest unit of QUIC that can be encapsulated in a UDP datagram. Multiple
QUIC packets can be encapsulated in a single UDP datagram.
: A complete processable payload with a packet type, encryption level and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structure? Sequence of octets? Or drop it entirely and define a packet as a header, payload, and authentication tag in that order.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
it relies on receiving priority information from the application that uses QUIC.
QUIC does not provide a mechanism for exchanging prioritization information.
Instead it relies on receiving priority information from the application that
uses QUIC.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementation-specific, with the hyphen

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ianswett ianswett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mike, I addressed or accepted most of your comments.

draft-ietf-quic-transport.md Show resolved Hide resolved
it relies on receiving priority information from the application that uses QUIC.
QUIC does not provide a mechanism for exchanging prioritization information.
Instead it relies on receiving priority information from the application that
uses QUIC.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this, then removed it because it read somewhat redundantly.

packet number. Multiple QUIC packets can be encapsulated in a single UDP
datagram.
: A complete processable unit of QUIC consisting of a header, payload, and
authentication tag. Multiple QUIC packets can be encapsulated in a single
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all packets have an authentication tag. A single atomic unit is really the only thing we can say.

@martinthomson martinthomson merged commit dd947bc into master Jun 25, 2019
@martinthomson martinthomson added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Jun 25, 2019
@martinthomson martinthomson deleted the ianswett-streams-editorial branch June 26, 2019 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants