Skip to content

Breaking draft 21 updates - #394

Merged
djc merged 23 commits into
masterfrom
draft-21-breaking
Jul 11, 2019
Merged

Breaking draft 21 updates#394
djc merged 23 commits into
masterfrom
draft-21-breaking

Conversation

@Ralith

@Ralith Ralith commented Jul 7, 2019

Copy link
Copy Markdown
Collaborator

Based on #393. I'd like some feedback on the introduction of Varint to public APIs. Alternative options include:

  • Panic on illegal values
    This might catch people by surprise, especially if error codes from unvetted external sources are used.
  • Add Result return values to currently infallible functions
    This is ugly.
  • Silently strip illegal bits
    This avoids the problems of the other two, but might still be an unpleasant surprise.

Drawbacks to the Varint approach include increased verbosity of infrequently-used APIs (connection close and stream reset/stop) and of inspecting application error codes in returned errors.

@Ralith
Ralith force-pushed the draft-21-breaking branch 3 times, most recently from 71be6e8 to 6d3b2a5 Compare July 7, 2019 07:35
@Ralith Ralith changed the title [WIP] Breaking draft 21 updates Breaking draft 21 updates Jul 7, 2019
@Ralith
Ralith force-pushed the draft-21-breaking branch 2 times, most recently from 6ed63e9 to d08e1b3 Compare July 8, 2019 17:16
@Ralith

Ralith commented Jul 8, 2019

Copy link
Copy Markdown
Collaborator Author

Switched capitalization to VarInt.

@Ralith
Ralith force-pushed the draft-21-breaking branch from d08e1b3 to 6e593ca Compare July 9, 2019 18:45
@djc

djc commented Jul 9, 2019

Copy link
Copy Markdown
Member

Nice one getting rid of ClientOpts!

djc
djc previously approved these changes Jul 9, 2019
@djc

djc commented Jul 9, 2019

Copy link
Copy Markdown
Member

My one request would be to consider moving all the code from varint into the VarInt type.

@Ralith
Ralith force-pushed the draft-21-breaking branch 4 times, most recently from d4cea56 to 088701f Compare July 10, 2019 07:41
Ensures correctness of application error codes without panics or
adding new internal failure paths.
@Ralith
Ralith force-pushed the draft-21-breaking branch from 088701f to de00d6e Compare July 10, 2019 07:50
@Ralith
Ralith force-pushed the draft-21-breaking branch 2 times, most recently from 63ff670 to 84482fb Compare July 11, 2019 06:20

@djc djc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking great, just a few nits.

Comment thread quinn-proto/src/varint.rs
pub const MAX: VarInt = VarInt((1 << 62) - 1);

/// Construct a `VarInt` infallibly
pub const fn from_u32(x: u32) -> Self {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need this if we also have the From impl?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

A From impl can't be a const fn (yet?).

Comment thread quinn-proto/src/connection.rs Outdated
Comment thread quinn-proto/src/transport_parameters.rs Outdated
Comment thread interop/src/main.rs
Comment thread quinn/src/connection.rs Outdated

/// A stream of QUIC streams initiated by a remote peer.
///
/// Incoming streams are opened in the same order that the peer created them, but data can be

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The former part of this sentence doesn't seem completely robust in case of datagram reordering, right?

@Ralith Ralith Jul 11, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is actually exactly correct as written, which is why it's worth calling out explicitly. Stream IDs are contiguous (modulo type bits), so if we see a discontinuity it's guaranteed that it's due to loss or reordering and the peer intended to open other streams first. This allows application protocols to assign special meaning to certain streams based on ordering without having to add tags, at the cost of requiring applications to take care not to do all stream processing in-order.

I'll expand it a bit for clarity.

@Ralith
Ralith force-pushed the draft-21-breaking branch from fe88211 to 4cd887a Compare July 11, 2019 15:25
Ralith added 13 commits July 11, 2019 08:44
This wasn't working, and is not required by the spec.
The Closed state is entered
- at the local application's request;
- at the peer's request;
- or when an error is detected.

In the first case, the application is already aware of the closure. In
the latter cases, connection loss is reported immediately. Hence,
reporting connection loss when a close frame is received in the Closed
state is always redundant.
APPLICATION_CLOSE and CONNECTION_CLOSE are handled similarly, so
separate cases was gratuitously verbose.
This allows peers to enter the draining state immediately, which could
reduce resource use.
This is a saner default, removing the need for a syscall for every
bit of randomness generated.
@Ralith
Ralith force-pushed the draft-21-breaking branch from 4cd887a to 9dd4da4 Compare July 11, 2019 15:45
@Ralith

Ralith commented Jul 11, 2019

Copy link
Copy Markdown
Collaborator Author

Moved client_hello into State::Handshake.

We only need the server config on incoming connections.

@djc djc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great stuff!

@djc
djc merged commit f173375 into master Jul 11, 2019
@delete-merged-branch
delete-merged-branch Bot deleted the draft-21-breaking branch July 11, 2019 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants