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

Add TimeoutReceiver to allow recv()ing with a timeout #13862

Closed
wants to merge 4 commits into from

Conversation

Seldaek
Copy link
Contributor

@Seldaek Seldaek commented Apr 30, 2014

Not sure if this is the best way to do this, or if there is already a way that I missed, but it comes in very handy when you want to avoid blocking forever.

@alexcrichton
Copy link
Member

I would have expected this to be done through selection with a timer channel itself, rather than encapsulating it in a TimeoutReceiver. I'm curious, did you have some use cases which weren't suitable for that?

@Seldaek
Copy link
Contributor Author

Seldaek commented May 1, 2014

Hmm ideally yes it should select either the timer channel or the other channel, like in Go you can select multiple and the first to send something will trigger that part. I have no idea how to achieve this in rust though so I resorted to this loop which seems kinda ugly and not very performant but does the job. If you have some pointers I am happy to try and improve this though.

@huonw
Copy link
Member

huonw commented May 1, 2014

I believe the select! macro might do what you want.

@Seldaek
Copy link
Contributor Author

Seldaek commented May 1, 2014

I updated the code to use the stuff from the select macro, this way it now uses 0% cpu while waiting as opposed to slamming one core fully like the previous commit ;)

Now regarding the usefulness of this PR, you could say that select!() is enough, but IMO it isn't. It still requires you to understand timers, think about using a timer to interrupt the other channel, and then find out about select!'s existence. This TimeoutReceiver should stand out a bit more in searches and it's possibly easier to grasp. Up to you though of course.

@alexcrichton
Copy link
Member

In general, we favor composability over specific use cases in libraries, one of the best examples of this being iterators. This seems like it falls into the bucket of a convenience function rather than serving a core purpose, so I would be inclined to have better documentation about timers and select.

Currently the select! macro and interface are pretty bad, and I would love to clean them up (especially making them safe), which may explain why the documentation around them is a bit sparse. I can imagine, however, that the select! macro itself will not change much over time.

So, all in all, I would be more in favor of documenting how to receive with a timeout, which would introduce both timers and selection, instead of adding a new primitive. While this primitive is useful for some cases, it unfortunately requires many allocations for each recv (creating an I/O object and channel) which is often a burdensome requirement on callers.

@Seldaek
Copy link
Contributor Author

Seldaek commented May 2, 2014

With this patch:

let (tx, rx) = channel();
let mut rx = TimeoutReceiver::new(rx, 10000);
match rx.recv() {
    Some(val) => println!("Received {}", val),
    None => println!("timed out, no message received in 10 seconds"),
}

Without:

let (tx, rx) = channel();
let timer = Timer::new().unwrap();
let timeout = timer.oneshot(10000);
select!(
    val = rx.recv() => println!("Received {}", val),
    () = timeout.recv() => println!("timed out, no message received in 10 seconds"),
)

On one hand I think it's a bit nicer with this TimeoutReceiver, it feels more rustic to me with the Option than with select!. But I agree it's not a massive difference, once you know how to do it :) So I guess I'll update the PR to just add a doc example then if that's ok, because I think that would have solved my problem as well.

Just for my knowledge's sake however @alexcrichton would you mind expanding on what you said with regard to my patch allocating too much? AFAIK it doesn't allocate more than my example above, and I don't see how it could do less per recv(), maybe using a periodic timer vs a oneshot one, but that's not exactly the same behavior.

@alexcrichton
Copy link
Member

Ah, sorry, by more allocate-y I meant in terms of continual receiving. The TimeoutReceiver in this PR will allocate a timer and a channal for each message received, whereas this code would in theory handle the same use case (roughly).

let (tx, rx) = channel();
let timer = Timer::new().unwrap();
let timeout = timer.period(10000);
loop {
    select! {
        val = rx.recv() => println!("Received {}", val),
        () = timeout.recv() => println!("timed out, no message received in 10 seconds"),
    }
}

(in this case no allocations are done on the inner loop).

That seems like a good idea to add a new example. I think at the top of std::comm may be the best location?

@Seldaek
Copy link
Contributor Author

Seldaek commented May 3, 2014

I will send another PR with an example yes, but your code there is not really correct in that if you receive a message after 5seconds, the next message will only have 5seconds before a timeout occurs, since it doesn't seem possible to "reset" a periodic timer. With my version it really was 10seconds per message. This usually matters quite a lot IMO since if you build in a timeout you probably want to abort when it fires.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with the example we talked about!

@Seldaek Seldaek deleted the patch-3 branch June 13, 2014 22:49
bors added a commit that referenced this pull request Jun 16, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 9, 2025
…ang#13863)

Fixes rust-lang#13862

`missing_headers::check` is sometimes called from outside of a body
(specifically, from `check_attributes`, where the LateContext's ParamEnv
is not yet properly initialized for that item). Using that empty
ParamEnv for trait solving things from within the body can then lead to
various ICEs, like the linked issue where we have a const generic
parameter `DMA_INST` without a `ConstArgHasType` bound in the ParamEnv
so the const parameter has no type, which is normally not supposed to
happen.

We have the item's DefId so we can just get its ParamEnv/TypingEnv from
there, and using that one for trait solving should be safe.

changelog: none
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.

3 participants