Issue 6683: LTS - Fix thread visibility issues

[sachin-j-joshi]

Signed-off-by: Sachin Joshi sachin.joshi@emc.com

Change log description
Issue 6683: LTS - Fix possible thread visibility issue with SystemJournal serialization.

Purpose of the change
Fixes #6683

What the code does

1. The default RevisionDataInput::readCollection returns ArrayList which is not thread safe. Return Vector instead.
2. Replace problematic Collections.synchronizedList(new ArrayList<ExtendedChunkInfo>()); with new Vector() in all SLTS code
3. Make use of iterator with Futures.loop more thread-safe

Note -

• It is possible in rare case that the data updated by one thread is not visible to another thread. This can lead to validation failure for snapshot.
• On my machine I can repro about 1 in 10000 times. Fix is to use synchronized list (eg. Vector).
• This kind of error does not mean there is a race to read/update same variable at the same time from multiple threads, instead it means that when different lamdas are executed serially one by one they can be executed on different thread and potentially different processor cores. That means data cached in first threads processor is not yet flushed to main memory, so second processor reads outdated value from main memory. By using volatile fields (via Vector) data guaranteed to be flushed correctly.

How to verify it
All tests should pass.

[codecov]

Codecov Report

Merging #6687 (f465f9b) into master (0205da8) will increase coverage by 0.00%.
The diff coverage is 97.29%.

@@            Coverage Diff            @@
##             master    #6687   +/-   ##
=========================================
  Coverage     86.35%   86.36%           
- Complexity    15805    15808    +3     
=========================================
  Files          1024     1024           
  Lines         59036    59035    -1     
  Branches       5968     5967    -1     
=========================================
+ Hits          50979    50984    +5     
+ Misses         4933     4925    -8     
- Partials       3124     3126    +2     

Impacted Files	Coverage Δ
...segmentstore/storage/chunklayer/SystemJournal.java	87.70% <96.29%> (+0.11%)	⬆️
...gmentstore/storage/chunklayer/ConcatOperation.java	97.22% <100.00%> (ø)
...tstore/storage/chunklayer/DefragmentOperation.java	92.55% <100.00%> (ø)
...segmentstore/storage/chunklayer/ReadOperation.java	88.78% <100.00%> (ø)
...entstore/storage/chunklayer/TruncateOperation.java	94.71% <100.00%> (ø)
.../segmentstore/storage/chunklayer/UtilsWrapper.java	97.01% <100.00%> (ø)
...egmentstore/storage/chunklayer/WriteOperation.java	94.19% <100.00%> (ø)
...hared/security/crypto/StrongPasswordProcessor.java	87.50% <0.00%> (-6.25%)	⬇️
...gmentstore/server/tables/WriterTableProcessor.java	86.45% <0.00%> (-1.05%)	⬇️
...segmentstore/server/logs/DataFrameInputStream.java	78.57% <0.00%> (-0.80%)	⬇️
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0205da8...f465f9b. Read the comment docs.

[sachin-j-joshi]

[RaulGracia]

Do you think that there could be other points in SLTS that may suffer from the same issue? Also, please run system tests on this change.

[sachin-j-joshi]

We did fix several of these during beta testing - #5665
Looks like I still missed a couple places back then. I'll double check if there is anything else I missed.

[RaulGracia]

@sachin-j-joshi apart from running system tests, is there anything missing from this PR? I assume you have synced up with @tkaitchuck on this change, right?

[sachin-j-joshi]

Putting it in draft mode -
@tkaitchuck suggested to change it to use Vector instead of Collections.synchronizedList(new ArrayList<>())
But I was able to still repro the failures after I changed it to Vector. So there is still something else that is missing.

[tkaitchuck]

Approving with comments

[sachin-j-joshi]

System tests test-pravega-continuous-1/2136/ passed

[RaulGracia]

Couple of minor questions before merging.