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

test, review & avoid (regressed) mutex behavior #5522

Merged
merged 8 commits into from Dec 29, 2018

Conversation

Projects
None yet
2 participants
@kares
Copy link
Member

kares commented Dec 16, 2018

these changes are based on reviewing code for #5520

than there's the actual "wild" fix: https://github.com/jruby/jruby/compare/mutex-fix?expand=1#diff-330867c3ee2ffcb2a20e80af7bf9b4b3R161 ... as this line doing synchronization locking seems to be causing the problem (see explanation at GH-5520)

not entirely sure - why it's happening, yet, maybe it relates to delivering interrupts on the main thread?

to reproduce, just run the test that is added here :

TIMES=100_000 bin/jruby test/jruby/test_thread.rb --name=/latch/

.. changing the line back to a blocking collection should make the test fail e.g.

    /** Stack of interrupt masks active for this thread */
-    private final List<RubyHash> interruptMaskStack = new CopyOnWriteArrayList<>();
+    private final List<RubyHash> interruptMaskStack = new Vector<>();

https://github.com/jruby/jruby/compare/mutex-fix?expand=1#diff-330867c3ee2ffcb2a20e80af7bf9b4b3R161

@headius feel free to push to the branch as/if needed. while I'd like to understand why that line causes such a regression I might not have the time to look in detail (next week).

@kares kares added this to the JRuby 9.2.6.0 milestone Dec 16, 2018

@kares kares requested a review from headius Dec 16, 2018

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Dec 17, 2018

interestingly, no longer passes the interrupt mask test - unexcluded from the related commit: 66d2905

headius added a commit that referenced this pull request Dec 18, 2018

Don't use addAll to fill one sync list with another's contents.
This relates to #5520 and #5522 but reduces the changes needed for
a fix to just this collection. The script provided in #5520 runs
to completion with this fix, even bumped up to 100-way concurrency
and 100k iterations. This does not appear to regress the handler
test that regressed due to #5522.
@headius

This comment has been minimized.

Copy link
Member

headius commented Dec 18, 2018

For 9.2.6 I have pushed a trivial version of the core fix here, the deadlock inside the copy of interrupt handlers. See adcd781.

The deadlock came from having two synchronized lists try to addAll at the same time. I have not confirmed exactly how that situation happens, but this fix makes sure it never will.

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Dec 21, 2018

rebased/redone on top of master - to also take a mask snapshot.
... might move the test commit to master just to see if it reproduces.

no need to sync on mutex on unlock/locked?
the underlying (re-entrant) lock does it for us

@kares kares force-pushed the mutex-fix branch 2 times, most recently from 5692996 to 349f760 Dec 28, 2018

@kares kares changed the title fix (regressed) thread mutual exclusion behavior test, review & avoid (regressed) mutex behavior Dec 29, 2018

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Dec 29, 2018

okay, test is doing nice and w snapshot taking I am no longer reproducing any (mutex) issues.
have some interrupt logic for matching compatibility, will base those as this one gets merged.

@kares kares merged commit 4acc3c8 into master Dec 29, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@kares kares deleted the mutex-fix branch Dec 29, 2018

@kares kares modified the milestones: JRuby 9.2.7.0, JRuby 9.2.6.0 Dec 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.