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

copy_with_timeout() may exceed its timeout #46

Closed
Shnatsel opened this issue Feb 12, 2021 · 1 comment · Fixed by #66
Closed

copy_with_timeout() may exceed its timeout #46

Shnatsel opened this issue Feb 12, 2021 · 1 comment · Fixed by #66
Labels
bug Something isn't working

Comments

@Shnatsel
Copy link

copy_with_timeout() as implemented in v0.7.2 may exceed its deadline:

http_req/src/request.rs

Lines 50 to 75 in eba0471

///Copies data from `reader` to `writer` until the `deadline` is reached.
///Returns how many bytes has been read.
pub fn copy_with_timeout<R, W>(reader: &mut R, writer: &mut W, deadline: Instant) -> io::Result<u64>
where
R: Read + ?Sized,
W: Write + ?Sized,
{
let mut buf = [0; BUF_SIZE];
let mut copied = 0;
let mut counter = Counter::new(TEST_FREQ);
loop {
let len = match reader.read(&mut buf) {
Ok(0) => return Ok(copied),
Ok(len) => len,
Err(ref e) if e.kind() == ErrorKind::Interrupted => continue,
Err(e) => return Err(e),
};
writer.write_all(&buf[..len])?;
copied += len as u64;
if counter.next().unwrap() && Instant::now() >= deadline {
return Ok(copied);
}
}
}

In here the timeout is only checked after the read operation is issued. However, a call to read() may block according to the documentation. If used on a reader that reads from a network socket, IIRC it will block waiting for data from the remote server until the underlying TCP timeout expires (5 minutes by default).

I believe this is a fundamental limitation of the approach; I am not aware of any ways to fix it without spawning an auxiliary thread or using an async runtime.

If a read timeout is desired, it should be configured on the socket explicitly instead.

If a timeout for the entire request is desired, it can be implemented either by spawning an auxiliary thread that interrupts the main thread (as done in attohttpc), or by setting the read timeout to the minimum of read timeout and the remaining time until the deadline for the entire request (as done in minreq).

@jayjamesjay jayjamesjay added bug Something isn't working help wanted Extra attention is needed labels Feb 17, 2021
@jayjamesjay jayjamesjay removed the help wanted Extra attention is needed label Jun 24, 2024
@jayjamesjay jayjamesjay linked a pull request Jun 27, 2024 that will close this issue
@jayjamesjay jayjamesjay mentioned this issue Jun 27, 2024
@jayjamesjay
Copy link
Owner

The issue was waiting for quite some time. v0.11.0 implements improved solution for handling timeout for the entire request.

The data is read from TCP stream on a separate thread and send over to main thread via mpsc::channel. The data is send in chunks.

The main thread is receiving the data and checking if deadline is not exceeded. It stops the operation when deadline is exceeded. In addition to that receiving is done using recv_timeout with timeout dynamically calculated based on time left before deadline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants