std: sync: Implement recv_timeout() #33748
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
I would assume it requires an RFC before merging, but having an implementation already tends to help the RFC process along. /cc @rust-lang/libs |
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` |
There was a problem hiding this comment.
Could this be tagged no_run? Would be unfortunate to have a test that's basically just sleep(1) :(
There was a problem hiding this comment.
I changed it to 100ms, but let me know if that's not enough so I mark it no-run.
There was a problem hiding this comment.
I'd prefer no-run. Slow tests add up.
|
Thanks for the PR @emilio! Unfortunately I think that the implementations for the stream and shared implementations may end up being much more complicated. There's a nontrivial protocol where a receiver registers itself as available to receive data, but if a timeout happens the receiver is not currently "unregistered". The logic here is basically in the As for the instability of these methods, I think it's fine to land these without an RFC. The new methods clearly follow existing conventions and they're pretty minor additions, so I'd at least think they can go through the normal flesh-out-the-bugs then FCP cycle. |
|
On Fri, May 20, 2016 at 09:42:59AM -0700, Alex Crichton wrote:
Yeah, totally, I expected it to be way harder than what I did there (my Basically I just wanted to make sure there was any intention to get it
Sounds good, thanks!
|
|
A possible alternative approach would be to provide a receiver select!(
x = rx.recv() => Some(x),
_ = later(delay) => None,
)All the deregistration logic is already in select, which might simplify matters. This is essentially half of Concurrent ML (cc @larsbergstrom), the bit that's missing is the ability to add custom handlers to receivers that are executed on deregistration. |
|
IIRC this was the approach that got backed out (old_io::Timer maybe?). On Fri, May 20, 2016 at 10:36:58AM -0700, Alan Jeffrey wrote:
|
|
Ok, I did more work on this today, and I think I got all the invariants right. |
| @@ -216,7 +216,7 @@ impl<T> Packet<T> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub fn recv(&mut self) -> Result<T, Failure> { | |||
| pub fn recv_impl(&mut self, deadline: Option<Instant>) -> Result<T, Failure> { | |||
There was a problem hiding this comment.
Perhaps this could just be the only exported function of each of these modules? e.g.:
pub fn recv(&mut self, deadline: Option<Instant>) -> ...
|
Slick idea using I think that the logic here all looks sound to me given that we're using that, so just a few minor nits here and there and otherwise looks great to me. |
|
And since this is likely close to landing now, cc @rust-lang/libs. Any thoughts on landing this unstable vs requiring an RFC? |
| self.recv_max_until(Instant::now() + timeout) | ||
| } | ||
|
|
||
| fn recv_max_until(&self, deadline: Instant) -> Result<T, RecvTimeoutError> { |
There was a problem hiding this comment.
At some point we might want to make this part of the public API, to avoid users jumping back and forth between Instant and Duration.
|
Yay, this looks like it'll land! |
|
Hm thinking some more, I wonder if to be performant we want to avoid the usage of |
alexcrichton
added
the
T-libs-api
|
Hm thinking some more, I wonder if to be performant we want to avoid
the usage of `Instant::now`? In the fast path where you don't block it
may be advantageous to avoid calling that. Could you whip up a quick
benchmark to see the overhead of `recv()` on a full channel vs
`recv_timeout(Duration::new(1, 0))` on a full channel?
Yes, I can do it tomorrow if you really want it, though doing an
optimistic `try_recv()` at the beginning (if that's what you're
considering) would affect the accuracy of the timeout in the empty case.
Anyway, if it has a noticeable performance impact, that'd be a trade-off
we have to decide about.
Will update tomorrow with the benchmark data.
@asajeffrey: I agree it could be worth to have that method in the public
API, though I'm not sure what could be a good name for it.
That's something that had not being proposed beforehand though, so if
the discussion about that would involve blocking the landing of
`recv_timeout()`, I'd rather leave it as a follow-up to discuss
properly.
|
|
My half-baked opinions about relative-time timeouts (parameterized by a The API that end-users usually want is relative-time, e.g. "download this file, but give up after ten minutes", which is why APIs tend to be relative-time. When you're implementing a relative-time API, you quite often want to do it using an absolute-time API, since you often have to use a while loop: fn blah_timeout(&self, timeout: Duration) {
self.blah_until(Instant::now() + timeout);
}
fn blah_until(&self, deadline: Instant) {
loop {
let done = self.lower_level_thing_until(deadline);
if done { break; }
...
}
}There's a lot less jumping back and forth between Annoyingly, |
|
Yeah relative vs absolute is something we haven't explored much in libstd just yet (mainly b/c Instant was only recently stabilized). Absolute times are also tricky with things like clock drift, but for now we probably want to stick to the convention in the rest of libstd of just taking a Duration. @emilio yeah it's true that there'd be some timing difference to attempt a receive first, but in general this shouldn't be used for precise timing and just "don't block for too long", in which either case should suffice |
|
@alexcrichton So here are the results for my benchmark, let me know if you can think of another test or anything that could be valuable: So yeah, definitely there's some overhead. I don't think there's too much, but if you want me to add an optimistic try_recv, I'll just do it :) |
|
Ah yeah that'd do it! With a fast-path recv being almost 2x slower, perhaps we can try with a |
|
@alexcrichton: done and rebased! :-) |
| #[test] | ||
| fn recv_timeout_upgrade() { | ||
| let (tx, rx) = channel::<()>(); | ||
| let timeout = Duration::from_millis(1000); |
There was a problem hiding this comment.
Could these be more like 1ms to prevent tests from blocking?
| #[test] | ||
| fn stress_recv_timeout_two_threads() { | ||
| let (tx, rx) = channel(); | ||
| let stress = stress_factor(); |
There was a problem hiding this comment.
This stress_factor() is actually almost always 1 nowadays, so perhaps a more explicit count could be used?
|
Ok, r=me modulo a few minor nits (you may also want to squash some commits). I'll leave this tagged with T-libs to ensure this comes up during triage, however, to ensure we get a chance to discuss the API here. Implementation wise everything looks good to me, thanks again @emilio! |
|
Ok, done! I left a 10ms timeout in one of the test IIRC, because without it the test would be absolutely pointless. I can re-edit it though. Should I open an issue to fill-in the issue number instead of 0000? Thanks for the reviews @alexcrichton! :) |
|
Oh right yeah thanks for the reminder, I've opened #34029 to suffice for this. |
|
Issue number updated :) |
|
📌 Commit e95f9c9 has been approved by |
|
⌛ Testing commit e95f9c9 with merge 908e402... |
|
💔 Test failed - auto-win-gnu-32-opt-rustbuild |
|
Heh, missed the type param in the docs example, just changed re-r? @alexcrichton |
|
Looks like the travis error may be legit: |
|
Yup, sorry, first rust patch and I don't know of any make target a la Should be fixed. On Wed, Jun 22, 2016 at 10:55:55AM -0700, Alex Crichton wrote:
|
|
@bors: r+ |
|
📌 Commit b94b158 has been approved by |
|
⌛ Testing commit b94b158 with merge 6dcc2c1... |
std: sync: Implement recv_timeout() This is an attempt to implement rust-lang/rfcs#962. I'm not sure about if a change like this would require an rfc or something like that, and this surely needs a lot more testing, but I wanted to take some eyes on it before following. cc @metajack @asajeffrey servo/servo#11279 servo/servo#11283 r? @aturon
|
Finally! Thanks for all your patience @alexcrichton :-) |
This is an attempt to implement rust-lang/rfcs#962.
I'm not sure about if a change like this would require an rfc or something like
that, and this surely needs a lot more testing, but I wanted to take some eyes
on it before following.
cc @metajack @asajeffrey servo/servo#11279 servo/servo#11283
r? @aturon