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

feat(h1): add h1_header_read_timeout #2661

Closed
wants to merge 1 commit into from

Conversation

silence-coding
Copy link
Contributor

The original MR cannot be opened. #2655

fn poll_read(&mut self, cx: &mut task::Context<'_>) -> Poll<crate::Result<()>> {
loop {
if self.is_closing {
return Poll::Ready(Ok(()));
} else if self.conn.can_read_head() {
ready!(self.poll_read_head(cx))?;
Copy link
Contributor

@paolobarbolini paolobarbolini Oct 14, 2021

Choose a reason for hiding this comment

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

I've been testing your changes for a few days now and it seems like this branch gets called again as soon as the HTTP/1.1 response is over, so the timer starts when the previous request is over and not when the new one begins, causing a lot of wrongful timeouts under the right conditions

Copy link

@silence-code silence-code Oct 16, 2021

Choose a reason for hiding this comment

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

Thank you very much, if I move "check_header_read_timeout" to the previous line of "ready!(self.conn.poll_read_head(cx))", can it be resolved?

The poll_read_head method of dispatch may not only read the request header, but the poll_read_head method of conn should be

// dispatch is ready for a message, try to read one

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. I've had very little time to investigate it, and I'm not familiar with hyper internals. I may have more time to spend on it in a week or two.

Copy link
Contributor Author

@silence-coding silence-coding Oct 18, 2021

Choose a reason for hiding this comment

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

I constructed the scenario you tested, and the same problem is that the branch is called again after the HTTP/1.1 response ends. I have read the code several times and still have no idea. Do you have any other solution? Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Server:

#[tokio::main]
async fn main() {
    let subscriber = FmtSubscriber::builder()
        .with_max_level(tracing::Level::WARN)
        .finish();
    tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed");
    let addr = "0.0.0.0:8888".parse().unwrap();
    let _ = hyper::server::Server::bind(&addr)
        .h1_header_read_timeout(Duration::from_secs(10))
        .serve(hyper::service::make_service_fn(move |_| async move {
            let handle_fn = hyper::service::service_fn(move |req| async move {
                let uri = req.uri().to_string();
                warn!("{} {:?}", uri, req.version());
                let mut response = Response::new(Body::from("sdd"));
                hyper::Result::Ok(response)
            });
            hyper::Result::Ok(handle_fn)
        }))
        .await;
}

Client:

pub fn test_slow_http() -> io::Result<()> {
    let mut stream = TcpStream::connect("127.0.0.1:8888")?;
    send_http(&mut stream, 3)?;
    // send_http(&mut stream, 3)?;
    // send_http(&mut stream, 4)?;
    Ok(())
}

fn send_http(stream: &mut TcpStream, time: u64) -> io::Result<()> {
    stream.write_all(b"GET / HTTP/1.1\r\n")?;
    sleep(Duration::from_secs(time));
    stream.write_all(b"Host: 127.0.0.1:8888\r\n")?;
    sleep(Duration::from_secs(time));
    stream.write_all(b"Host1: 127.0.0.1:8888\r\n")?;
    sleep(Duration::from_secs(time));
    stream.write_all(b"Host2: 127.0.0.1:8888\r\n")?;
    stream.write_all(b"\r\n")?;
    stream.write_all(b"\r\n")?;
    sleep(Duration::from_secs(1));
    let mut buffer = [0; 1024];
    let n = stream.read(&mut buffer)?;
    println!("{:?}", String::from_utf8(buffer[0..n].to_vec()));
    sleep(Duration::from_secs(1));
    Ok(())
}

Copy link
Contributor Author

@silence-coding silence-coding Oct 25, 2021

Choose a reason for hiding this comment

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

@paolobarbolini I use a bad method (HeaderReadTimeout) to avoid the problem of incorrectly creating header_read_timeout_fut. Can you help me see if there are any other problems with this method? Or there's a better realization. Thanks

Copy link
Contributor

@paolobarbolini paolobarbolini Oct 25, 2021

Choose a reason for hiding this comment

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

That is_first_pending feels more like a workaround to me. I'll try coming up with something that fixes the issue at its core.

Copy link

@silence-code silence-code Oct 28, 2021

Choose a reason for hiding this comment

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

Yes, this is just a solution to solve the symptoms rather than the root causes.

@silence-coding silence-coding changed the title feat(h1): add h1_header_read_timeout [Bad MR] [Need Help] feat(h1): add h1_header_read_timeout Oct 18, 2021
@silence-coding silence-coding changed the title [Bad MR] [Need Help] feat(h1): add h1_header_read_timeout feat(h1): add h1_header_read_timeout Oct 25, 2021
@paolobarbolini
Copy link
Contributor

I've opened #2675 with an attempt to properly fix it

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.

3 participants