Skip to content

Mutex fixes for interruptibility#5942

Merged
headius merged 2 commits intojruby:masterfrom
headius:thread_sync_interrupt_fixes
Oct 26, 2019
Merged

Mutex fixes for interruptibility#5942
headius merged 2 commits intojruby:masterfrom
headius:thread_sync_interrupt_fixes

Conversation

@headius
Copy link
Member

@headius headius commented Oct 23, 2019

This PR contains fixes for some interruptibility issues introduced in JRuby 9.2.8:

  • A thread woken up while attempting to acquire a contended lock would raise an error rather than continuing to try to acquire the lock.
  • A thread sleeping via a ConditionVariable would be uninterruptible.

WIP until I confirm it's ready.

The original code would immediately error out if a contended lock
acquisition were interrupted before it could acquire the lock.
This led to a behavioral difference from MRI, which does not raise
errors. It is not clear whether that's because locks in MRI can
never be truly contended (if the lock is not available the thread
is put to sleep and woken by the lock owner) or because they
explicitly try again to acquire the lock after waking.

In this version of the code, we opt to continue trying to acquire
the lock via a loop, unless a thread event like raise or kill tell
us to divert from the normal path of execution. The only possible
outcomes from interrupting a lock acquisition are to keep trying
until we have the lock or error out. This patch opts for the
former.

Fixes jruby#5875.
@headius headius added this to the JRuby 9.2.9.0 milestone Oct 23, 2019
@headius
Copy link
Member Author

headius commented Oct 23, 2019

Fix for #5875 is in.

Previous logic used the same semaphore to sleep as any other sleep
which interfered with code that expected the Mutex to be the lock
used. The new logic uses the Mutex's JDK Lock, via a Condition, to
do the sleeping.

Because it is not possible for us to change the artificial thread
status we maintain to "sleep" after the lock is released, this
modified logic also introduces a new thread state that indicates
that the native JDK thread state should be used. This gets closer
to avoiding the racing status.

It does not appear to eliminate the race altogether.

Fixes jruby#5863.
@headius headius force-pushed the thread_sync_interrupt_fixes branch from 1ed4081 to f6a9b46 Compare October 26, 2019 23:06
@headius
Copy link
Member Author

headius commented Oct 26, 2019

Fix for #5863 is in.

The second fix introduces a new RubyThread status called "NATIVE", which indicates that the native JDK thread status should be translated into a Ruby thread status. This allows us to use the provided JDK Condition.await, which does the appropriate lock release followed by going into a wait state (via LockSupport.park). This in turn allows the specs to pass that depend on the lock being released by the time the thread status is "sleep".

This fix is also a stepping stone toward eliminating the synthetic Ruby thread statuses. As we go forward we can modify more of these state changes to just defer to the NATIVE status, leaning on the JDK to handle state changes without racing.

@headius
Copy link
Member Author

headius commented Oct 26, 2019

Assuming that specs pass, I consider this one ready to go. Probably should create some specs for the cases in #5942 and #5863.

@headius headius merged commit f8d8eae into jruby:master Oct 26, 2019
@headius headius deleted the thread_sync_interrupt_fixes branch October 26, 2019 23:52
headius added a commit that referenced this pull request Oct 27, 2019
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.

1 participant