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.

Sign up for GitHub

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

Jump to bottom

Condvar: implement wait_timeout for targets without threads #134389

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Condvar: implement wait_timeout for targets without threads #134389

wants to merge 1 commit into from
+4 −2

Conversation

surban
Copy link
Contributor

@surban surban commented Dec 16, 2024 •
edited
Loading

This always falls back to sleeping since there is no way to notify a condvar on a target without threads.

Even on a target that has no threads the following code is a legitimate use case:

use std::sync::{Condvar, Mutex};
use std::time::Duration;

fn main() {
    let cv = Condvar::new();
    let mutex = Mutex::new(());
    let mut guard = mutex.lock().unwrap();

    cv.notify_one();

    let res;
    (guard, res) = cv.wait_timeout(guard, Duration::from_secs(3)).unwrap();
    assert!(res.timed_out());
}

@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2024

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 16, 2024
@surban surban mentioned this pull request Dec 17, 2024
Open
@surban surban requested a review from Darksonn December 17, 2024 10:53
@tgross35
Copy link
Contributor

This seems reasonable to me but since this is new user-visible behavior, requesting libs-api to take a look.

@rustbot label +I-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 18, 2024
@surban
Implement Condvar::wait_timeout for targets without threads
45c7ddf 
This always falls back to sleeping since there is no way
to notify a condvar on a target without threads.
@surban surban force-pushed the condvar-no-threads branch from 3ccd3fe to 45c7ddf Compare December 18, 2024 10:33
@surban surban changed the title Condvar: proper implementation for target without threads Condvar: implement wait_timeout for targets without threads Dec 18, 2024
@surban
Copy link
Contributor Author

surban commented Dec 18, 2024

@tgross35

There was a misunderstanding about buffering in Condvars. Indeed the documentation states:

Calls to notify_one are not buffered in any way.
Calls to notify_all() are not buffered in any way.

Thus my previous approach using a counter makes no sense and indeed Condvar::wait should always panic since there is no way to receive a notification while waiting.

However, Condvar::wait_timeout should always wait for the specified timeout and then return with a timed out state.

I have modified the PR and description accordingly.

@surban surban requested a review from tgross35 December 18, 2024 10:43
@tgross35 tgross35 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Darksonn Darksonn Awaiting requested review from Darksonn

@tgross35 tgross35 Awaiting requested review from tgross35

Assignees

@tgross35 tgross35

Labels
I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@surban @rustbot @tgross35 @Darksonn