TINKERPOP-2217

[danielcweber]

https://issues.apache.org/jira/browse/TINKERPOP-2217

Fix potentially harmful timing issue: _writeInProgress could be observed by BeginSendingMessages to indicate that the loop in SendMessagesFromQueueAsync is still "in flight" while in reality, it has already exited.

[FlorianHockmann]

Thanks for tackling this, @danielcweber! LGTM in general, I only commented on 2 nits.

[FlorianHockmann]

(nitpick) I don't have a strong opinion on this, but an (integer) flag that can only have two values to represent the two states seems simpler to me than a counter that is only compared to 0 and 1.

[danielcweber]

Only that it's wrong. The send-while-loop may exit, meanwhile the queuing observes the flag to be 1, not starting the loop. After that, the flag is reset, but it's too late. The work item may never be processed.

[danielcweber]

It doesn't help IMO to include CompareExchange into the while loop. When it's decided to exit the loop, another queueing attempt may already have observed the queue to be running, and work may be lost.

That being said, I'm not particularly awesome at lock free programming. I think we could also just use ordinary locks in all those places and be good.

[FlorianHockmann]

Yeah, never mind. I though that Decrement could be replaced by the previous version with CompareExchange, but that would exit the while loop after a single iteration. Since you have to evaluate the state of _writeInProgress in every iteration, it really needs to be a counter instead of a flag.
It's really a bit confusing that _writeInProgress is now effectively a counter and a status flag at the same time, but I also don't know how we could solve that differently.

[FlorianHockmann]

(My answer actually only addressed your first comment here as I wrote it before seeing your second comment.)

That being said, I'm not particularly awesome at lock free programming. I think we could also just use ordinary locks in all those places and be good.

Me neither, but how could we implement this with ordinary locks without blocking multiple task waiting to get the lock? The current implementation ensures that a single task can send all messages in the queue and do that in the background.
But it would in general be good if @jorgebay could also review this as he definitely has more experience than I do in this area.

[danielcweber]

No, it's not a status flag ever, it's only a counter. The comparison == 1 just asserts whether the increment (= work item queue) is the first one when idle, and thus has to start the loop. Same for the check on 0. We must ensure that we really drained the queue and decremented the counter to zero to be able to leave the loop.

What's non optimal is that we now have two sources of truth of the count of work items: The queue itself and the counter. EDIT: That's why I put the increment and enqueue together as a unit. There should now never be discrepancy.

[danielcweber]

Me neither, but how could we implement this with ordinary locks without blocking multiple task waiting to get the lock?

That's not a big problem, you don't have to go lock free to be async. Also, you don't have to block inside of locks, and still you can maintain async code. Stephen Cleary has a wonderful library of async synchronization primitives that could help a lot here, but I'm not sure whether you'd want to take a dependency on a 3rd party lib.

[jorgebay]

Nice catch @danielcweber !

I think we can fix the race condition by peeking after yielding the _writeInProgress flag:

        private async Task SendMessagesFromQueueAsync()
        {
            // ...
            Interlocked.CompareExchange(ref _writeInProgress, 0, 1);

            // Since the loop ended and the write in progress was set to 0
            // a new item could have been added, write queue can contain items at this time
            if (!_writeQueue.IsEmpty && Interlocked.CompareExchange(ref _writeInProgress, 1, 0) == 1)
            {
                await SendMessagesFromQueueAsync().ConfigureAwait(false);
            }
        }

I personally would like this change to the approach proposed in this patch because it requires less effort to review 3 lines of code than 20+ in this sensitive part of the driver. Would you like to change your patch?

[danielcweber]

Can be done. Also, you might wanna check out my alternative approach, because actually all we need is async locking, we don't need no send-queue at all.

[jorgebay]

thanks @danielcweber!

VOTE +1

[FlorianHockmann]

VOTE +1

[FlorianHockmann]

Merged via CTR.