IO timeout usability improvements

[carllerche]

The current timeout API on the various stream implementations behaves as a deadline vs. a timeout setting. I propose the following.

• Rename set_timeout -> set_deadline. (as well as the read and write variants).
• Move set_deadline to a trait (for example: ReadDeadline and WriteDeadline)
• Provide TimedReader and TimedWriter that operate as follows:

/// Various network related helpers

use std::io::{Reader, Writer, IoResult};
use std::io::net::unix::UnixStream;
use util::Duration;

pub trait ReadDeadline {
    fn set_read_deadline(&mut self, timeout_ms: Option<u64>);
}

impl ReadDeadline for UnixStream {
    fn set_read_deadline(&mut self, timeout_ms: Option<u64>) {
        self.set_read_timeout(timeout_ms);
    }
}

pub trait WriteDeadline {
    fn set_write_deadline(&mut self, timeout_ms: Option<u64>);
}

impl WriteDeadline for UnixStream {
    fn set_write_deadline(&mut self, timeout_ms: Option<u64>) {
        self.set_write_timeout(timeout_ms);
    }
}

pub struct TimedReader<R> {
    reader: R,
    timeout_ms: u64
}

impl<R: Reader + ReadDeadline> TimedReader<R> {
    pub fn new(reader: R, timeout_ms: u64) -> TimedReader<R> {
        TimedReader {
            reader: reader,
            timeout_ms: timeout_ms
        }
    }
}

impl<R: Reader + ReadDeadline> Reader for TimedReader<R> {
    fn read(&mut self, buf: &mut [u8]) -> IoResult<uint> {
        self.reader.set_read_deadline(Some(self.timeout_ms));
        self.reader.read(buf)
    }
}

pub struct TimedWriter<W> {
    writer: W,
    timeout_ms: u64
}

impl<W: Writer + WriteDeadline> TimedWriter<W> {
    pub fn new(writer: W, timeout_ms: u64) -> TimedWriter<W> {
        TimedWriter {
            writer: writer,
            timeout_ms: timeout_ms
        }
    }
}

impl<W: Writer + WriteDeadline> Writer for TimedWriter<W> {
    fn write(&mut self, buf: &[u8]) -> IoResult<()> {
        self.writer.set_write_deadline(Some(self.timeout_ms));
        self.writer.write(buf)
    }
}

cc @alexcrichton

[tomaka]

Move set_deadline to a trait (for example: ReadDeadline and WriteDeadline)

And similarly a AcceptDeadline

[brson]

We also probably want these to not take integer milliseconds but some kind of time type.

[conradkleinespel]

Would love to see this implemented too. Anyway I can help?

Just for the sake of completeness, would be good to add support for this to std::io::net::tcp::TcpStream and std::io::net::udp::UdpSocket.

[nodakai]

(Sorry I posted my comment on a wrong tree before.) I did a quick search for candidates for updating to use Duration instead of number of milliseconds etc. How many of them can be updated to use Duration without adding more dependency? I really want nice and stable API on I/O timeout.

$ ack --ignore-dir llvm --ignore-dir test 'time.*u64' src/
src/libtime/lib.rs
71:        pub fn mach_absolute_time() -> u64;
145:        let ns_since_1601 = ((time.dwHighDateTime as u64 << 32) |
146:                             (time.dwLowDateTime  as u64 <<  0)) / 10;
175:pub fn precise_time_ns() -> u64 {
179:    fn os_precise_time_ns() -> u64 {
195:    fn os_precise_time_ns() -> u64 {
204:            time * TIMEBASE.numer as u64 / TIMEBASE.denom as u64
209:    fn os_precise_time_ns() -> u64 {

src/libcollections/vec.rs
2390:        b.bytes = (times * src_len) as u64;

src/librbml/io.rs
187:        b.bytes = (times * len) as u64;

src/libnative/io/file_windows.rs
216:    fn set_timeout(&mut self, _t: Option<u64>) {}
217:    fn set_read_timeout(&mut self, _t: Option<u64>) {}
218:    fn set_write_timeout(&mut self, _t: Option<u64>) {}
480:        created: stat.st_ctime as u64,
481:        modified: stat.st_mtime as u64,
482:        accessed: stat.st_atime as u64,
510:pub fn utime(p: &CString, atime: u64, mtime: u64) -> IoResult<()> {

src/libnative/io/pipe_windows.rs
270:    pub fn connect(addr: &CString, timeout: Option<u64>) -> IoResult<UnixStream> {
546:    fn set_timeout(&mut self, timeout: Option<u64>) {
551:    fn set_read_timeout(&mut self, timeout: Option<u64>) {
554:    fn set_write_timeout(&mut self, timeout: Option<u64>) {
736:    fn set_timeout(&mut self, timeout: Option<u64>) {

src/libnative/io/process.rs
127:    fn set_timeout(&mut self, timeout: Option<u64>) {

src/libnative/io/util.rs
57:pub fn ms_to_timeval(ms: u64) -> libc::timeval {
64:pub fn ms_to_timeval(ms: u64) -> libc::timeval {
104:                       timeout_ms: u64) -> IoResult<()> {
151:             timeout: u64) -> libc::c_int {
164:             timeout: u64) -> libc::c_int {

src/libnative/io/mod.rs
167:                   timeout: Option<u64>)
193:                    timeout: Option<u64>) -> IoResult<Box<rtio::RtioPipe + Send>> {
256:    fn fs_utime(&mut self, src: &CString, atime: u64,
257:                mtime: u64) -> IoResult<()> {

src/libnative/io/net.rs
239:                   timeout: Option<u64>) -> IoResult<TcpStream> {
380:    fn set_timeout(&mut self, timeout: Option<u64>) {
385:    fn set_read_timeout(&mut self, timeout: Option<u64>) {
388:    fn set_write_timeout(&mut self, timeout: Option<u64>) {
648:    fn set_timeout(&mut self, timeout: Option<u64>) {
866:    fn set_timeout(&mut self, timeout: Option<u64>) {
871:    fn set_read_timeout(&mut self, timeout: Option<u64>) {
874:    fn set_write_timeout(&mut self, timeout: Option<u64>) {

src/libnative/io/pipe_unix.rs
81:           timeout: Option<u64>) -> IoResult<Inner> {
126:                   timeout: Option<u64>) -> IoResult<UnixStream> {
196:    fn set_timeout(&mut self, timeout: Option<u64>) {
201:    fn set_read_timeout(&mut self, timeout: Option<u64>) {
204:    fn set_write_timeout(&mut self, timeout: Option<u64>) {
307:    fn set_timeout(&mut self, timeout: Option<u64>) {

src/libnative/io/file_unix.rs
183:    fn set_timeout(&mut self, _t: Option<u64>) {}
184:    fn set_read_timeout(&mut self, _t: Option<u64>) {}
185:    fn set_write_timeout(&mut self, _t: Option<u64>) {}
446:    fn mktime(secs: u64, nsecs: u64) -> u64 { secs * 1000 + nsecs / 1000000 }
462:        created: mktime(stat.st_ctime as u64, stat.st_ctime_nsec as u64),
463:        modified: mktime(stat.st_mtime as u64, stat.st_mtime_nsec as u64),
464:        accessed: mktime(stat.st_atime as u64, stat.st_atime_nsec as u64),
494:pub fn utime(p: &CString, atime: u64, mtime: u64) -> IoResult<()> {

src/libstd/io/fs.rs
805:pub fn change_file_times(path: &Path, atime: u64, mtime: u64) -> IoResult<()> {

src/libstd/io/net/udp.rs
186:    pub fn set_timeout(&mut self, timeout_ms: Option<u64>) {
194:    pub fn set_read_timeout(&mut self, timeout_ms: Option<u64>) {
202:    pub fn set_write_timeout(&mut self, timeout_ms: Option<u64>) {

src/libstd/io/net/tcp.rs
118:            io.tcp_connect(addr, Some(timeout.num_milliseconds() as u64)).map(TcpStream::new)
228:    pub fn set_timeout(&mut self, timeout_ms: Option<u64>) {
245:    pub fn set_read_timeout(&mut self, timeout_ms: Option<u64>) {
272:    pub fn set_write_timeout(&mut self, timeout_ms: Option<u64>) {
444:    pub fn set_timeout(&mut self, ms: Option<u64>) { self.obj.set_timeout(ms); }

src/libstd/io/net/unix.rs
77:            let s = io.unix_connect(&path.to_c_str(), Some(timeout.num_milliseconds() as u64));
109:    pub fn set_timeout(&mut self, timeout_ms: Option<u64>) {
117:    pub fn set_read_timeout(&mut self, timeout_ms: Option<u64>) {
125:    pub fn set_write_timeout(&mut self, timeout_ms: Option<u64>) {
212:    pub fn set_timeout(&mut self, timeout_ms: Option<u64>) {

src/libstd/io/process.rs
539:    pub fn set_timeout(&mut self, timeout_ms: Option<u64>) {

src/libstd/io/mem.rs
564:        b.bytes = (times * len) as u64;

src/librustrt/rtio.rs
192:                   timeout: Option<u64>) -> IoResult<Box<RtioTcpStream + Send>>;
200:                    timeout: Option<u64>) -> IoResult<Box<RtioPipe + Send>>;
224:    fn fs_utime(&mut self, src: &CString, atime: u64, mtime: u64) ->
248:    fn set_timeout(&mut self, timeout: Option<u64>);
264:    fn set_timeout(&mut self, timeout_ms: Option<u64>);
265:    fn set_read_timeout(&mut self, timeout_ms: Option<u64>);
266:    fn set_write_timeout(&mut self, timeout_ms: Option<u64>);
290:    fn set_timeout(&mut self, timeout_ms: Option<u64>);
291:    fn set_read_timeout(&mut self, timeout_ms: Option<u64>);
292:    fn set_write_timeout(&mut self, timeout_ms: Option<u64>);
318:    fn set_timeout(&mut self, timeout: Option<u64>);
328:    fn set_timeout(&mut self, timeout_ms: Option<u64>);
329:    fn set_read_timeout(&mut self, timeout_ms: Option<u64>);
330:    fn set_write_timeout(&mut self, timeout_ms: Option<u64>);
339:    fn set_timeout(&mut self, timeout: Option<u64>);

src/librustuv/uvio.rs
143:    fn tcp_connect(&mut self, addr: rtio::SocketAddr, timeout: Option<u64>)
259:    fn fs_utime(&mut self, path: &CString, atime: u64, mtime: u64)
293:    fn unix_connect(&mut self, path: &CString, timeout: Option<u64>)

src/librustuv/timeout.rs
112:    pub fn set_timeout(&mut self, ms: Option<u64>,
235:        mut self, obj: T, timeout: Option<u64>, io: &mut UvIoFactory,

src/librustuv/timer.rs
53:    pub fn start(&mut self, f: uvll::uv_timer_cb, msecs: u64, period: u64) {

src/librustuv/file.rs
248:    pub fn utime(loop_: &Loop, path: &CString, atime: u64, mtime: u64)
276:        fn to_msec(stat: uvll::uv_timespec_t) -> u64 {

src/librustuv/process.rs
276:    fn set_timeout(&mut self, timeout: Option<u64>) {

src/librustuv/net.rs
202:                   timeout: Option<u64>) -> Result<TcpWatcher, UvError> {
305:    fn set_timeout(&mut self, timeout: Option<u64>) {
310:    fn set_read_timeout(&mut self, ms: Option<u64>) {
322:    fn set_write_timeout(&mut self, ms: Option<u64>) {
477:    fn set_timeout(&mut self, ms: Option<u64>) {
793:    fn set_timeout(&mut self, timeout: Option<u64>) {
798:    fn set_read_timeout(&mut self, ms: Option<u64>) {
820:    fn set_write_timeout(&mut self, ms: Option<u64>) {

src/librustuv/pipe.rs
89:    pub fn connect(io: &mut UvIoFactory, name: &CString, timeout: Option<u64>)
175:    fn set_timeout(&mut self, timeout: Option<u64>) {
180:    fn set_read_timeout(&mut self, ms: Option<u64>) {
192:    fn set_write_timeout(&mut self, ms: Option<u64>) {
315:    fn set_timeout(&mut self, ms: Option<u64>) {

[aturon]

cc me

[murarth]

If no one else is working on this, I'll take a stab at it.

[thestinger]

It's a bad time to start working on this because nearly all of this stuff is going to get an overhaul. It would make the most sense to implement it as part of a major rewrite.

[conradkleinespel]

@thestinger Do you know if this overhaul is happening before v1? This issue has been open for a while now.

[aturon]

@conradkleinespel

The overhaul is part of the runtime removal project. The first step is complete: removing rustuv, rtio, and the runtime scheduling abstractions has all landed. We're now revisiting the APIs previously built on the runtime system, including thread-local storage, sync, the std::task module, and afterwards, std::io and std::os.

Please let me know if you would like to be involved in this effort!

[nodakai]

@aturon I'm very interested in this, partily because I frequently write networking code with timeout in C++ and am hoping Rust to become its superiror alternative.

But I'm concerned about removal of libtime from the stdlib. Pthread API with timeout are all based on absolute time which is only offered by libtime right now rather than relative time offered by std::duration. The design decision behind Pthread disagrees with Alex's sync-rs which is based on std::duration.

[aturon]

Thanks @nodakai. (And @conradkleinespel, I see you offered to help a little ways back).

I will be in touch ASAP about the plans here and ways that both of you might be able to help with design or implementation work.

As far as time goes, the reason libtime was moved out of the distro is that the API has gotten relatively little attention and likely needs substantial revision before moving into std. (Note that Duration has its own problems, as well.)

In the 1.0 beta timeframe, perhaps it is best to not stabilize methods taking a timeout (i.e., leave them as #[unstable]; we may want to keep our options open here and put more effort into the underlying time representation.

I will discuss this with the team and let you know what the best avenue forward looks to be.

[sfackler]

@aturon here's an option to help mitigate the absolute time issues: Define an AbsoluteTime trait (or whatever), and have the relevant methods abstract over that. The standard library can have an impl AbsoluteTime for uint. libtime can have an impl as well. Then when we end up with a quality time library and want to move it into libstd we can just impl the trait for that and be good to go.

[alexcrichton]

@nodakai remember though that pthreads isn't the end-all-be-all of API design. My in-progress sync-rs for example uses relative timeouts because Duration is already in the standard library and that's the only api Windows exposes. There are pros and cons either way, but neither is a silver bullet from what I've seen.

[aturon]

@alexcrichton What did you think about the point made in the linked posix docs about the rationale, though, namely that you can reliably build relative from absolute but not the other way around?

[alexcrichton]

Someone's gotta lose in this paradigm choice because posix implements it one way and Windows implements it another. I don't personally advocate for either side, but I prefer whatever can give us a clean API today, which is quite easily gained with a Duration today.

I'm not sure I quite understand where it says that a relative timeout can be implemented on top of an absolute timeout, however. If I would like to sleep for at most 5ms inside of wait, then it's semantically different to take the absolute time, add 5ms, and sleep until that time. There's no actual guarantee that you slept in the call to wait for 5ms (preemptions, extra code, etc before the call to wait).

[conradkleinespel]

@aturon Thanks for the progress update. I'm still interested in helping.

[nodakai]

@alexcrichton As you observed, the rationale section of Pthread isn't an absolute proof that joining the "absolute time clan" is the only viable option for us. But still, POSIX is much better designed than Win32 API and I believe it will benefit us to follow it closely.

For example we will never be free from spurious wakeups with a condition variable. Sleeping with timeout of 50 ms might end up with a spurious wakeup only after 10 ms. Then we would typically want to sleep for 40 ms again. That can only be done by recording the absolute time just before we issue timedwait(). We definitely need sort of absolute time anyways.

Personally I'm hesitant to follow Win32 timeout design which is so sloppy:

• http://msdn.microsoft.com/en-us/library/windows/desktop/ms687069.aspx#waitfunctionsandtime-outintervals

... If the time-out interval is less than the resolution of the system clock, the wait may time out in less than the specified length of time. If the time-out interval is greater than one tick but less than two, the wait can be anywhere between one and two ticks, and so on.

• http://msdn.microsoft.com/en-us/library/windows/desktop/ms687032.aspx

dwMilliseconds [in]

The time-out interval, in milliseconds...

Note The dwMilliseconds value does not include time spent in low-power states. For example, the timeout will not keep counting down while the computer is asleep.

[thestinger]

Rust should be able to do at least as well as C++11, which has a thread API heavily based on POSIX threads but extends it with convenience methods to handle the looping for spurious wakeups and more.

[thestinger]

The Windows condition variable API is broken and is not something that should be copied. An API using a relative time needs to return the elapsed time if it wakes up early.

[aturon]

I'm going to close this since it currently relates only to old_io, and the story for the new IO system will be fleshed out in a new RFC (which will likely be based on timeouts rather than deadlines.)