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

API convention for blocking-, timeout-, and/or deadline-related functions #46316

Open
ia0 opened this Issue Nov 27, 2017 · 8 comments

Comments

Projects
None yet
6 participants
@ia0
Contributor

ia0 commented Nov 27, 2017

The standard library currently exposes several blocking- and/or timeout-related functions:

Function \ Versions Blocking Timeout (ms) Timeout
std::sync::Condvar::wait* wait wait_timeout_ms wait_timeout
std::sync::mpsc::Receiver::recv* recv none recv_timeout
std::thread::park* park park_timeout_ms park_timeout
std::thread::sleep* none sleep_ms sleep

The timeout versions taking a u32 in milliseconds are actually deprecated for the version taking a Duration since 1.6.0.

This issue tracks the possibility to extend these APIs and provide a convention for blocking-, timeout-, and/or deadline-related functions. The current suggestion is:

Function \ Versions Blocking Timeout Deadline
std::sync::Condvar::wait* wait wait_timeout wait_deadline
std::sync::mpsc::Receiver::recv* recv recv_timeout recv_deadline
std::thread::park* park park_timeout park_deadline
std::thread::sleep* none sleep_for sleep_until

The blocking versions do not take any extra argument and are not suffixed. The timeout versions take a timeout as a Duration and return if this timeout is reached (the timeout starts when the function is called with best-effort precision). They are suffixed by _timeout. The deadline versions take a deadline as an Instant and return if this deadline is reached (the deadline precision is best-effort). They are suffixed by _deadline.

For functions that do not have a meaningful blocking version (like sleep which would essentially block until the program ends), the timeout version would be suffixed by _for and the deadline version would be suffixed by _until. We don't have enough data-points to see if this rule is actually applicable. In a first iteration, we could leave aside those functions that do not have a meaningful blocking version.

@ia0

This comment has been minimized.

Contributor

ia0 commented Nov 27, 2017

@alexcrichton, as requested in #45969, I'm cc-ing you here to discuss which API we want for deadline-related functions.

@sfackler

This comment has been minimized.

Member

sfackler commented Nov 28, 2017

parking-lot uses wait_until which I like, though recv_until doesn't really make sense as a name.

@ia0

This comment has been minimized.

Contributor

ia0 commented Nov 28, 2017

Thanks @sfackler for the parking-lot example. This example actually conflicts with the standard library versions and extends the "no meaningful blocking version" suggestion above to functions that actually have a blocking version.

If I reformulate the suggestion according to this information, it would be:

  • If the primary role of the function is to do something and this thing may accidentally block (e.g. recv, send, wait, or park), then use _timeout and _deadline since the name of the function is not related to blocking.
  • If the primary role of the function is to block (e.g. sleep, wait, or park), then use _for and _until since the name of the function is related to blocking.

These rules overlap which is unfortunate, but this is maybe the freedom we need to account for the current state of naming by interpreting the primary role of a function differently. For example, for the standard library, the main role of Condvar::wait is to ensure we received a notification. The timeout and deadline parameters are potential accessories to avoid blocking too long. While for parking-lot, the main role of Condvar::wait is to block until we receive a notification. The timeout and deadline parameters are additional ways of unblocking. We could probably argue about the fact that if a timeout or deadline version needs to return whether the timeout or deadline was reached (and this information is semantically important), then it probably means that blocking is not the main role of the function. But going this way would probably only assign sleep-like functions to the _for/_until bucket, so it's probably a too strong constraint.

Just to clarify, with the current suggestion (and the first one which was very similar), recv_until is not an option since recv is doing something specific (receiving a message) and it only blocks by accident (this is not its primary role).

Here is the current suggestion (and whether it differs from the current state):

Action function \ Versions Blocking Timeout Deadline Has diff
std::sync::Condvar::wait* wait wait_timeout wait_deadline no
std::sync::mpsc::Receiver::recv* recv recv_timeout recv_deadline no
std::thread::park* park park_timeout park_deadline no
Blocking function \ Versions Blocking Timeout Deadline Has diff
parking_lot::Condvar::wait* wait wait_for wait_until no
std::thread::sleep* none sleep_for sleep_until yes
@ia0

This comment has been minimized.

Contributor

ia0 commented Nov 30, 2017

I took the time to go over the existing crates by downloading the latest version of each crate and grepping for public functions taking an Instant as argument (there are too many Duration arguments and we are interested in functions that come with duration and instant versions). The main noticeable facts are:

  • The notion of non-blocking functions (in addition to the current blocking, duration, and instant versions) that I forgot to take into account.
  • The usage of try_ to switch from _timeout/_deadline to _for/_until.
  • The usage of _at (in addition to _until and _deadline) for instant versions.

I changed the terminology for timeout and deadline versions to duration and instant versions to reflect the types and avoid possible biaises.

Here are some examples according to the 4 versions (blocking, non-blocking, duration, and instant):

Context Blocking Non-blocking Duration Instant
sqa_engine::sync::AudioThreadHandle recv try_recv wait_for wait_until
parking_lot::Mutex lock try_lock try_lock_for try_lock_until
parking_lot::RwLock read try_read try_read_for try_read_until
std::sync::mpsc::Receiver recv try_recv recv_timeout (recv_deadline)
std::sync::Condvar wait wait_timeout (wait_deadline)
parking_lot::Condvar wait wait_for wait_until
std::thread sleep_for sleep_until

Using try_ for the non-blocking version seems to always be verified. Then we have 3 different possibilities for the duration and instant versions (sorted according to my preferences):

  • Keep the try_ prefix and use _for and _until.
  • Use _timeout and _deadline.
  • Use another verb that works with _for and _until.

We see some usage of _at, but these usages seem to not be related to blocking functions, but simply to time functions. In particular there are only duration and instant versions (there are no blocking or non-blocking versions). Here are some examples (with my proposal):

Context Duration Instant
future_utils::Timeout new new_at
tk_easyloop timeout timeout_at
tokio_core::reactor::Interval new new_at
tokio_timer::Timer interval interval_at
proposal do_stuff_in do_stuff_at

Some outsiders:

Context Duration Instant
task_scheduler::Scheduler after_duration after_instant
freertos_rs::TaskDelay delay_until(?)
@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 1, 2017

It's worth pointing out that there may also just not be a convention to be had here. There's certainly not an "obvious agreement" amongst everything today it seems like and I wouldn't want to bend over too much to try to fit things in!

That being said I do sort of like the _until suffix for Instant-taking operations. Even something like recv_until sort of makes sense to me if you sit and think about it for a second. That may be a bit of a pipe dream though :)

spastorino added a commit to spastorino/rust that referenced this issue Dec 5, 2017

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 17, 2018

This thread suggests renaming std::thread::sleep, but that function is already stable. Is the naming consistency worth introducing a new function and deprecating the old one? (The old one would have to stay deprecated-but-stable "forever".)

This issue is now also the tracking issue for std::sync::mpsc::Receiver::recv_deadline which landed in Nightly on 2017-11-29. However it seems like this issue also discusses other APIs that are not implemented (yet?)

@ia0

This comment has been minimized.

Contributor

ia0 commented Mar 17, 2018

I created this issue in response to #45969 (comment). Maybe we should split it between the tracking issue of std::sync::mpsc::Receiver::recv_deadline and a possible convention for blocking functions. We could also decide to forget about a possible convention (because it seems like it is too late to do this kind of work since it would imply having deprecated-but-stable or duplicated APIs) and just keep this issue as a tracking issue.

What is the timeline for a tracking issue? Should we keep it open until the feature goes stable? Is there any time constraints about this?

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 17, 2018

There is no time constraint. A tracking issue stays open until the corresponding feature becomes stable (in the sense of: does not require #[feature(foo)] to opt in) or is deprecated in the master branch. That change usually reaches the Nightly channel the next day, and the Stable 6 to 12 weeks later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment