Skip to content

MpscBlockingConsumerArrayQueue: fix race writing to blocked field#336

Merged
nitsanw merged 3 commits into
JCTools:masterfrom
diffusiondata:master
Mar 3, 2021
Merged

MpscBlockingConsumerArrayQueue: fix race writing to blocked field#336
nitsanw merged 3 commits into
JCTools:masterfrom
diffusiondata:master

Conversation

@philipa

@philipa philipa commented Mar 2, 2021

Copy link
Copy Markdown
Contributor

Fix for this race:

Consumer in parkUntilNext()         Producer in offerAndWakeup()

casProducerIndex(0, 1)
                                    casProducerIndex(1, 2)
                                    soBlocked(null)
soBlocked(thread)

... spin until timeout

casProducerIndex(1, 0) fails

spinWaitForUnblock() for ever

franz1981
franz1981 requested changes Mar 2, 2021
View reviewed changes
Comment thread jctools-core/src/main/java/org/jctools/queues/MpscBlockingConsumerArrayQueue.java Outdated
franz1981
franz1981 approved these changes Mar 2, 2021
View reviewed changes

@franz1981 franz1981 left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM I haven't yet run the reproducer TBH, but the fix logic looks good

@coveralls

coveralls commented Mar 2, 2021 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 740

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 12 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.02%) to 86.705%
Files with Coverage Reduction New Missed Lines %
jctools-core/src/main/java/org/jctools/maps/NonBlockingHashMap.java 1 80.52%
jctools-core/src/main/java/org/jctools/queues/MpmcUnboundedXaddArrayQueue.java 2 96.15%
jctools-core/src/main/java/org/jctools/maps/NonBlockingHashMapLong.java 3 80.45%
jctools-core/src/main/java/org/jctools/maps/NonBlockingIdentityHashMap.java 3 82.73%
jctools-core/src/main/java/org/jctools/maps/NonBlockingSetInt.java 3 77.09%
Totals Coverage Status
Change from base Build 738: -0.02%
Covered Lines: 4950
Relevant Lines: 5709
💛 - Coveralls

@nitsanw

nitsanw commented Mar 3, 2021

Copy link
Copy Markdown
Contributor

Thanks @philipa this is a bad bug and a nice fix! will review and get a release out ASAP

@nitsanw nitsanw merged commit 3539dfa into JCTools:master Mar 3, 2021
@nitsanw nitsanw mentioned this pull request Mar 3, 2021
Closed
@dependabot dependabot Bot mentioned this pull request Mar 8, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@franz1981 franz1981 franz1981 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@philipa @coveralls @nitsanw @franz1981