[bisq-network/bisq] (7/7) FileManager.java Cleanup and Audit (#3690)

[Julian Knutsen]

### Motivation
There was suspicion of a corruption bug inside FileManager. In order to unblock a review, I went through and audited the code and cleaned it up to make the concurrency more obvious.

At the end of the analysis, there was no corruption bug regarding the TODO in FileManager.java:75, but the interactions between the `persistable` field and the `savePending` were hard to understand. This has been updated with comments and an `AtomicReference` usage to make it more clear.

In some interleavings, two `writeToFile` calls for the same exact data could have occurred and this remains the case since references are passed in whose underlying data can change while `saveToFile` is running on another thread. That is why many implementations use a `Concurrent` data structure. Although, it is worth point out that not all do and it _could_ be the source of some strange bugs.

### Analysis
The comment was concerned with a situation where an in-progress `saveFileTask` would allow a UserThread `saveLater` call to schedule another write. There are two reasons this is OK here:

1) Reference writes are atomic
2) `saveToFile` is `synchronized`

In the event that an in-progress `saveLater` call spawned another `saveFileTask`, only one would be allowed to write the file at a time.

It is possible that the second call could write the file first, but since all callers share the same persistable reference, **_both_** `saveToFile` calls will write the latest data.


### Bug Fix
There was one strange behavior that was fixed in this PR. If the first `saveLater` call had a large delay it would override any future `saveLater` delays until the original one was finished. This was because the first `saveLater` set `savePending` so all future `saveLater` calls returned early without scheduling a thread.

The update causes all requests to spawn a task so if the second `saveLater` call has a shorter delay, it will run and batch with the first call. The second task will finally get scheduled and it will immediately exit since there is no work to do.

### Future Work
I think if the end goal is to have all writes completely thread-safe. You would need to do something like have `PersistableEnvelope` define a `clone()` function that all subclasses implement. The FileManager could then just clone the object prior to passing it off to the writing thread and have some guarantees.

That is way outside the scope of this work, but it may be a good piece for someone else to pick up.
You can view, comment on, or merge this pull request online at:

  https://github.com/bisq-network/bisq/pull/3690

-- Commit Summary --

  * [PR COMMENTS] Make maxSequenceNumberBeforePurge final
  * [TESTS] Clean up 'Analyze Code' warnings
  * [REFACTOR] HashMapListener::onAdded/onRemoved
  * [REFACTOR] removeFromMapAndDataStore can operate on Collections
  * Change removeFromMapAndDataStore to signal listeners at the end in a batch
  * Update removeExpiredEntries to remove all items in a batch
  * ProposalService::onProtectedDataRemoved signals listeners once on batch removes
  * Remove HashmapChangedListener::onBatch operations
  * [TESTS] Regression test for #3629
  * [BUGFIX] Reconstruct HashMap using 32-byte key
  * [BUGFIX] Use 32-byte key in requestData path
  * [DEAD CODE] Remove getProtectedDataStoreMap
  * [TESTS] Allow tests to validate SequenceNumberMap write separately
  * Implement remove-before-add message sequence behavior
  * [TESTS] Allow remove() verification to be more flexible
  * Broadcast remove-before-add messages to P2P network
  * [TESTS] Clean up remove verification helpers
  * [BUGFIX] Fix duplicate sequence number use case (startup)
  * Clean up AtomicBoolean usage in FileManager
  * [DEADCODE] Clean up FileManager.java
  * [BUGFIX] Shorter delay values not taking precedence
  * [REFACTOR] Inline saveNowInternal

-- File Changes --

    M common/src/main/java/bisq/common/storage/FileManager.java (59)
    M core/src/main/java/bisq/core/alert/AlertManager.java (32)
    M core/src/main/java/bisq/core/dao/governance/proposal/ProposalListPresentation.java (49)
    M core/src/main/java/bisq/core/dao/governance/proposal/ProposalService.java (65)
    M core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalStore.java (2)
    M core/src/main/java/bisq/core/filter/FilterManager.java (31)
    M core/src/main/java/bisq/core/offer/OfferBookService.java (37)
    M core/src/main/java/bisq/core/support/dispute/agent/DisputeAgentManager.java (23)
    A core/src/test/java/bisq/core/dao/governance/proposal/ProposalServiceP2PDataStorageListenerTest.java (127)
    M p2p/src/main/java/bisq/network/p2p/P2PModule.java (1)
    M p2p/src/main/java/bisq/network/p2p/P2PService.java (13)
    M p2p/src/main/java/bisq/network/p2p/peers/getdata/RequestDataHandler.java (2)
    M p2p/src/main/java/bisq/network/p2p/storage/HashMapChangedListener.java (14)
    M p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java (133)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java (8)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java (1)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java (120)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java (32)
    M p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoreDisconnectTest.java (7)
    M p2p/src/test/java/bisq/network/p2p/storage/TestState.java (191)
    M p2p/src/test/java/bisq/network/p2p/storage/mocks/ProtectedStoragePayloadStub.java (2)

-- Patch Links --

https://github.com/bisq-network/bisq/pull/3690.patch
https://github.com/bisq-network/bisq/pull/3690.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3690
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191125/d687e632/attachment.html>