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

Mutex fixes for interruptibility #5942

Merged
merged 2 commits into from Oct 26, 2019
Merged

Conversation

@headius
Copy link
Member

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 #5875.
@headius headius added this to the JRuby 9.2.9.0 milestone Oct 23, 2019
@headius

This comment has been minimized.

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 #5863.
@headius headius force-pushed the headius:thread_sync_interrupt_fixes branch from 1ed4081 to f6a9b46 Oct 26, 2019
@headius

This comment has been minimized.

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

This comment has been minimized.

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
5 checks passed
5 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jruby.jruby Build #20191026.6 succeeded
Details
jruby.jruby (Job linux) Job linux succeeded
Details
jruby.jruby (Job mac) Job mac succeeded
Details
jruby.jruby (Job windows) Job windows succeeded
Details
@headius headius deleted the headius:thread_sync_interrupt_fixes branch Oct 26, 2019
headius added a commit that referenced this pull request Oct 27, 2019
See #5718.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant
You can’t perform that action at this time.