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

Amend RFC 517: Add material on deadlines #577

Closed
wants to merge 1 commit into from

Conversation

aturon
Copy link
Member

@aturon aturon commented Jan 13, 2015

The IO reform RFC is being split into several semi-independent pieces, posted as PRs like this one.

This RFC amendment touches on deadlines.

Rendered

> To be added in a follow-up PR.
Most blocking system operations can take a timeout or a deadline
(depending on the platform) for completion, and it's important that
Rusts IO APIs offer the same capability. This poses a bit of a
Copy link
Member

Choose a reason for hiding this comment

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

Rust's

@bill-myers
Copy link

I think there should also be a "with_timeout" functionality in addition to "with_deadline" that would apply the timeout to each operation.

For instance, it might be desirable to require that each network read not take more than 10 seconds (because otherwise we assume the network is nonfunctional), but not bound the overall time of a set of network operations.

Perhaps these could be generalized into a "with_timeout_closure" version that takes a closure that returns the timeout for every operation, on top of which both per-operation timeouts, overall deadlines and other schemes (such as a simple form of on-demand cancellation) could be implemented.

Deadlined { deadline: deadline, inner: inner }
}

pub fn deadline(&self) -> Deadline {
Copy link
Member

Choose a reason for hiding this comment

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

Ahh, implement IntoDeadline instead (for &Deadlined)?

@grogers0
Copy link

One problem with a deadline is that it is inherently tied to absolute real time. When we are talking about network timeouts, we rarely care about the absolute time an operation completes by, but instead the relative duration to stop after. It's the difference between the CLOCK_REALTIME and CLOCK_MONOTONIC. When you need one or the other the distinction matters.

Using relative timeouts on an individual operation basis may work for some use cases, but it doesn't compose well. Say I provide a library that wraps message framing on top of a tcp stream and you set the timeout on the next readmsg operation. I would expect it to time out after that duration, even though the implementation may make several calls to the underlying tcp read, rather than passing that timeout through to each read call, which could dramatically overrun the time budget.

@thestinger
Copy link

The proposals for cancellation, deadlines and shutting down connections are all laughably bad. A sane input / output library would be composable with timers and channels and wouldn't need these over-engineered hacks. I've already pointed out what it should look like countless times, so I'm not even going to bother getting into it again.

The core developers have no clue what they're doing when it comes to input / output but they have such a high opinion of themselves that they've totally ignored the community input and keep pushing through these moronic plans.

@thestinger
Copy link

Claiming that the community feedback was addressed in these proposals is a joke. It's the same crap as before, but split up into a whole bunch of awful proposals with the previous community feedback silenced.

@kjpgit
Copy link

kjpgit commented Jan 27, 2015

I wouldn't expect a v 1.0 IO library in a brand new language to be a full async-reactor. As long as I can take a tcpstream and every time I read/write/use it, set a deadline of N seconds, I'm ok. Looks like the Deadlined trait does that.

I just hope it only calls fcntl once to put it into non blocking mode, though, and not on each read/write.

@kjpgit
Copy link

kjpgit commented Jan 27, 2015

For the record, the python way of calling socket.settimeout() once and not having to mess with it again is pretty darn convenient. So it might be nice if there's a default that one can still override if needed.

@kjpgit
Copy link

kjpgit commented Jan 27, 2015

Let's step back and look at the use case. A personal use case of mine is, 'make a socket and as long as some data has moved in the last 30 seconds, it's alive'. And I might also wrap it in an OpenSSL stream. That's trivial in python: settimeout(), it auto renews. So I agree with @bill-myers that a with_timeout would be nice.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 27, 2015

Rust should NOT be using deadlines for it's built-in IO:

  • It's inconsistent with all other low-level IO libraries, including POSIX, WinApi, STL and BOOST, all of which use timeouts exclusively.
  • The argument in the RFC for deadlines over timeouts is flat-out wrong: the clock must be checked strictly more times when using deadlines over timeouts. (Deadlines still require checking the clock on every part of a multipart operation, in order to convert deadlines into the timeouts expected by the OS). With timeouts you don't ever need to check the clock for atomic operations.
  • The monotonic/realtime difference explained by grogers0
  • It condemns rust to use deadlines for other things too: presumably "std::sync" will get methods to allow waits which timeout too, should I have to access the clock twice to lock a mutex? (Once to convert to an absolute time, once to convert back again, because nobody else uses absolute time)

@carllerche
Copy link
Member

I'm a 👎 on this RFC as well. Many of the reasons have already been addressed. I agree w/ @Diggsey and others. Real time (vs. monotonic) should never be used w/ timeouts.

For context, most of what I am talking about is regarding the network APIs.

I can't think of a situation where this API would be used in a well polished app. It seems like this is only here for cases where one is hacking up a solution and just wants to grab something that "works" from std (vs. doing it "the right way").

An additional concern that I have that has not been mentioned yet is that in order to implement this feature, the underlying socket will need to be switched to non-blocking mode. Yet, the containing struct is "sync" and the user would assume assume that the type is a "sync io" type. The reason this matters is interopt with crates that would use the FD directly. There will most likely be some unexpected behavior / additional complexity that would fall out of it. There are most likely ways to address these concerns, but the additional complexity doesn't seem worth it for this API in the first place.

@wycats
Copy link
Contributor

wycats commented Jan 27, 2015

I can't think of a situation where this API would be used in a well polished app. It seems like this is only here for cases where one is hacking up a solution and just wants to grab something that "works" from std (vs. doing it "the right way").

One example would be a situation where someone is trying to support the full range of platforms that Rust supports, as opposed to a specific platform where "the right way" can easily be achieved by talking to platform APIs directly.

@retep998
Copy link
Member

Every major platform uses timeouts instead of deadlines though. There's really no reason to have this overhead in the standard library itself. Just expose timeout functionality in a way that closely matches the major platforms. If people want these high overhead abstractions they can just use an external crate, especially since Cargo is such an easy system to use.
The whole point of this IO reform is to make things match the common functionality of the major platforms, not introduce brand new untested abstractions that almost no other language or library uses.

@carllerche
Copy link
Member

@wycats Could you provide an example where socket timeouts aren't available?

Also, there are better ways of dealing w/ "platform compatibility" by abstracting at a higher level than introducing an API that should probably not be used in a well built app.

@wycats
Copy link
Contributor

wycats commented Jan 27, 2015

@wycats Could you provide an example where socket timeouts aren't available?

It sounds like you're arguing that you would prefer literally no timeout support at all to a deadline API. Is that correct?

@carllerche
Copy link
Member

I am recommending (as others have in this thread) providing socket level timeouts as is doable on posix & windows. I am not aware of any platform for which is is not doable, so I am asking for an example.

@wycats
Copy link
Contributor

wycats commented Jan 27, 2015

I am recommending (as others have in this thread) providing socket level timeouts as is doable on posix & windows

I'm trying to understand your hierarchy of preferences. Would you prefer no support for any kind of "timeouts" to a deadline API?

@retep998
Copy link
Member

I would prefer no timeouts to having a deadline API, because it is much easier to add something later than it is to remove things.

@wycats
Copy link
Contributor

wycats commented Jan 27, 2015

The monotonic/realtime difference explained by grogers0

One problem with a deadline is that it is inherently tied to absolute real time

I think the ambiguity about realtime vs. monotonic in this RFC is a bug. It seems like you think a deadline couldn't be defined to use monotonic time. Can you say more about why that is?

@thestinger
Copy link

@wycats:

One example would be a situation where someone is trying to support the full range of platforms that Rust supports, as opposed to a specific platform where "the right way" can easily be achieved by talking to platform APIs directly.

In reality, there aren't portability problems with doing input / output sanely. This is just the usual FUD that people have come to expect from the core developers. It's funny to see these proposals full of not-invented-here hacks without platform support being portrayed as a compromise due to "portability".

@grogers0
Copy link

Rereading the proposal it is explicitly silent on the real/monotonic clock thing. I don't see any technical reason you couldn't have an API that uses monotonic time as a deadline. It'd be kinda weird though. Using timeout conveniently sidesteps that issue.

I wouldn't worry about checking the clock one extra time with either pattern, there will be different use cases that favor one or the other method. Pick whichever is more natural, in general that means do as others have already done. Maybe it's just me, but having each blocking call take a timeout isn't that terrible.

@nagisa
Copy link
Member

nagisa commented Jan 28, 2015

@grogers0 This RFC provides a very clear motivation for introducing such abstraction over time outs: composite methods. How would you even set the time out for Tee<Socket>::write() if each such call actually involves at least two Socket::writes itself? Why would Tee have a way to set time out at all if it usually composes synchronous FS API, which generally doesn’t provide any time out functionality directly?


Deadlines look like a fine abstraction over time outs to me, but I don’t see any/enough reason to include this before the 1.0, especially because we already have methods to set timeout on sockets, where such thing matters the most.

@arielb1
Copy link
Contributor

arielb1 commented Jan 28, 2015

@thestinger

It certainly looks to me like this io library is planned to be something in the style of Python's/C's standard io – reasonably complete, but synchronous and slow. I guess that a high-performance library would have to be implemented on Cargo.

@carllerche
Copy link
Member

@arielb1 As far as I can tell, std::io's net lib is going to be essentially a 0 cost abstraction over the OS's IO apis. Even if this deadline API were to land (and I don't think it will), it would be an opt-in only API, which would mean that if you didn't use it, you wouldn't pay the overhead.

The idea that synchronous IO is "slow" compared to async IO is flat out wrong. There are many cases where a sync IO api would be faster than an async implementation. There are times to use async IO and there are times to use sync IO. Use the correct strategy for the use case at hand.

I believe that Rust's std::io implementation will end up being a very solid sync IO implementation thanks to these RFCs and community involvement.

@thestinger
Copy link

@arielb1: Not just synchronous only and slow (two separate issues) but with poor support for composition and verbose APIs.

@thestinger
Copy link

@carllerche: Using abstractions that aren't directed supported by the operating system like this deadline stuff ends up adding up. It's already slow, even though it's not providing any fancy features:

https://kjpgit.github.io/posts/testing-rust-io.html

Things don't have to be this way, but they are and will continue to be until the attitude changes.

@aturon
Copy link
Member Author

aturon commented Jan 29, 2015

Thanks everyone for the feedback so far, and sorry to be slow to respond. It's been a tough week.

To clarify a few points:

  • The intent was to use monotonic time for deadlines, but this was not made clear in the text.
  • The cancellation and splitting part of the old RFC has not been reinstated as part of the new split, but I realize that wasn't very clearly communicated (and there are still stub sections).

That said, after discussion here and elsewhere, here's the more conservative direction I'd like to pursue:

  • Do not support cancellation/shutdown.
  • Do not implement Clone or use Arc anywhere within Rust objects wrapping file descriptors/handles. Instead, expose a clone-like method returning an io::Result that uses something like dup2 under the hood.
  • Consequently, use sockopts to set timeouts. Provide a Rust API based directly on (relative) timeouts, matching this API. It's not yet clear whether this API is best provided through a wrapper object as proposed here (which has some composability/bug catching benefits, but some potential drawbacks as well) or something more like today's set_timeout.

This plan should make at least the fs and net portions of Rust's IO extremely thin, cross-platform layers over system APIs. In particular, Rust IO objects basically correspond directly to fds/handles. This is not to say that we would never provide further conveniences on top (e.g. we could eventually offer deadlines as well as timeouts for convenience), but this seems like the right starting place given the overall goals of the RFC.

I believe that this plan represents the rough consensus of feedback so far, as well as my own current feelings. I'm going to close this sub-RFC, since it will need substantial revision; I'll open a new one soon. However, please continue to add comments if you have more feedback or suggestions about the above!

@aturon aturon closed this Jan 29, 2015
@carllerche
Copy link
Member

@aturon I'm 👍 for this strategy.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 29, 2015

@aturon Great! I think there will be some healthy competition on crates.io for good high level abstractions, and if one stands out it can always be pulled in later, but in the mean-time we will at least have a familiar, easy to reason about API that is both stable and fast.

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.