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.

Sign up for GitHub

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

Jump to bottom

Fix #182: Convert error codes before passing them to QUIC #211

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix #182: Convert error codes before passing them to QUIC #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
209a6c9
Add conversion function from error code to h3 error code
barafael Sep 3, 2024
9cbcd80
Fix #182: Convert error codes before passing them to QUIC
barafael Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions wtransport-proto/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,16 @@ pub enum ErrorCode {
}

impl ErrorCode {
/// Converts this stream [`ErrorCode`] to HTTP/3 error code.
/// <https://ietf-wg-webtrans.github.io/draft-ietf-webtrans-http3/draft-ietf-webtrans-http3.html#section-4.3)>
pub fn to_http3(self) -> VarInt {
let code = self.to_code();
Copy link
Contributor

@finnbear finnbear Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Upon reading all relevant documentation, it appears that the conversion is only for WT application error codes and not any of the existing ErrorCode variants, all of which are in the space of valid HTTP/3 error codes.

This can be seen as follows:

In other words, if we wanted application error codes, they would work like this:

enum ErrorCode{
    Datagram,
    NoError,
    ...
    Application(u8), // Edit: should be u32
}

impl ErrorCode {
    pub fn to_code(self) -> VarInt {
        match self {
            ErrorCode::Datagram => h3_error_codes::H3_DATAGRAM_ERROR,
            ErrorCode::NoError => h3_error_codes::H3_NO_ERROR,
            ...
            ErrorCode::Application(n) => to_http3(n),
        }
    }
}

But I'm not sure Application would be a useful variant. Instead, I suggest looking into whether reset(error_code) should be a u8 (edit: u32) and go through the to_http3 conversion:

wtransport/wtransport/src/stream.rs

Lines 83 to 93 in 4fcb949

/// Closes the send stream immediately.
///
/// No new data can be written after calling this method. Locally buffered data is dropped, and
/// previously transmitted data will no longer be retransmitted if lost. If an attempt has
/// already been made to finish the stream, the peer may still receive all written data.
///
/// If called more than once, subsequent calls will result in `StreamWriteError::Closed`.
#[inline(always)]
pub fn reset(&mut self, error_code: VarInt) -> Result<(), StreamWriteError> {
self.0.reset(error_code)
}

Likewise, consider converting VarInt to u8 (edit: u32) or Option<u8> (edit: Option<u32>), the result of a (potentially fallible) from_http3 conversion:

wtransport/wtransport/src/error.rs

Lines 157 to 159 in 4fcb949

/// The peer is no longer accepting data on this stream.
#[error("stream stopped (code: {0})")]
Stopped(VarInt),

(or, if non-application error codes are expected, consider exposing pub fn application_error_code(VarInt) -> Option<u8> so users can make sense of the error code).
(or, having a separate StreamWriteError variant for successfully converted application error codes)
(or, using Option<ErrorCode> instead of VarInt for Stopped)

Copy link
Contributor

@finnbear finnbear Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is one thing I'm unsure about:

I assume this is because these are two different versions of the spec.

Edit: Ah, it changed from 2^8 to 2^32 between drafts 5 and 6 (draft N can be obtained at https://www.ietf.org/archive/id/draft-ietf-webtrans-http3-0N.html#name-resetting-data-streams)

let first: u64 = 0x52e4a40fa8db;
let code = u64::from(code);
let val = first + code + (code / 0x1e);
VarInt::try_from_u64(val).expect("Valid conversion")
}

/// Returns the integer representation (code) of the error.
pub fn to_code(self) -> VarInt {
match self {
Expand Down
12 changes: 6 additions & 6 deletions wtransport/src/driver/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ impl Driver {

stream
.into_stream()
.stop(ErrorCode::BufferedStreamRejected.to_code())
.stop(ErrorCode::BufferedStreamRejected.to_http3())
.expect("Stream not already stopped");
}
}
Expand All @@ -164,7 +164,7 @@ impl Driver {
stream
.into_stream()
.1
.stop(ErrorCode::BufferedStreamRejected.to_code())
.stop(ErrorCode::BufferedStreamRejected.to_http3())
.expect("Stream not already stopped");
}
}
Expand Down Expand Up @@ -308,7 +308,7 @@ mod worker {

if let DriverError::Proto(error_code) = &error {
self.quic_connection
.close(varint_w2q(error_code.to_code()), b"");
.close(varint_w2q(error_code.to_http3()), b"");
}

self.driver_result.set(error);
Expand Down Expand Up @@ -595,14 +595,14 @@ mod worker {
Ok(session_request) => stream.into_session(session_request),
Err(HeadersParseError::MethodNotConnect) => {
stream
.stop(ErrorCode::RequestRejected.to_code())
.stop(ErrorCode::RequestRejected.to_http3())
.expect("Stream not already stopped");
return Ok(());
}
// TODO(biagio): we might have more granularity with errors
Err(_) => {
stream
.stop(ErrorCode::Message.to_code())
.stop(ErrorCode::Message.to_http3())
.expect("Stream not already stopped");
return Ok(());
}
Expand All @@ -613,7 +613,7 @@ mod worker {
Err(TrySendError::Full(mut stream)) => {
debug!("Discarding session request: sessions queue is full");
stream
.stop(ErrorCode::RequestRejected.to_code())
.stop(ErrorCode::RequestRejected.to_http3())
.expect("Stream not already stopped");
}
Err(TrySendError::Closed(_)) => return Err(DriverError::NotConnected),
Expand Down
10 changes: 5 additions & 5 deletions wtransport/src/endpoint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ impl Endpoint<endpoint_side::Client> {
let frame = match stream_session.read_frame().await {
Ok(frame) => frame,
Err(ProtoReadError::H3(error_code)) => {
quic_connection.close(varint_w2q(error_code.to_code()), b"");
quic_connection.close(varint_w2q(error_code.to_http3()), b"");
return Err(ConnectingError::ConnectionError(
ConnectionError::local_h3_error(error_code),
));
Expand All @@ -404,7 +404,7 @@ impl Endpoint<endpoint_side::Client> {
};

if !matches!(frame.kind(), FrameKind::Headers) {
quic_connection.close(varint_w2q(ErrorCode::FrameUnexpected.to_code()), b"");
quic_connection.close(varint_w2q(ErrorCode::FrameUnexpected.to_http3()), b"");
return Err(ConnectingError::ConnectionError(
ConnectionError::local_h3_error(ErrorCode::FrameUnexpected),
));
Expand All @@ -413,7 +413,7 @@ impl Endpoint<endpoint_side::Client> {
let headers = match Headers::with_frame(&frame) {
Ok(headers) => headers,
Err(error_code) => {
quic_connection.close(varint_w2q(error_code.to_code()), b"");
quic_connection.close(varint_w2q(error_code.to_http3()), b"");
return Err(ConnectingError::ConnectionError(
ConnectionError::local_h3_error(error_code),
));
Expand All @@ -423,7 +423,7 @@ impl Endpoint<endpoint_side::Client> {
let session_response = match SessionResponseProto::try_from(headers) {
Ok(session_response) => session_response,
Err(_) => {
quic_connection.close(varint_w2q(ErrorCode::Message.to_code()), b"");
quic_connection.close(varint_w2q(ErrorCode::Message.to_http3()), b"");
return Err(ConnectingError::ConnectionError(
ConnectionError::local_h3_error(ErrorCode::Message),
));
Expand Down Expand Up @@ -759,7 +759,7 @@ impl SessionRequest {
}
Err(ProtoWriteError::Stopped) => {
self.quic_connection
.close(varint_w2q(ErrorCode::ClosedCriticalStream.to_code()), b"");
.close(varint_w2q(ErrorCode::ClosedCriticalStream.to_http3()), b"");

Err(ConnectionError::local_h3_error(
ErrorCode::ClosedCriticalStream,
Expand Down