From: Eric Wong <e@80x24.org>
To: spew@80x24.org
Subject: [PATCH] thread_sync.c (rb_mutex_synchronize): only unlock if we own the mutex
Date: Sat, 18 Aug 2018 06:26:57 +0000 [thread overview]
Message-ID: <20180818062657.3424-1-e@80x24.org> (raw)
If an exception is raised inside Mutex#sleep (via ConditionVariable#wait),
we cannot guarantee we can own the mutex in the ensure callback.
However, who owns the mutex at that point does not matter. What
matters is the Mutex is usable after an exception occurs.
* thread_sync.c (rb_mutex_synchronize): only unlock if we own the mutex
* spec/ruby/library/conditionvariable/wait_spec.rb: only test lock
usability after thread kill. Who owns the lock at any
particular moment is an implementation detail which we cannot
easily guarantee.
---
.../library/conditionvariable/wait_spec.rb | 26 ++-----
thread_sync.c | 74 ++++++++++++-------
2 files changed, 56 insertions(+), 44 deletions(-)
diff --git a/spec/ruby/library/conditionvariable/wait_spec.rb b/spec/ruby/library/conditionvariable/wait_spec.rb
index d4950a7b27..99e14efe35 100644
--- a/spec/ruby/library/conditionvariable/wait_spec.rb
+++ b/spec/ruby/library/conditionvariable/wait_spec.rb
@@ -23,21 +23,15 @@
th.join
end
- it "reacquires the lock even if the thread is killed" do
+ it "the lock remains usable even if the thread is killed" do
m = Mutex.new
cv = ConditionVariable.new
in_synchronize = false
- owned = nil
th = Thread.new do
m.synchronize do
in_synchronize = true
- begin
- cv.wait(m)
- ensure
- owned = m.owned?
- $stderr.puts "\nThe Thread doesn't own the Mutex!" unless owned
- end
+ cv.wait(m)
end
end
@@ -49,24 +43,19 @@
th.kill
th.join
- owned.should == true
+ m.try_lock.should == true
+ m.unlock
end
- it "reacquires the lock even if the thread is killed after being signaled" do
+ it "lock remains usable even if the thread is killed after being signaled" do
m = Mutex.new
cv = ConditionVariable.new
in_synchronize = false
- owned = nil
th = Thread.new do
m.synchronize do
in_synchronize = true
- begin
- cv.wait(m)
- ensure
- owned = m.owned?
- $stderr.puts "\nThe Thread doesn't own the Mutex!" unless owned
- end
+ cv.wait(m)
end
end
@@ -84,7 +73,8 @@
}
th.join
- owned.should == true
+ m.try_lock.should == true
+ m.unlock
end
it "supports multiple Threads waiting on the same ConditionVariable and Mutex" do
diff --git a/thread_sync.c b/thread_sync.c
index 5e511af0db..f3d1ccb120 100644
--- a/thread_sync.c
+++ b/thread_sync.c
@@ -321,6 +321,38 @@ rb_mutex_owned_p(VALUE self)
return owned;
}
+static void
+mutex_do_unlock(rb_mutex_t *mutex, rb_thread_t *th)
+{
+ struct sync_waiter *cur = 0, *next = 0;
+ rb_mutex_t **th_mutex = &th->keeping_mutexes;
+
+ VM_ASSERT(mutex->th == th);
+
+ mutex->th = 0;
+ list_for_each_safe(&mutex->waitq, cur, next, node) {
+ list_del_init(&cur->node);
+ switch (cur->th->status) {
+ case THREAD_RUNNABLE: /* from someone else calling Thread#run */
+ case THREAD_STOPPED_FOREVER: /* likely (rb_mutex_lock) */
+ rb_threadptr_interrupt(cur->th);
+ goto found;
+ case THREAD_STOPPED: /* probably impossible */
+ rb_bug("unexpected THREAD_STOPPED");
+ case THREAD_KILLED:
+ /* not sure about this, possible in exit GC? */
+ rb_bug("unexpected THREAD_KILLED");
+ continue;
+ }
+ }
+ found:
+ while (*th_mutex != mutex) {
+ th_mutex = &(*th_mutex)->next_mutex;
+ }
+ *th_mutex = mutex->next_mutex;
+ mutex->next_mutex = NULL;
+}
+
static const char *
rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t *th)
{
@@ -333,31 +365,7 @@ rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t *th)
err = "Attempt to unlock a mutex which is locked by another thread";
}
else {
- struct sync_waiter *cur = 0, *next = 0;
- rb_mutex_t **th_mutex = &th->keeping_mutexes;
-
- mutex->th = 0;
- list_for_each_safe(&mutex->waitq, cur, next, node) {
- list_del_init(&cur->node);
- switch (cur->th->status) {
- case THREAD_RUNNABLE: /* from someone else calling Thread#run */
- case THREAD_STOPPED_FOREVER: /* likely (rb_mutex_lock) */
- rb_threadptr_interrupt(cur->th);
- goto found;
- case THREAD_STOPPED: /* probably impossible */
- rb_bug("unexpected THREAD_STOPPED");
- case THREAD_KILLED:
- /* not sure about this, possible in exit GC? */
- rb_bug("unexpected THREAD_KILLED");
- continue;
- }
- }
- found:
- while (*th_mutex != mutex) {
- th_mutex = &(*th_mutex)->next_mutex;
- }
- *th_mutex = mutex->next_mutex;
- mutex->next_mutex = NULL;
+ mutex_do_unlock(mutex, th);
}
return err;
@@ -497,6 +505,20 @@ mutex_sleep(int argc, VALUE *argv, VALUE self)
return rb_mutex_sleep(self, timeout);
}
+static VALUE
+mutex_unlock_if_owned(VALUE self)
+{
+ rb_thread_t *th = GET_THREAD();
+ rb_mutex_t *mutex;
+ GetMutexPtr(self, mutex);
+
+ /* we may not own the mutex if an exception occured */
+ if (mutex->th == th) {
+ mutex_do_unlock(mutex, th);
+ }
+ return Qfalse;
+}
+
/*
* call-seq:
* mutex.synchronize { ... } -> result of the block
@@ -509,7 +531,7 @@ VALUE
rb_mutex_synchronize(VALUE mutex, VALUE (*func)(VALUE arg), VALUE arg)
{
rb_mutex_lock(mutex);
- return rb_ensure(func, arg, rb_mutex_unlock, mutex);
+ return rb_ensure(func, arg, mutex_unlock_if_owned, mutex);
}
/*
--
EW
reply other threads:[~2018-08-18 6:26 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180818062657.3424-1-e@80x24.org \
--to=e@80x24.org \
--cc=spew@80x24.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).