Avoid using iterator to access ArrayBuffer that can be modified during iteration

[lbulej]

The switch to Scala 2.13 apparently unearthed an issue that has been present in finagle-chirper all the time. The code changes in finagle-chirper between Renaissance 0.13 and Renaissance 0.14 were only meant to "port" it to Scala 2.13 and required changing the signature of various methods that compute feed stats to accept a SeqView instead of Seq (because in Scala 2.13, a non-strict SeqView is no longer a subtype of a strict Seq), but even in Scala 2.12, the view method of an ArrayBuffer returned a SeqView.

The main cause appears to be the fact that the analyze() method accesses an ArrayBuffer (representing a per-user feed) without a lock. The feed is appended to under a lock in the Master, but no lock is held when calculating stats on individual feeds (in a Future). When processing a request for feed stats, the lock is held while creating a read only snapshot of the trie that holds all user feeds and a view is created for each feed (which essentially snapshots the number of items in the feed), but the view is processed later.

The analyze() function running as part of the future evaluation uses an iterator to go over chirps in a feed, but the underlying ArrayBuffer can be modified (appended to) in the meantime. In Scala 2.13 this results in the ConcurrentModificationException. I tested the same code in Scala 2.12 (with the same libraries) and I was not able to reproduce the problem.

Now, I believe that the race is intentional and should be benign (similar to updating profiling counters in hotspot), so to keep it the way it was, I just changed the analyze() method to avoid using an iterator and instead iterate over a Range and then access the ArrayBuffer elements by index. This should be "safe" even if the ArrayBuffer is expanded (and the backing array changed) because the ArrayBuffer is only appended to. Using a direct index should either fetch the element from an old array or the new array, but the returned item should be the same — assuming the implementation updates the reference to the backing array after it has copied the contents of the old array to the new array.

• Strictly speaking, I'm not sure whether even this would hold under the memory model, but I understand the desire to avoid locking for these computations — the whole point of the stats is to provide some reasonable numbers at some point in time and if some updates come in between, they will be included in the next stats call.

Fixes #354 (as long as keeping the race can be considered a fix)

[farquet]

Very nice analysis!
I agree that it makes sense not to add locking for a stats feature. Your fix is probably how one would want to solve this in an actual production workload.
I've tested your branch locally and numbers are comparable with renaissance 0.14 as expected.