Fix race condition in ExponentiallyDecayingReservoir's rescale method

[ho3rexqj]

Fixes issues #1005 and #793 - note that in #793 the longPeriodsOfInactivityShouldNotCorruptSamplingState test did not cover multiple concurrent requests after a long idle period.

The race condition currently occurs when two threads update the reservoir concurrently - after the first thread has successfully updated nextScaleTime (line 162) but before the write lock is obtained (line 163) a second thread updates the reservoir (line 95+). With several threads pushing updates concurrently this presents a narrow window during which the second thread may obtain the lock before the first resulting in a corrupt reservoir state. Note, however, that if a snapshot is requested just prior to the two concurrent updates the read lock will already be active when the two threads enter rescaleIfNeeded, resulting in a much larger window for this race condition.

[arteam]

Thank you very much for the fix and very detailed unit test!

[ryantenney]

I have an issue with this. The compareAndSet operation is atomic so it shouldn't allow more than one thread into that if block. Granted 'lockForRescale()' does effectively the same thing, however it is a more expensive operation than the compareAndSet.

[arteam]

The issue as I see it:

• Thread 1 calls update
• Thread 2 calls update
• Thread 1 calls rescaleIfNeeded
• Thread 2 calls rescaleIfNeeded
• Thread 1 reads the CAS and updates it
• Thread 1 gets swapped out
• Thread 2 reads the updated CAS and exits
• Thread 2 obtains a read lock and works with a new weight, but old (not scaled) values
• Thread 1 gets swapped in and acquires a write lock, but it's too late

Basically, the CAS only guarantees that nextScaleTime gets updated atomically, but not other reservoir's state.

[arteam]

In case the fix the correct, I would like to cherry-pick this change to the 3.1.3-maintenance branch to provide users who affected by this bug an easy path to upgrade.

[arteam]

I didn't get more reasons opposing this fix, so I'm going to move this bugfix to the 3.1.3 branch. In the worst case, we have a performance regression, in the best we fix a concurrency bug.

[ahadadi]

@arteam, I think your comment:
"Thread 2 obtains a read lock and works with a new weight, but old (not scaled) values"
is incorrect.
The weight becomes new when startTime gets updated, but it gets updated only under the write lock by thread 1.
I still think this is a good change, as it simplifies reasoning about the code.