seq: fix possible race in seq_queue

[ralphlange]

Fix possible race condition in seq_queue,
where internal state was checked outside of a mutex.
Fixes unit test failure in MacOS builds.

Co-authored-by: google-labs-jules[bot]

[anderslindho]

Tested locally with macOS and arm64. I reproduced the failure 2/2 on queueTest.t with Bail out! 4589<=4587. I passed 1/1 with full runtests and 3/3 on queueTest.t specifically after the patch.

[anderslindho]

One follow-up comment. The queue status helpers do not take q->mutex. Most uses seem read-only/status-only, but seqQueueIsEmpty() is used by seq_pvGetQ. Should we maybe lock seqQueueIsEmpty() too, or all of seqQueueFree(), seqQueueUsed(), seqQueueIsEmpty(), seqQueueIsFull() for consistency?

[simon-ess]

I see the same behaviour as @anderslindho, and agree with his assessment of adding mutex locks.

There's also the other question: why does this fail on MacOS but not on various linuces?

[ralphlange]

There's also the other question: why does this fail on MacOS but not on various linuces?

Timing, I would think. These multi-core arms are just a whole bit faster.

[ralphlange]

... agree with his assessment of adding mutex locks.

Let me check a bit further in the commit history.

The original author(s?) were obviously very concerned about lock contention - the extra gymnastics that this PR removes are telling a story... In principle, read-only/status-only could be fine (off-by-one is ok in those cases), unless a race would produce crap data.

I agree that locking would increase consistency and beauty - just wondering about the price tag.

[ralphlange]

Commit ede72542

seq: complete redesign of broken queue adt

• use size_t for all indices and sizes
• state and ensure invariants
• an additional boolean is needed to indicate overflow
• added a mutex (used only when queue is overflowing)
• added generic (unsafe) access methods

[ralphlange]

So the idea was to only lock in overflow - interesting.

Nice article on the topic (not on locks, though):
https://www.snellman.net/blog/archive/2016-12-13-ring-buffers

[anderslindho]

Indeed a nice article!

I just noticed that there is a header comment that will need updating if we accept this fix (seq_queue.h):

The implementation allows one reader and one writer to access the queue
without taking a mutex, except where unavoidable, i.e. when the queue is
full.

Re costs, haven't we taken the brunt of them already with the change in this PR?

[simon-ess]

I am sort of surprised at this. There is a mutex to block concurrent put/get when full which... fine? But why not concurrent gets or puts? Isn't that also a race condition? i.e. two puts at the same time would write to the same spot in memory with one of them being overwritten, no?

[ralphlange]

i.e. two puts at the same time would write to the same spot in memory with one of them being overwritten, no?

See the comment that Anders mentioned: It's an attempt to implement a lock-free "SPSC" (single producer, single consumer) queue.

[simon-ess]

Ah, I think I missed that it was a single producer and consumer. This does make more sense then.