Use better types for all timeout arguments

[Susurrus]

It looks like there may be other work on this area from perusing the existing issues and PRs, so maybe this is covered by that work already, but I wanted to use a Duration (or equivalent) for poll::poll(). Looks like there are a number of functions that could use this type treatment:

• poll::poll()
• event::kevent() (though this looks like a backcompat issue)
• epoll::epoll_wait()

I'm not certain if there are rationales for why this hasn't been done already since it looks like some functions have been converted to use TimeSpec/TimeVal already.

[kamalmarhubi]

I think part of the rationale was wanting to support old versions of Rust. Duration was only added in 1.3.0, and for a long time we supported back to 1.1.0. I think we now support ~1.7.0 so this could be revisited. @nix-rust/nix-maintainers thoughts?

[arcnmx]

If the type is stable in all supported Rust versions, definite +1 to using proper time/duration types as arguments.

[posborne]

Yeah, no objection from me.

[Susurrus]

This brings up an interesting point, both for users and developers, that the minimum supported rust version isn't explicitly stated anywhere, though your CI only tests Rust 1.7. Can this be added to the readme?

[posborne]

Can this be added to the readme?

That seems reasonable. I do believe we have tried to make a note of any changes to supported version in the changelog.

[kamalmarhubi]

1.7 is the current minimum, I believe.

Re taking Duration, I'd suggest taking Into<Duration> or perhaps Into<TimeVal> / Into<TimeSpec> as appropriate for the function.

[Susurrus]

Why not just use Duration everywhere? #511 shows that if helper methods are desired they can just be implemented for Duration instead of introducing a new type. Or am I missing something and it's inappropriate or unergonomic in some cases?

[Susurrus]

@posborne @kamalmarhubi Any comments on this?

[posborne]

I agree that taking Duration would be more ergonomic and would be in favor of taking Into<Duration> (which will work just fine with a Duration due to reflexivity). The only minor downside would be that we might need to do conversions more frequently if a syscall is made frequently (but even then the conversion overhead is approximately nothing compared to the syscall in most cases).

[Susurrus]

Would Rust be able to optimize across Into<Duration>? So we can have keep the nix-specific TimeVal and it contains the most common representation and if the Duration is converted back before use within a function, then Rust could optimize that out and no conversion would be done if you do back-to-back calls?

[Susurrus]

Regarding my comment above, it doesn't appear that Rust can reach through multiple From types. It's likely we'll want to have From implementations between several types. The types used for relative time currently is:

• poll::poll() - uint of ms
• event::kevent() - timespec
• sys::epoll::epoll_wait() - int of ms
• select::select() - timeval
• sys::aio_suspend() - timespec
• sys::Dqblk::{block_time_limit, set_block_time_limit, inode_time_limit, set_inode_time_limit} - implied to be time_t or target-specific type holding seconds
• unistd::sleep() - uint seconds

[Susurrus]

Some more notes:

• sys::epoll::epoll_wait() uses an int to store ms units, but then uses -1 and 0 as special values.
• select() actually modifies the timeout argument.
• pselect() uses a const timespec *

[Susurrus]

I think this discussion should only attempt to address the TimeSpec and TimeVal types here. My current proposal is change functions that take TimeVal or TimeSpec to instead take Into<TimeVal> or Into<TimeSpec> where possible (select() uses it as an output value, so it couldn't with this). Then we can also implement From<Duration> for both of these types to facilitate passing Duration types directly into these functions. I'd therefore also suggest that we remove all arithmetic operations on both of these types in favor of doing that through the Duration type instead. It should be fairly uncommon to do arithmetic from these types anyways, especially if users are using Durations as the backing datatype and a conversion occurring to pass to the function. I think I know what this will look like so I'll write up an implementation and we'll see where we get. This will likely supplant #511.

[Susurrus]

I've started implementing this, and I think the easiest solution here is to replicate the Duration API for both TimeSpec and TimeVal. There are no examples that really demonstrate how From<Duration> is useful, but I'd expect we'll be asked for it at some point, so I figure it's worth implementing now.

[Thomasdezeeuw]

@Susurrus any progress here? I could really use this and I'm willing to lent a hand to implement this.

[Susurrus]

My progress is what you see tracked here; I started exploring this, but didn't go very far because I don't have a specific use case for it and our examples are trivial enough, so designing a better API here is mostly guesswork for me. If you have specific use case and can iterate on a nice API, I'd really appreciate you driving this issue. I think my last comments are the direction this should go, but I don't have much code that requires this kind of API so my testing abilities are limited.

[Thomasdezeeuw]

I currently need nix::sys::event::kevent(_ts). It would be nice if that could accept an Option<Duration>, rather then usize or Option<timespec>. In the future I'll likely need epoll_wait as well.

Should I create a PR that changes that? I know more function use timespec, but it will be a start at least.

[Susurrus]

What I'd like to have is a zero-cost implementation for using the same datatype across different functions. So if you have a timespec already, there is no-cost in passing it to any of these functions that take a timespec already. So I'd much rather function take an Into<Timespec> and we can implement that for Duration. I think a PR that does that for kevent_ts(), aio_suspend, and pselect() would be a great start!

[Thomasdezeeuw]

Created an intial PR in #838.