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 std::sync::mpsc::Receiver::recv_deadline() #45969

Merged
merged 3 commits into from Nov 29, 2017

Conversation

Projects
None yet
6 participants
@ia0
Contributor

ia0 commented Nov 13, 2017

Essentially renames recv_max_until to recv_deadline (mostly copying recv_timeout
documentation). This function is useful to avoid the often unnecessary call to
Instant::now in recv_timeout (e.g. when the user already has a deadline). A
concrete example would be something along those lines:

use std::sync::mpsc::Receiver;
use std::time::{Duration, Instant};

/// Reads a batch of elements
///
/// Returns as soon as `max_size` elements have been received or `timeout` expires.
fn recv_batch_timeout<T>(receiver: &Receiver<T>, timeout: Duration, max_size: usize) -> Vec<T> {
    recv_batch_deadline(receiver, Instant::now() + timeout, max_size)
}

/// Reads a batch of elements
///
/// Returns as soon as `max_size` elements have been received or `deadline` is reached.
fn recv_batch_deadline<T>(receiver: &Receiver<T>, deadline: Instant, max_size: usize) -> Vec<T> {
    let mut result = Vec::new();
    while let Ok(x) = receiver.recv_deadline(deadline) {
        result.push(x);
        if result.len() == max_size {
            break;
        }
    }
    result
}
Add std::sync::mpsc::Receiver::recv_deadline()
Essentially renames recv_max_until to recv_deadline (mostly copying recv_timeout
documentation). This function is useful to avoid the often unnecessary call to
Instant::now in recv_timeout (e.g. when the user already has a deadline). A
concrete example would be something along those lines:

```rust
use std::sync::mpsc::Receiver;
use std::time::{Duration, Instant};

/// Reads a batch of elements
///
/// Returns as soon as `max_size` elements have been received or `timeout` expires.
fn recv_batch_timeout<T>(receiver: &Receiver<T>, timeout: Duration, max_size: usize) -> Vec<T> {
    recv_batch_deadline(receiver, Instant::now() + timeout, max_size)
}

/// Reads a batch of elements
///
/// Returns as soon as `max_size` elements have been received or `deadline` is reached.
fn recv_batch_deadline<T>(receiver: &Receiver<T>, deadline: Instant, max_size: usize) -> Vec<T> {
    let mut result = Vec::new();
    while let Ok(x) = receiver.recv_deadline(deadline) {
        result.push(x);
        if result.len() == max_size {
            break;
        }
    }
    result
}
```
@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster
Member

shepmaster commented Nov 18, 2017

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 22, 2017

Member

Thanks for the PR! We’ll periodically check in on it to make sure that @Kimundi or someone else from the @rust-lang/libs team reviews it soon.

Member

kennytm commented Nov 22, 2017

Thanks for the PR! We’ll periodically check in on it to make sure that @Kimundi or someone else from the @rust-lang/libs team reviews it soon.

@ia0

This comment has been minimized.

Show comment
Hide comment
@ia0

ia0 Nov 22, 2017

Contributor

Thanks for the feedback!

Contributor

ia0 commented Nov 22, 2017

Thanks for the feedback!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 27, 2017

Member

Thanks for the PR @ia0! In general we never land new APIs as #[stable] so this'll want to switch to using #[unstable] along with a tracking issue for the API itself (I can tag it if you cc me on the issue).

Overall I like the idea of Instant-taking APIs to complement the Duration-taking APIs as well. I think it would be especially beneficial to do this for APIs like Condvar and std::thread in addition too. The tracking issue I think could be a good location to game out conventions here for more functions in the standard library and maybe even have a checklist for functions to implement.

Member

alexcrichton commented Nov 27, 2017

Thanks for the PR @ia0! In general we never land new APIs as #[stable] so this'll want to switch to using #[unstable] along with a tracking issue for the API itself (I can tag it if you cc me on the issue).

Overall I like the idea of Instant-taking APIs to complement the Duration-taking APIs as well. I think it would be especially beneficial to do this for APIs like Condvar and std::thread in addition too. The tracking issue I think could be a good location to game out conventions here for more functions in the standard library and maybe even have a checklist for functions to implement.

@ia0

This comment has been minimized.

Show comment
Hide comment
@ia0

ia0 Nov 27, 2017

Contributor

Thanks for your review @alexcrichton! I created #46316 to track a possible API convention for blocking-, timeout-, and deadline-related functions.

I switched to an unstable feature. And actually I wonder if I shouldn't have done the same for #45506 then.

Contributor

ia0 commented Nov 27, 2017

Thanks for your review @alexcrichton! I created #46316 to track a possible API convention for blocking-, timeout-, and deadline-related functions.

I switched to an unstable feature. And actually I wonder if I shouldn't have done the same for #45506 then.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 28, 2017

Member

Awesome, thanks @ia0! I'll comment on that soon.

It looks like tests are failing though? I think the doctests here will need to be tagged with the #![feature] directive

Member

alexcrichton commented Nov 28, 2017

Awesome, thanks @ia0! I'll comment on that soon.

It looks like tests are failing though? I think the doctests here will need to be tagged with the #![feature] directive

@ia0

This comment has been minimized.

Show comment
Hide comment
@ia0

ia0 Nov 28, 2017

Contributor

Indeed, I forgot to update the tests. It should be fixed now (at least locally ./x.py test src/libstd --stage=1 is not failing anymore).

Contributor

ia0 commented Nov 28, 2017

Indeed, I forgot to update the tests. It should be fixed now (at least locally ./x.py test src/libstd --stage=1 is not failing anymore).

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Nov 29, 2017

Member

@bors: r+

Thanks!

Oh also I forgot to say, but for the impl you added in #45506 it's safe to be "insta stable" and avoid the unstable attribute, trait implementations are "special" in that they're not currently covered by the stability system.

Member

alexcrichton commented Nov 29, 2017

@bors: r+

Thanks!

Oh also I forgot to say, but for the impl you added in #45506 it's safe to be "insta stable" and avoid the unstable attribute, trait implementations are "special" in that they're not currently covered by the stability system.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 29, 2017

Contributor

📌 Commit 8e025d8 has been approved by alexcrichton

Contributor

bors commented Nov 29, 2017

📌 Commit 8e025d8 has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 29, 2017

Contributor

⌛️ Testing commit 8e025d8 with merge 9546a1b...

Contributor

bors commented Nov 29, 2017

⌛️ Testing commit 8e025d8 with merge 9546a1b...

bors added a commit that referenced this pull request Nov 29, 2017

Auto merge of #45969 - ia0:mpsc_recv_deadline, r=alexcrichton
Add std::sync::mpsc::Receiver::recv_deadline()

Essentially renames recv_max_until to recv_deadline (mostly copying recv_timeout
documentation). This function is useful to avoid the often unnecessary call to
Instant::now in recv_timeout (e.g. when the user already has a deadline). A
concrete example would be something along those lines:

```rust
use std::sync::mpsc::Receiver;
use std::time::{Duration, Instant};

/// Reads a batch of elements
///
/// Returns as soon as `max_size` elements have been received or `timeout` expires.
fn recv_batch_timeout<T>(receiver: &Receiver<T>, timeout: Duration, max_size: usize) -> Vec<T> {
    recv_batch_deadline(receiver, Instant::now() + timeout, max_size)
}

/// Reads a batch of elements
///
/// Returns as soon as `max_size` elements have been received or `deadline` is reached.
fn recv_batch_deadline<T>(receiver: &Receiver<T>, deadline: Instant, max_size: usize) -> Vec<T> {
    let mut result = Vec::new();
    while let Ok(x) = receiver.recv_deadline(deadline) {
        result.push(x);
        if result.len() == max_size {
            break;
        }
    }
    result
}
```
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 29, 2017

Contributor

💔 Test failed - status-travis

Contributor

bors commented Nov 29, 2017

💔 Test failed - status-travis

@ia0

This comment has been minimized.

Show comment
Hide comment
@ia0

ia0 Nov 29, 2017

Contributor

Thanks for the review and explanation about impl stabilization.

The homu test seems to have failed because the build of LLVM for x86_64-apple-darwin seems to fail because future_error is unavailable:

[00:12:30] In file included from /Users/travis/build/rust-lang/rust/src/llvm/lib/Support/ThreadPool.cpp:14:
[00:12:30] In file included from /Users/travis/build/rust-lang/rust/src/llvm/include/llvm/Support/ThreadPool.h:30:
[00:12:30] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/future:1447:23: error: 'future_error' is unavailable: introduced in macOS 10.8
[00:12:30]                       future_error(make_error_code(future_errc::broken_promise))
[00:12:30]                       ^
[00:12:30] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/future:502:63: note: 'future_error' has been explicitly marked unavailable here
[00:12:30] class _LIBCPP_EXCEPTION_ABI _LIBCPP_AVAILABILITY_FUTURE_ERROR future_error
[00:12:30]                                                               ^

The other tests seem to have been cancelled.

Contributor

ia0 commented Nov 29, 2017

Thanks for the review and explanation about impl stabilization.

The homu test seems to have failed because the build of LLVM for x86_64-apple-darwin seems to fail because future_error is unavailable:

[00:12:30] In file included from /Users/travis/build/rust-lang/rust/src/llvm/lib/Support/ThreadPool.cpp:14:
[00:12:30] In file included from /Users/travis/build/rust-lang/rust/src/llvm/include/llvm/Support/ThreadPool.h:30:
[00:12:30] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/future:1447:23: error: 'future_error' is unavailable: introduced in macOS 10.8
[00:12:30]                       future_error(make_error_code(future_errc::broken_promise))
[00:12:30]                       ^
[00:12:30] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/future:502:63: note: 'future_error' has been explicitly marked unavailable here
[00:12:30] class _LIBCPP_EXCEPTION_ABI _LIBCPP_AVAILABILITY_FUTURE_ERROR future_error
[00:12:30]                                                               ^

The other tests seem to have been cancelled.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm
Member

kennytm commented Nov 29, 2017

kennytm added a commit to kennytm/rust that referenced this pull request Nov 29, 2017

Rollup merge of rust-lang#45969 - ia0:mpsc_recv_deadline, r=alexcrichton
Add std::sync::mpsc::Receiver::recv_deadline()

Essentially renames recv_max_until to recv_deadline (mostly copying recv_timeout
documentation). This function is useful to avoid the often unnecessary call to
Instant::now in recv_timeout (e.g. when the user already has a deadline). A
concrete example would be something along those lines:

```rust
use std::sync::mpsc::Receiver;
use std::time::{Duration, Instant};

/// Reads a batch of elements
///
/// Returns as soon as `max_size` elements have been received or `timeout` expires.
fn recv_batch_timeout<T>(receiver: &Receiver<T>, timeout: Duration, max_size: usize) -> Vec<T> {
    recv_batch_deadline(receiver, Instant::now() + timeout, max_size)
}

/// Reads a batch of elements
///
/// Returns as soon as `max_size` elements have been received or `deadline` is reached.
fn recv_batch_deadline<T>(receiver: &Receiver<T>, deadline: Instant, max_size: usize) -> Vec<T> {
    let mut result = Vec::new();
    while let Ok(x) = receiver.recv_deadline(deadline) {
        result.push(x);
        if result.len() == max_size {
            break;
        }
    }
    result
}
```

bors added a commit that referenced this pull request Nov 29, 2017

Auto merge of #46362 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45969, #46077, #46219, #46287, #46293, #46322, #46323, #46330, #46354, #46356
- Failed merges:

@bors bors merged commit 8e025d8 into rust-lang:master Nov 29, 2017

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment