[bisq-network/bisq] Make serialisation in FileManager::saveToFile thread-safe (#4025)

[Steven Barclay]

<!-- 
- make yourself familiar with the CONTRIBUTING.md if you have not already (https://github.com/bisq-network/bisq/blob/master/CONTRIBUTING.md)
- make sure you follow our [coding style guidelines][https://github.com/bisq-network/style/issues)
- pick a descriptive title
- provide some meaningful PR description below
- create the PR
- in case you receive a "Change request" and/or a NACK, please react within 30 days. If not, we will close your PR and it can not be up for compensation.
- After addressing the change request, __please re-request a review!__ Otherwise we might miss your PR as we tend to only look at pull requests tagged with a "review required".
-->

Add `toProtoMessageSynchronized()` default method to `PersistableEnvelope`, which performs (blocking) protobuf serialisation in the user thread, regardless of the calling thread. This should prevent data races like the `ConcurrentModificationException` observed in #3752, under the reasonable assumption that shared persistable objects are only mutated in the user thread.

Also add a `ThreadedPersistableEnvelope` sub-interface overriding the default method above, to let objects which are expensive to serialise (like `DaoStateStore`) be selectively serialised in the `save-file-task-X` thread as before, but directly synchronised with each mutating operation. As most objects are cheap to serialise, this avoids a noticeable performance drop without having to track down every mutating method for each store.

In all cases but one, classes implementing `ThreadedPersistableEnvelope` are stores like `TradeStatistic2Store`, with a single `ConcurrentHashMap` field. These require no further serialisation, since the map entries are immutable, so the only mutating operations are `map.put(..)` calls which are already synchronised with map reads. (Even if `map.values().stream()` sees updates at different keys happen out-of-order, it should be benign, e.g. trade statistics might be persisted out-of-order if two trades come along at almost exactly the same time.)

The remaining case is `DaoStateStore`, which is only ever reset or modified via a single `persist(..)` call with a cloned `DaoState` instance and hash chain from `DaoStateSnapshotService`, so there is no aliasing risk from the various DAO state mutations done in `DaoStateService` and elsewhere.

This should fix #3752.

--

Adding some temporary benchmark logging to `PersistableEnvelope` shows the time spent in each `saveToFile` protobuf conversion forced to run on the user thread with the PR changes, from my Bisq node (running on mainnet) from startup to shutdown:

[Serialisation_Benchmarks.txt](https://github.com/bisq-network/bisq/files/4286961/Serialisation_Benchmarks.txt)

As can be seen in the filtered log, the serialisation of `PreferencesPayload`, `UserPayload` and `TradableList` took 100-140ms (which might cause noticeable jitter on the user thread) the first time they were persisted upon application startup. However, subsequent serialisations of the same objects seem to occur much faster (presumably after the protobuf code has had a chance to warm up), with no (single-threaded) `PersistableEnvelope` object taking longer than around 20ms to serialise once the application has started.

The assumed expensive-to-serialise objects like `DaoStateStore` and those managed by `MapStoreService` all implement `ThreadedPersistableEnvelope`, so don't appear in the above benchmark and are converted directly on each `save-file-task-X` thread as before.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Make 'fromProto' static methods return most specific type
  * Make serialisation in FileManager::saveToFile thread-safe

-- File Changes --

    M common/src/main/java/bisq/common/proto/persistable/NavigationPath.java (2)
    M common/src/main/java/bisq/common/proto/persistable/PersistableEnvelope.java (14)
    A common/src/main/java/bisq/common/proto/persistable/ThreadedPersistableEnvelope.java (44)
    M common/src/main/java/bisq/common/storage/FileManager.java (2)
    M core/src/main/java/bisq/core/account/sign/SignedWitnessStore.java (6)
    M core/src/main/java/bisq/core/account/witness/AccountAgeWitnessStore.java (6)
    M core/src/main/java/bisq/core/dao/governance/blindvote/MyBlindVoteList.java (3)
    M core/src/main/java/bisq/core/dao/governance/blindvote/storage/BlindVoteStore.java (6)
    M core/src/main/java/bisq/core/dao/governance/myvote/MyVoteList.java (5)
    M core/src/main/java/bisq/core/dao/governance/proposal/storage/appendonly/ProposalStore.java (6)
    M core/src/main/java/bisq/core/dao/governance/proposal/storage/temp/TempProposalStore.java (6)
    M core/src/main/java/bisq/core/dao/state/DaoStateStorageService.java (6)
    M core/src/main/java/bisq/core/dao/state/DaoStateStore.java (6)
    M core/src/main/java/bisq/core/dao/state/unconfirmed/UnconfirmedBsqChangeOutputList.java (3)
    M core/src/main/java/bisq/core/payment/PaymentAccountList.java (3)
    M core/src/main/java/bisq/core/trade/statistics/TradeStatistics2Store.java (6)
    M core/src/main/java/bisq/core/user/PreferencesPayload.java (5)
    M p2p/src/main/java/bisq/network/p2p/peers/peerexchange/PeerList.java (3)
    M p2p/src/main/java/bisq/network/p2p/storage/persistence/PersistableNetworkPayloadList.java (12)
    M p2p/src/main/java/bisq/network/p2p/storage/persistence/SequenceNumberMap.java (6)

-- Patch Links --

https://github.com/bisq-network/bisq/pull/4025.patch
https://github.com/bisq-network/bisq/pull/4025.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/4025
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200304/70b18b71/attachment.html>