I don't think I will be able to deflake the original test as the order of whether doc1, doc2, doc1 or doc1, doc1, doc2 is enqueued is no longer deterministic. I don't think that's a problem though, but it breaks this test.

This behavior is still reasonable though, so it should be fine.

This behavior is still reasonable though, so it should be fine.

Woah, did not know about volatile in Java.

If lastOperation is only accessed inside the synchronized call, why do we still need the volatile label? My stackoverflow hunting initially led me to think of volatile variables as a simpler/weaker form of synchronization than synchronized, which makes volatile redundant (link, another link). However, after reading another conflicting post that argues volatile is required for reading the variable, I'm now uncertain.

Actually, I realize we're reading lastOperation in flush(), which is out of the synchronized block, which then means that we need to use volatile.

Drive by comment... Although I can't find the documentation to back this up, I'm 99% sure that you basically never need to declare a variable as volatile in Java. In my entire career I have never needed to use volatile, even in heavily multi-threaded code. I think that the only cases where you would need to use volatile would be (a) if you're implementing some sort of custom lock-free synchronization on the variable (e.g. busy waiting) or (b) if the variable's value could be changed asynchronously from native code.

volatile ensures that the value that is read is never read from the CPU cache. Without this, flush() is not guaranteed to see the latest state of lastOperation if lastOperation was modified by another thread.

cc @wilhuff

This volatile is an indicator of broader problems within this implementation.

• verifyNotClosed is called unlocked and accesses closed without locking
• writesEnqueued is written to by executeWrite also without locking

In Java, volatile is expensive relative to the underlying atomic operations that CPUs can perform--it's like a half lock. If you made all of these values volatile it would actually be more expensive than just synchronizing. In Java synchronization is very cheap in the uncontended case, and this doesn't seem to be a class that should have to optimize for that.

My advice:

• don't use volatile; just use synchronization consistently
• acquire the lock nearer the API boundary so you're not acquiring it multiple times per private method used to implement the API call
• name your internal methods to show you assume the lock is held (e.g. verifyNotClosedLocked)
• avoid trying to be fancy with multithreading at all

That is to say, at a high level, the way to fix the problem with flush accessing lastOperation outside a synchronized block is to add a synchronized block. You should also fix the other unsynchonized accesses by adding synchronized blocks. Once you do, you'll notice you're synchronizing multiple times per API invocation, in which case you can hoist the synchronized blocks.

Also, see go/atomic-danger internally.

Thanks for the answer, very helpful. I will look at this a bit more tomorrow (and will likely add more synchronization).

To "defend" the current state of the PR: Both this.closed and this.writesEnqueued is only accessed from the user thread. bulkCommitBatch, lastOperation and rateLimiter are accessed both from the user thread and as a result of a BatchCommit response, in which case they are accessed from the BulkWriter thread.

Time to read go/atomic-danger

I updated this PR to use synchronization. PTAL.

Wow my understanding of Java volatile was very mis-guided. I just read https://developer.android.com/reference/java/util/concurrent/atomic/package-summary and the documents to which it links.

I learned that the following code could loop forever if this.done is not declared volatile:

while (!this.done) {
  Thread.sleep(100);
}

The compiler could choose to read this.done exactly once and re-use that cached value forever, even if another thread comes in and sets this.done to true. Declaring this.done as volatile would preclude this optimization and the thread would eventually see this.done set to true by another thread. I did not know this and have definitely coded some buggy busy waits in the past!

s/Bulk Writer/BulkWriter

Done

Since all accesses to writes enqueued are under lock, should we add a @GuardedBy annotation?

I removed them since this now applies to all state. Based on the Android link, I added them back. There is also some tooling that catches violations when GuardedBy object is accessed under lock (i.e. in Android Studio's build), so it is likely always better to add this annotations.