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

fix(client): early server response shouldn't propagate NO_ERROR #3275

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 10 additions & 1 deletion src/body/incoming.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,16 @@ impl Body for Incoming {
ping.record_data(bytes.len());
return Poll::Ready(Some(Ok(Frame::data(bytes))));
}
Some(Err(e)) => return Poll::Ready(Some(Err(crate::Error::new_body(e)))),
Some(Err(e)) => {
return match e.reason() {
// These reasons should cause stop of body reading, but don't fail it.
// The same logic as for `Read for H2Upgraded` is applied here.
Copy link
Contributor Author

@DDtKey DDtKey Jul 23, 2023

Choose a reason for hiding this comment

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

I'm referring to this line here:

Some(Reason::NO_ERROR) | Some(Reason::CANCEL) => Ok(()),

And it makes sense in terms of protocol behavior for the reasons.

Both of them call the same operation (self.recv_stream.poll_data(cx)) and seems that logic should be the same. But we can make it more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, an additional example from another lang & lib where it was also solved by avoiding error if response has been read: square/okhttp#6295

Some(h2::Reason::NO_ERROR) | Some(h2::Reason::CANCEL) => {
Poll::Ready(None)
}
_ => Poll::Ready(Some(Err(crate::Error::new_body(e)))),
}
}
None => {
*data_done = true;
// fall through to trailers
Expand Down
2 changes: 1 addition & 1 deletion src/proto/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ pub(crate) enum BodyLength {
Unknown,
}

/// Status of when a Disaptcher future completes.
/// Status of when a Dispatcher future completes.
pub(crate) enum Dispatched {
/// Dispatcher completely shutdown connection.
Shutdown,
Expand Down
58 changes: 57 additions & 1 deletion tests/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1338,7 +1338,7 @@ mod conn {
use bytes::{Buf, Bytes};
use futures_channel::{mpsc, oneshot};
use futures_util::future::{self, poll_fn, FutureExt, TryFutureExt};
use http_body_util::{BodyExt, Empty, StreamBody};
use http_body_util::{BodyExt, Empty, Full, StreamBody};
use hyper::rt::Timer;
use tokio::io::{AsyncReadExt as _, AsyncWriteExt as _};
use tokio::net::{TcpListener as TkTcpListener, TcpStream};
Expand Down Expand Up @@ -2126,6 +2126,62 @@ mod conn {
.expect("client should be open");
}

#[tokio::test]
async fn http2_responds_before_consuming_request_body() {
// Test that a early-response from server works correctly (request body wasn't fully consumed).
// https://github.com/hyperium/hyper/issues/2872
use hyper::service::service_fn;

let _ = pretty_env_logger::try_init();

let (listener, addr) = setup_tk_test_server().await;

// Spawn an HTTP2 server that responds before reading the whole request body.
// It's normal case to decline the request due to headers or size of the body.
tokio::spawn(async move {
let sock = TokioIo::new(listener.accept().await.unwrap().0);
hyper::server::conn::http2::Builder::new(TokioExecutor)
.timer(TokioTimer)
.serve_connection(
sock,
service_fn(|_req| async move {
Ok::<_, hyper::Error>(Response::new(Full::new(Bytes::from(
"No bread for you!",
))))
}),
)
.await
.expect("serve_connection");
});

let io = tcp_connect(&addr).await.expect("tcp connect");
let (mut client, conn) = conn::http2::Builder::new(TokioExecutor)
.timer(TokioTimer)
.handshake(io)
.await
.expect("http handshake");

tokio::spawn(async move {
conn.await.expect("client conn shouldn't error");
});

// Use a channel to keep request stream open
let (_tx, recv) = mpsc::channel::<Result<Frame<Bytes>, Box<dyn Error + Send + Sync>>>(0);
let req = Request::post("/a").body(StreamBody::new(recv)).unwrap();
let resp = client.send_request(req).await.expect("send_request");
assert!(resp.status().is_success());

let mut body = String::new();
concat(resp.into_body())
.await
.unwrap()
.reader()
.read_to_string(&mut body)
.unwrap();

assert_eq!(&body, "No bread for you!");
}

#[tokio::test]
async fn h2_connect() {
let (listener, addr) = setup_tk_test_server().await;
Expand Down