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

POSIX threads corrections & enhancements #6944

Merged
merged 4 commits into from
Oct 16, 2018

Conversation

ysbaddaden
Copy link
Contributor

A collection of corrections and small enhancements to POSIX threads integration:

Fixes:

  • wrong error reporting: pthreads return an errnum but don't set errno;
  • prevent deadlocks & undefined behaviors by setting PTHREAD_MUTEX_ERRORCHECK;
  • make Thread::Mutex#try_lock report whether the lock was acquired or not;

Features:

  • add an overload with timeout to Thread::ConditionVariable#wait;
  • add Thread.yield to pass CPU time to another thread.

- Pass the returned errnum to `Errno.new`.

- Fix `Thread::Mutex.try_lock` to return true when the lock was
  acquired and false otherwise.

- Add a spec for `Thread.current`.
Prevents deadlocks and undefined mutex behavior.

> A thread attempting to relock this mutex without first
> unlocking it shall return with an error. A thread attempting
> to unlock a mutex which another thread has locked shall
> return with an error. A thread attempting to unlock an
> unlocked mutex shall return with an error.
>
> http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_mutexattr_settype.html
spec/std/thread_spec.cr Show resolved Hide resolved
src/thread.cr Show resolved Hide resolved
@ysbaddaden ysbaddaden force-pushed the core-fix-posix-threads branch from 6b8a295 to 54d3217 Compare October 16, 2018 09:47
@ysbaddaden
Copy link
Contributor Author

  • Fixed formatting error.
  • Darwin doesn't support setting a monotonic clock on condition variables; we must rely on the non-standard pthread_cond_timedwait_relative_np. Maybe the latest macOS releases added support (along with clock_gettime) thought, but it's not backward compatible, and CI doesn't have it anyway.

src/thread/mutex.cr Show resolved Hide resolved
src/thread.cr Show resolved Hide resolved
Configures thread condition variables to use the monotonic clock.

Adds an overload for `#wait` that takes a timeout and executes the
block when the timeout is reached, but doesn't when the condition
variable is normally resumed.

Refactors & enables Thread::ConditionVariable specs so they don't
randomly hang.
@ysbaddaden ysbaddaden force-pushed the core-fix-posix-threads branch from 54d3217 to 41045d6 Compare October 16, 2018 11:56
@ysbaddaden
Copy link
Contributor Author

Commits have been reordered, but since their sha didn't change, GitHub failed to notice it.

@RX14
Copy link
Contributor

RX14 commented Oct 16, 2018

@ysbaddaden the sha not changing is impossible...

@@ -1,90 +1,89 @@
require "spec"

# These specs are marked as pending until we add
# support for parallelism, where we'll see if we
# need condition variables.
Copy link
Member

Choose a reason for hiding this comment

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

Since these were pending until somebody works on parallelism, and nobody did that yet, is it worth uncommenting these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Who said "nobody's working on MT" :)

But sure, I can keep the note.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I meant all the specs. But if "somebody" ;-) is working on it, then this is fine.

@ysbaddaden
Copy link
Contributor Author

Well, I have no idea why github doesn't understand reordered commits, then 😕

@RX14 RX14 merged commit 0c6c721 into crystal-lang:master Oct 16, 2018
@ysbaddaden ysbaddaden deleted the core-fix-posix-threads branch October 16, 2018 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants