Pull Request Test Coverage Report for Build 699

• 41 of 43 (95.35%) changed or added relevant lines in 1 file are covered.
• 15 unchanged lines in 4 files lost coverage.
• Overall coverage decreased (-0.02%) to 86.194%

Totals
Change from base Build 681:	-0.02%
Covered Lines:	4776
Relevant Lines:	5541

💛 - Coveralls

I don't understand why the last poll should spin wait for element TBH, given that there is no new element on offset' beside the one already consumed by take(timeout), need to look better into this

Look at spinWaitForElement. It will wait for ever for an update to offset, but none is forthcoming. Further, the "busy bit" of producerIndex is left set but blocked is null, so the next offer will spin too.

Or try reverting the fix and running the test. It failed consistently for me, even with additional logging so I could figure out what was happening.

What it looks strange to me is the check on https://github.com/pushtechnology/JCTools/blob/865a75f12cfa3d63029f16979dc8ee2af7e4a5b1/jctools-core/src/main/java/org/jctools/queues/MpscBlockingConsumerArrayQueue.java#L577 : I will add some logs to understand why it isn't returning null instead

Oh, right. I was using poll(timeout) throughout, so this is irrelevant. I'll updated the comment above accordingly.

Ah ok so the issue happens with 2 subsequent poll(timeout) and the expectation would be that:

1. first one should time out but read the offered element
2. second one should time out and return null

Or

1. first one should time out and return null
2. second one should return an element regardless timing out (if the offer land in time)

Yes, at least two poll(timeout)s and a concurrent offerAndWait().

It reproduced for me fairly readily once I had the test balanced to ensure a mostly empty queue. I'd be interested to know if the test fails for you (with the fix reverted).

Hi @philipa ! long time no see :-)

Thanks for the find + PR, this is an interesting one. The thinking behind the original ordering is that we want to avoid the spin when we wake the consumer from an unbounded take. There have been prior concerns around spinning with no yield in single CPU environments or where the scheduler unpark results in the unparked thread executing on the same core. Upon reflection I'm not sure if re-ordering is the "right" solution (I think it's correct), or if we need heavier weight guards on the state transitions here via e.g. a CAS on the blocked field. WDYT?

Not sure CAS on blocked works alone, since the producer would need to distinguish between the consumer waiting for a value at the original offset vs it having woken up early, sneakily read the element, and now waiting for an element at the next offset. cIndex is also pertinent here.

Here is an alternative fix to consumer-side which should not affect the existing happy paths at all: 3f82f1c. The existing cas in the finally block already detects this race and so it's just a matter of handling it appropriately (I removed the try/finally and now have separate checks for interrupt and timeout cases). wdyt?

Looks good. I prefer the revised code flow. It would make sense to restructure take similarly to remove the finally block.

I did like e8e0839 though.

+100 for the changes of @njhill

Thanks @philipa @franz1981, agree about the restructure of take. I should note that I was the one that introduced this bug in the first place, as well as the the try/finally flow which @nitsanw wasn't in favour of at the time... so more reason to change it I guess!

@philipa thanks for the good work! I'll do a final pass today

@philipa @njhill See minor changes: diffusiondata@c3ea433

If you can squash it all to one nice commit I think we're done :-)

Refactoring looks good.

Excuse my GH ignorance, but don't you have a "squash and merge" button for PRs?

Great work team :-)