Skip to content

Commit dbe4b43

Browse files
committed
[GR-20089] Always use condLock.lock() for locking the ConditionVariable's ReentrantLock
* Changing the thread status or consuming interrupt as before was incorrect and could lead to an interrupt from Thread#{run,wakeup} being missed. * condLock is only held for a short number of non-blocking instructions, so there is no need to poll for safepoints while locking it.
1 parent b5dabd8 commit dbe4b43

File tree

1 file changed

+13
-22
lines changed

1 file changed

+13
-22
lines changed

src/main/java/org/truffleruby/core/mutex/ConditionVariableNodes.java

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import org.truffleruby.builtins.PrimitiveArrayArgumentsNode;
2323
import org.truffleruby.core.thread.GetCurrentRubyThreadNode;
2424
import org.truffleruby.core.thread.ThreadManager;
25-
import org.truffleruby.core.thread.ThreadManager.BlockingAction;
2625
import org.truffleruby.core.thread.ThreadStatus;
2726
import org.truffleruby.language.Visibility;
2827
import org.truffleruby.language.objects.AllocateObjectNode;
@@ -44,8 +43,12 @@ public abstract static class AllocateNode extends CoreMethodArrayArgumentsNode {
4443

4544
@Specialization
4645
protected DynamicObject allocate(DynamicObject rubyClass) {
47-
ReentrantLock lock = new ReentrantLock();
48-
return allocateNode.allocate(rubyClass, lock, lock.newCondition(), 0, 0);
46+
// condLock is only held for a short number of non-blocking instructions,
47+
// so there is no need to poll for safepoints while locking it.
48+
// It is an internal lock and so locking should be done with condLock.lock()
49+
// to avoid changing the Ruby Thread status and consume Java thread interrupts.
50+
final ReentrantLock condLock = new ReentrantLock();
51+
return allocateNode.allocate(rubyClass, condLock, condLock.newCondition(), 0, 0);
4952
}
5053

5154
}
@@ -95,11 +98,11 @@ private void waitInternal(DynamicObject conditionVariable, ReentrantLock mutexLo
9598
// it should only be considered if we are inside Mutex#sleep when Thread#{run,wakeup} is called.
9699
Layouts.THREAD.getWakeUp(thread).set(false);
97100

98-
// condLock must be locked before unlocking mutexLock, to avoid losing potential signals
99-
getContext().getThreadManager().runUntilResult(this, () -> {
100-
condLock.lockInterruptibly();
101-
return BlockingAction.SUCCESS;
102-
});
101+
// condLock must be locked before unlocking mutexLock, to avoid losing potential signals.
102+
// We must not change the Ruby Thread status and not consume a Java thread interrupt while locking condLock.
103+
// If there is an interrupt, it should be consumed by condition.await() and the Ruby Thread sleep status
104+
// must imply being ready to be interrupted by Thread#{run,wakeup}.
105+
condLock.lock();
103106
mutexLock.unlock();
104107

105108
Layouts.CONDITION_VARIABLE
@@ -205,13 +208,7 @@ protected DynamicObject signal(DynamicObject self) {
205208
final ReentrantLock condLock = Layouts.CONDITION_VARIABLE.getLock(self);
206209
final Condition condition = Layouts.CONDITION_VARIABLE.getCondition(self);
207210

208-
if (!condLock.tryLock()) {
209-
getContext().getThreadManager().runUntilResult(this, () -> {
210-
condLock.lockInterruptibly();
211-
return BlockingAction.SUCCESS;
212-
});
213-
}
214-
211+
condLock.lock();
215212
try {
216213
if (Layouts.CONDITION_VARIABLE.getWaiters(self) > 0) {
217214
Layouts.CONDITION_VARIABLE.setSignals(self, Layouts.CONDITION_VARIABLE.getSignals(self) + 1);
@@ -234,13 +231,7 @@ protected DynamicObject broadcast(DynamicObject self) {
234231
final ReentrantLock condLock = Layouts.CONDITION_VARIABLE.getLock(self);
235232
final Condition condition = Layouts.CONDITION_VARIABLE.getCondition(self);
236233

237-
if (!condLock.tryLock()) {
238-
getContext().getThreadManager().runUntilResult(this, () -> {
239-
condLock.lockInterruptibly();
240-
return BlockingAction.SUCCESS;
241-
});
242-
}
243-
234+
condLock.lock();
244235
try {
245236
if (Layouts.CONDITION_VARIABLE.getWaiters(self) > 0) {
246237
Layouts.CONDITION_VARIABLE.setSignals(

0 commit comments

Comments
 (0)