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

std: Implement thread::sleep #23330

Merged
merged 2 commits into from
Mar 17, 2015
Merged

std: Implement thread::sleep #23330

merged 2 commits into from
Mar 17, 2015

Conversation

alexcrichton
Copy link
Member

This function is the current replacement for std::old_io::timer which will
soon be deprecated. This function is unstable and has its own feature gate as it
does not yet have an RFC nor has it existed for very long.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

@sfackler all very good points on alexcrichton@806da80, thanks for the sharp eye! They should all be taken care of now

if pthread_getattr_np(pthread_self(), &mut attr) != 0 {
panic!("failed to get thread attributes");
}
eq!(pthread_getattr_np(pthread_self(), &mut attr), 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reduces clarity of the error and makes harder to find the cause IMO. Also, if debug_assert_eq!, does what I think it does, this code behave incorrectly and most likely will SIGSEGV/SIGILL in light of #22642 and possibly many other cases we don’t account for yet.

I would defer this change until somebody gets rid of pthread usage here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reduces clarity of the error and makes harder to find the cause IMO.

Could you expand on this some more? I expect this panic message to be more informative as it includes the line number, expression, and return value if it's not zero.

Also, if debug_assert_eq!, does what I think it does, this code behave incorrectly and most likely will SIGSEGV/SIGILL

That's a good point, I'll switch back to assert_eq!

I would defer this change until somebody gets rid of pthread usage here.

I don't quite follow, do you think we can get rid of this usage?

@alexcrichton alexcrichton force-pushed the thread-sleep branch 3 times, most recently from 67df986 to a4af022 Compare March 13, 2015 18:00
}
}

pub fn sleep(dur: Duration) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A duplicate definition.

This module had become a #[cfg] jungle, try to bring at least a small semblance
of order to it!
This function is the current replacement for `std::old_io::timer` which will
soon be deprecated. This function is unstable and has its own feature gate as it
does not yet have an RFC nor has it existed for very long.
@alexcrichton
Copy link
Member Author

@bors: r=aturon 04cf534

request: *const libc::timespec,
remain: *mut libc::timespec) -> libc::c_int;
}
clock_nanosleep(libc::CLOCK_MONOTONIC, 0, ts, ts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no point using clock_nanosleep if you don't use an absolute time. I'm gonna have to post the manpage again:

The fact that nanosleep() sleeps for a relative interval can be
problematic if the call is repeatedly restarted after being inter‐
rupted by signals, since the time between the interruptions and
restarts of the call will lead to drift in the time when the sleep
finally completes. This problem can be avoided by using
clock_nanosleep(2) with an absolute time value.

Right now the code is more complicated than necessary with no benefit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The move to clock_nanosleep was motivated by dealing with system clock drift instead of restarts because of EINTR. That form of slop is likely quite small and is already dealt with in the spec of the sleep function in std::thread (it is not precise).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's neither a cross-platform fix nor does it go the full way to make the implementation rock-solid on one platform. It's useless code bloat.

Funny how you always refuse to use a memcpy/memset wrapper with measurable performance impact in io code. But this haphazard fix is somehow worth two extra platform specific functions that will only be used once.

@Manishearth
Copy link
Member

@bors: force

The other PR is just a doc rollup

bors added a commit that referenced this pull request Mar 17, 2015
This function is the current replacement for `std::old_io::timer` which will
soon be deprecated. This function is unstable and has its own feature gate as it
does not yet have an RFC nor has it existed for very long.
@bors
Copy link
Contributor

bors commented Mar 17, 2015

⌛ Testing commit 04cf534 with merge bfac337...

@bors bors merged commit 04cf534 into rust-lang:master Mar 17, 2015
@ghmlee
Copy link

ghmlee commented Mar 18, 2015

Thanks. :)

@alexcrichton alexcrichton deleted the thread-sleep branch March 27, 2015 20:43
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.