Skip to content

Commit efdc2d8

Browse files
authored
Refactor Error handling to fix bugs (#271)
* progress * fix test * remove redundant method for client/server data handling * fmt * notify connection on error and fix tests * more test fixing * correct duvet citation in test * fix a test * remove panic * fix tests * fix the rest of the errors * progress new error type * trying refactored error types * start to rethink shared state by removing the global Arc<RwLock<>> and replacing it with Atomics or OnceLock structures for each different thing * discover problem with blocked accept function if a request stream is started but no headers sent. This is the fist step to fix this with a test. But the test is not perfect yet * some progress with new error types * fmt * accept should not block * ignore wt for now, as more changes are needed. fix in a later commit * make it compile again * H3_REQUEST_INCOMPLETE is now a stream error * check error on resolver * fix tests with new api * fix last test * progress on new types * progress * progress * new error types, avoid allocation and fix some violations with unidirectional streams * progress * progress * progress * progress * progress * rename qpack errors * progress * progress * progress * ignore datagrams for now * ignore datagram and wt for now * progress * fix datagram now * progress * progress * progress * fix tests * more tests * compiles again * progress * progress * progress * progress * fix some tests * fix more errors * remove unnecessary option in return type * some things about graceful shutdown * add qpack spec to duvet * header errors and removal of old error code * fix datagram feature * fix more tests * fmt * repair doc fail * fix h3-datagram and more cleanup * fmt * rename Error variant * fmt * progress on changes for h3-webtransport * progress on webtransport * opened another rabbit hole by starting to refactor the h3-datagram traits. But it is necessary to make h3-webtransport work with the new error types * new datagram traits * progress * remove qpck spec from duvet to short this pr. Since there are no citations I can simply add this in a follow up PR again. * cleanup duvet * cleanup example * cleanup Todo comment * comment is not true anymore * finnish comment * remove old code * remove outdated examples from readme. * more cleanup * format * duvet update * non_exhaustive * malformed * Arc -> Box
1 parent 9b02824 commit efdc2d8

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

45 files changed

+3572
-2507
lines changed

.duvet/snapshot.txt

+40-40
Original file line numberDiff line numberDiff line change
@@ -455,17 +455,17 @@ SPECIFICATION: https://www.rfc-editor.org/rfc/rfc9114
455455
TEXT[!SHOULD,implementation]: (Section 8.1).
456456
TEXT[!MUST,implication]: The recipient MUST NOT consider unknown stream types
457457
TEXT[!MUST,implication]: to be a connection error of any kind.
458-
TEXT[!SHOULD,implication]: As certain stream types can affect connection state, a recipient
459-
TEXT[!SHOULD,implication]: SHOULD NOT discard data from incoming unidirectional streams prior to
460-
TEXT[!SHOULD,implication]: reading the stream type.
458+
TEXT[!SHOULD]: As certain stream types can affect connection state, a recipient
459+
TEXT[!SHOULD]: SHOULD NOT discard data from incoming unidirectional streams prior to
460+
TEXT[!SHOULD]: reading the stream type.
461461
TEXT[!MAY,todo]: Implementations MAY send stream types before knowing whether the peer
462462
TEXT[!MAY,todo]: supports them.
463463
TEXT[!MUST,todo]: However, stream types that could modify the state or
464464
TEXT[!MUST,todo]: semantics of existing protocol components, including QPACK or other
465465
TEXT[!MUST,todo]: extensions, MUST NOT be sent until the peer is known to support them.
466-
TEXT[!MUST,todo]: A receiver MUST tolerate unidirectional streams being
467-
TEXT[!MUST,todo]: closed or reset prior to the reception of the unidirectional stream
468-
TEXT[!MUST,todo]: header.
466+
TEXT[!MUST,implementation,todo]: A receiver MUST tolerate unidirectional streams being
467+
TEXT[!MUST,implementation,todo]: closed or reset prior to the reception of the unidirectional stream
468+
TEXT[!MUST,implementation,todo]: header.
469469

470470
SECTION: [Control Streams](#section-6.2.1)
471471
TEXT[!MUST,implementation]: Each side MUST initiate a single control stream at the beginning of
@@ -510,11 +510,11 @@ SPECIFICATION: https://www.rfc-editor.org/rfc/rfc9114
510510
TEXT[!MAY,implementation]: sent on connections where no data is currently being transferred.
511511
TEXT[!MUST,implication]: Endpoints MUST NOT consider these streams to have any meaning upon
512512
TEXT[!MUST,implication]: receipt.
513-
TEXT[!MAY,implementation]: When sending a reserved stream type,
514-
TEXT[!MAY,implementation]: the implementation MAY either terminate the stream cleanly or reset
515-
TEXT[!MAY,implementation]: it.
516-
TEXT[!SHOULD]: When resetting the stream, either the H3_NO_ERROR error code or
517-
TEXT[!SHOULD]: a reserved error code (Section 8.1) SHOULD be used.
513+
TEXT[!MAY,implication]: When sending a reserved stream type,
514+
TEXT[!MAY,implication]: the implementation MAY either terminate the stream cleanly or reset
515+
TEXT[!MAY,implication]: it.
516+
TEXT[!SHOULD,exception]: When resetting the stream, either the H3_NO_ERROR error code or
517+
TEXT[!SHOULD,exception]: a reserved error code (Section 8.1) SHOULD be used.
518518

519519
SECTION: [Frame Layout](#section-7.1)
520520
TEXT[!MUST,todo]: Each frame's payload MUST contain exactly the fields identified in
@@ -523,8 +523,8 @@ SPECIFICATION: https://www.rfc-editor.org/rfc/rfc9114
523523
TEXT[!MUST,implementation,test]: after the identified fields or a frame payload that terminates before
524524
TEXT[!MUST,implementation,test]: the end of the identified fields MUST be treated as a connection
525525
TEXT[!MUST,implementation,test]: error of type H3_FRAME_ERROR.
526-
TEXT[!MUST,implementation,test]: In particular, redundant length
527-
TEXT[!MUST,implementation,test]: encodings MUST be verified to be self-consistent; see Section 10.8.
526+
TEXT[!MUST]: In particular, redundant length
527+
TEXT[!MUST]: encodings MUST be verified to be self-consistent; see Section 10.8.
528528
TEXT[!MUST,implementation]: When a stream terminates cleanly, if the last frame on the stream was
529529
TEXT[!MUST,implementation]: truncated, this MUST be treated as a connection error of type
530530
TEXT[!MUST,implementation]: H3_FRAME_ERROR.
@@ -565,10 +565,10 @@ SPECIFICATION: https://www.rfc-editor.org/rfc/rfc9114
565565
TEXT[!MUST,implementation,test]: Receiving a
566566
TEXT[!MUST,implementation,test]: CANCEL_PUSH frame on a stream other than the control stream MUST be
567567
TEXT[!MUST,implementation,test]: treated as a connection error of type H3_FRAME_UNEXPECTED.
568-
TEXT[!MUST,todo]: If a CANCEL_PUSH frame is received that
569-
TEXT[!MUST,todo]: references a push ID greater than currently allowed on the
570-
TEXT[!MUST,todo]: connection, this MUST be treated as a connection error of type
571-
TEXT[!MUST,todo]: H3_ID_ERROR.
568+
TEXT[!MUST]: If a CANCEL_PUSH frame is received that
569+
TEXT[!MUST]: references a push ID greater than currently allowed on the
570+
TEXT[!MUST]: connection, this MUST be treated as a connection error of type
571+
TEXT[!MUST]: H3_ID_ERROR.
572572
TEXT[!MUST,todo]: If a server receives a CANCEL_PUSH frame for a push
573573
TEXT[!MUST,todo]: ID that has not yet been mentioned by a PUSH_PROMISE frame, this MUST
574574
TEXT[!MUST,todo]: be treated as a connection error of type H3_ID_ERROR.
@@ -587,10 +587,10 @@ SPECIFICATION: https://www.rfc-editor.org/rfc/rfc9114
587587
TEXT[!MUST,implementation,test]: H3_FRAME_UNEXPECTED.
588588
TEXT[!MUST,implementation]: The same setting identifier MUST NOT occur more than once in the
589589
TEXT[!MUST,implementation]: SETTINGS frame.
590-
TEXT[!MAY,todo]: A receiver MAY treat the presence of duplicate
591-
TEXT[!MAY,todo]: setting identifiers as a connection error of type H3_SETTINGS_ERROR.
592-
TEXT[!MUST,implication]: An implementation MUST ignore any parameter with an identifier it
593-
TEXT[!MUST,implication]: does not understand.
590+
TEXT[!MAY]: A receiver MAY treat the presence of duplicate
591+
TEXT[!MAY]: setting identifiers as a connection error of type H3_SETTINGS_ERROR.
592+
TEXT[!MUST]: An implementation MUST ignore any parameter with an identifier it
593+
TEXT[!MUST]: does not understand.
594594

595595
SECTION: [Defined SETTINGS Parameters](#section-7.2.4.1)
596596
TEXT[implementation]: Setting identifiers of the format 0x1f * N + 0x21 for non-negative
@@ -599,22 +599,22 @@ SPECIFICATION: https://www.rfc-editor.org/rfc/rfc9114
599599
TEXT[implementation]: meaning.
600600
TEXT[!SHOULD,implementation]: Endpoints SHOULD include at least one such setting in their
601601
TEXT[!SHOULD,implementation]: SETTINGS frame.
602-
TEXT[!MUST,implication]: Endpoints MUST NOT consider such settings to have
603-
TEXT[!MUST,implication]: any meaning upon receipt.
604-
TEXT[implementation,todo]: Setting identifiers that were defined in [HTTP/2] where there is no
605-
TEXT[implementation,todo]: corresponding HTTP/3 setting have also been reserved
606-
TEXT[implementation,todo]: (Section 11.2.2).
607-
TEXT[!MUST,implementation,todo]: These reserved settings MUST NOT be sent, and
608-
TEXT[!MUST,implementation,todo]: their receipt MUST be treated as a connection error of type
609-
TEXT[!MUST,implementation,todo]: H3_SETTINGS_ERROR.
602+
TEXT[!MUST,implementation]: Endpoints MUST NOT consider such settings to have
603+
TEXT[!MUST,implementation]: any meaning upon receipt.
604+
TEXT[implementation]: Setting identifiers that were defined in [HTTP/2] where there is no
605+
TEXT[implementation]: corresponding HTTP/3 setting have also been reserved
606+
TEXT[implementation]: (Section 11.2.2).
607+
TEXT[!MUST,implementation]: These reserved settings MUST NOT be sent, and
608+
TEXT[!MUST,implementation]: their receipt MUST be treated as a connection error of type
609+
TEXT[!MUST,implementation]: H3_SETTINGS_ERROR.
610610

611611
SECTION: [Initialization](#section-7.2.4.2)
612-
TEXT[!MUST,todo]: An HTTP implementation MUST NOT send frames or requests that would be
613-
TEXT[!MUST,todo]: invalid based on its current understanding of the peer's settings.
614-
TEXT[!SHOULD,todo]: Each endpoint SHOULD use
615-
TEXT[!SHOULD,todo]: these initial values to send messages before the peer's SETTINGS
616-
TEXT[!SHOULD,todo]: frame has arrived, as packets carrying the settings can be lost or
617-
TEXT[!SHOULD,todo]: delayed.
612+
TEXT[!MUST,implementation,todo]: An HTTP implementation MUST NOT send frames or requests that would be
613+
TEXT[!MUST,implementation,todo]: invalid based on its current understanding of the peer's settings.
614+
TEXT[!SHOULD,implementation,todo]: Each endpoint SHOULD use
615+
TEXT[!SHOULD,implementation,todo]: these initial values to send messages before the peer's SETTINGS
616+
TEXT[!SHOULD,implementation,todo]: frame has arrived, as packets carrying the settings can be lost or
617+
TEXT[!SHOULD,implementation,todo]: delayed.
618618
TEXT[!MUST,implication]: Endpoints MUST NOT require any data to be received from
619619
TEXT[!MUST,implication]: the peer prior to sending the SETTINGS frame; settings MUST be sent
620620
TEXT[!MUST,implication]: as soon as the transport is ready to send data.
@@ -715,13 +715,13 @@ SPECIFICATION: https://www.rfc-editor.org/rfc/rfc9114
715715
TEXT[implication]: types be ignored (Section 9).
716716
TEXT[!MAY,implication]: These frames have no semantics, and
717717
TEXT[!MAY,implication]: they MAY be sent on any stream where frames are allowed to be sent.
718-
TEXT[!MUST,todo]: Endpoints MUST
719-
TEXT[!MUST,todo]: NOT consider these frames to have any meaning upon receipt.
718+
TEXT[!MUST,implementation,todo]: Endpoints MUST
719+
TEXT[!MUST,implementation,todo]: NOT consider these frames to have any meaning upon receipt.
720720
TEXT[todo]: Frame types that were used in HTTP/2 where there is no corresponding
721721
TEXT[todo]: HTTP/3 frame have also been reserved (Section 11.2.1).
722-
TEXT[!MUST,todo]: These frame
723-
TEXT[!MUST,todo]: types MUST NOT be sent, and their receipt MUST be treated as a
724-
TEXT[!MUST,todo]: connection error of type H3_FRAME_UNEXPECTED.
722+
TEXT[!MUST,implementation,todo]: These frame
723+
TEXT[!MUST,implementation,todo]: types MUST NOT be sent, and their receipt MUST be treated as a
724+
TEXT[!MUST,implementation,todo]: connection error of type H3_FRAME_UNEXPECTED.
725725

726726
SECTION: [Error Handling](#section-8)
727727
TEXT[!MAY,todo]: An endpoint MAY choose to treat a stream error as a connection error

README.md

-62
Original file line numberDiff line numberDiff line change
@@ -57,72 +57,10 @@ The [examples](./examples) directory can help get started in two ways:
5757

5858
### Server
5959

60-
```rust
61-
let (endpoint, mut incoming) = h3_quinn::quinn::Endpoint::server(server_config, "[::]:443".parse()?)?;
62-
63-
while let Some((req, stream)) = h3_conn.accept().await? {
64-
loop {
65-
match h3_conn.accept().await {
66-
Ok(Some((req, mut stream))) => {
67-
let resp = http::Response::builder().status(Status::OK).body(())?;
68-
stream.send_response(resp).await?;
69-
70-
stream.send_data(Bytes::new("It works!")).await?;
71-
stream.finish().await?;
72-
}
73-
Ok(None) => {
74-
break;
75-
}
76-
Err(err) => {
77-
match err.get_error_level() {
78-
ErrorLevel::ConnectionError => break,
79-
ErrorLevel::StreamError => continue,
80-
}
81-
}
82-
}
83-
}
84-
}
85-
endpoint.wait_idle();
86-
```
87-
8860
You can find a full server example in [`examples/server.rs`](./examples/server.rs)
8961

9062
### Client
9163

92-
``` rust
93-
let addr: SocketAddr = "[::1]:443".parse()?;
94-
95-
let quic = h3_quinn::Connection::new(client_endpoint.connect(addr, "server")?.await?);
96-
let (mut driver, mut send_request) = h3::client::new(quinn_conn).await?;
97-
98-
let drive = async move {
99-
future::poll_fn(|cx| driver.poll_close(cx)).await?;
100-
Ok::<(), Box<dyn std::error::Error>>(())
101-
};
102-
103-
let request = async move {
104-
let req = http::Request::builder().uri(dest).body(())?;
105-
106-
let mut stream = send_request.send_request(req).await?;
107-
stream.finish().await?;
108-
109-
let resp = stream.recv_response().await?;
110-
111-
while let Some(mut chunk) = stream.recv_data().await? {
112-
let mut out = tokio::io::stdout();
113-
out.write_all_buf(&mut chunk).await?;
114-
out.flush().await?;
115-
}
116-
Ok::<_, Box<dyn std::error::Error>>(())
117-
};
118-
119-
let (req_res, drive_res) = tokio::join!(request, drive);
120-
req_res?;
121-
drive_res?;
122-
123-
client_endpoint.wait_idle().await;
124-
```
125-
12664
You can find a full client example in [`examples/client.rs`](./examples/client.rs)
12765

12866
## QUIC Generic

examples/client.rs

+23-8
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use std::{path::PathBuf, sync::Arc};
22

33
use futures::future;
4+
use h3::error::{ConnectionError, StreamError};
45
use rustls::pki_types::CertificateDer;
56
use structopt::StructOpt;
67
use tokio::io::AsyncWriteExt;
@@ -116,8 +117,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
116117
let (mut driver, mut send_request) = h3::client::new(quinn_conn).await?;
117118

118119
let drive = async move {
119-
future::poll_fn(|cx| driver.poll_close(cx)).await?;
120-
Ok::<(), Box<dyn std::error::Error>>(())
120+
return Err::<(), ConnectionError>(future::poll_fn(|cx| driver.poll_close(cx)).await);
121121
};
122122

123123
// In the following block, we want to take ownership of `send_request`:
@@ -129,7 +129,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
129129
let request = async move {
130130
info!("sending request ...");
131131

132-
let req = http::Request::builder().uri(uri).body(())?;
132+
let req = http::Request::builder().uri(uri).body(()).unwrap();
133133

134134
// sending request results in a bidirectional stream,
135135
// which is also used for receiving response
@@ -149,16 +149,31 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
149149
// receiving potential response body
150150
while let Some(mut chunk) = stream.recv_data().await? {
151151
let mut out = tokio::io::stdout();
152-
out.write_all_buf(&mut chunk).await?;
153-
out.flush().await?;
152+
out.write_all_buf(&mut chunk).await.unwrap();
153+
out.flush().await.unwrap();
154154
}
155155

156-
Ok::<_, Box<dyn std::error::Error>>(())
156+
Ok::<_, StreamError>(())
157157
};
158158

159159
let (req_res, drive_res) = tokio::join!(request, drive);
160-
req_res?;
161-
drive_res?;
160+
161+
if let Err(err) = req_res {
162+
if err.is_h3_no_error() {
163+
info!("connection closed with H3_NO_ERROR");
164+
} else {
165+
error!("request failed: {:?}", err);
166+
}
167+
error!("request failed: {:?}", err);
168+
}
169+
if let Err(err) = drive_res {
170+
if err.is_h3_no_error() {
171+
info!("connection closed with H3_NO_ERROR");
172+
} else {
173+
error!("connection closed with error: {:?}", err);
174+
return Err(err.into());
175+
}
176+
}
162177

163178
// wait for the connection to be closed before exiting
164179
client_endpoint.wait_idle().await;

examples/server.rs

+12-17
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
use std::{net::SocketAddr, path::PathBuf, sync::Arc};
22

33
use bytes::{Bytes, BytesMut};
4-
use http::{Request, StatusCode};
4+
use http::StatusCode;
55
use rustls::pki_types::{CertificateDer, PrivateKeyDer};
66
use structopt::StructOpt;
77
use tokio::{fs::File, io::AsyncReadExt};
88
use tracing::{error, info, trace_span};
99

10-
use h3::{error::ErrorLevel, quic::BidiStream, server::RequestStream};
10+
use h3::server::RequestResolver;
1111
use h3_quinn::quinn::{self, crypto::rustls::QuicServerConfig};
1212

1313
#[derive(StructOpt, Debug)]
@@ -118,29 +118,23 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
118118

119119
loop {
120120
match h3_conn.accept().await {
121-
Ok(Some((req, stream))) => {
122-
info!("new request: {:#?}", req);
123-
121+
Ok(Some(resolver)) => {
124122
let root = root.clone();
125123

126124
tokio::spawn(async {
127-
if let Err(e) = handle_request(req, stream, root).await {
125+
if let Err(e) = handle_request(resolver, root).await {
128126
error!("handling request failed: {}", e);
129127
}
130128
});
131129
}
132-
133-
// indicating no more streams to be received
130+
// indicating that the remote sent a goaway frame
131+
// all requests have been processed
134132
Ok(None) => {
135133
break;
136134
}
137-
138135
Err(err) => {
139136
error!("error on accept {}", err);
140-
match err.get_error_level() {
141-
ErrorLevel::ConnectionError => break,
142-
ErrorLevel::StreamError => continue,
143-
}
137+
break;
144138
}
145139
}
146140
}
@@ -159,14 +153,15 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
159153
Ok(())
160154
}
161155

162-
async fn handle_request<T>(
163-
req: Request<()>,
164-
mut stream: RequestStream<T, Bytes>,
156+
async fn handle_request<C>(
157+
resolver: RequestResolver<C, Bytes>,
165158
serve_root: Arc<Option<PathBuf>>,
166159
) -> Result<(), Box<dyn std::error::Error>>
167160
where
168-
T: BidiStream<Bytes>,
161+
C: h3::quic::Connection<Bytes>,
169162
{
163+
let (req, mut stream) = resolver.resolve_request().await?;
164+
170165
let (status, to_serve) = match serve_root.as_deref() {
171166
None => (StatusCode::OK, None),
172167
Some(_) if req.uri().path().contains("..") => (StatusCode::NOT_FOUND, None),

0 commit comments

Comments
 (0)