Fix data race in IgnoredMailboxMap
#7100
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make the 'dataMap' field a ConcurrentHashMap instead of a HashMap, to prevent a ConcurrentModificationException, which was recently observed when calling 'IgnoredMailboxMap::toProtoMessage' from the persistence manager inside the user thread during startup of bisq-desktop. Tracing through the code, this likely happened during an iteration through the keyset of 'dataMap' (to check for nulls, inside the proto serialisation logic during persistence), while simultaneously adding ignored mailbox messages to the map on a separate thread spawned from the method, 'MailboxMessageService::threadedBatchProcessMailboxEntries'. (The latter was made asynchronous so as not to block the UI.) (Since there is a call to 'PersistenceManager::requestPersistence' after every addition to the ignored mailbox map, there hopefully won't be any missed entries in the final persisted map, even though the snapshot of ConcurrentHashMap being iterated through won't be fresh as long as new entries are simultaneously added.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make the
dataMapfield ofIgnoredMailboxMapaConcurrentHashMapinstead of aHashMap, to prevent aConcurrentModificationException, which was recently observed when callingIgnoredMailboxMap::toProtoMessagefrom the persistence manager, inside the user thread during startup of bisq-desktop. Tracing through the code, this likely happened during an iteration through the keyset ofdataMap(to check for nulls, inside the proto serialisation logic during persistence), while simultaneously adding ignored mailbox messages to the map on a separate thread spawned from the method,MailboxMessageService::threadedBatchProcessMailboxEntries. (The latter was made asynchronous a long time ago, so as not to block the UI.)(Since there is a call to
PersistenceManager::requestPersistenceafter every addition to the ignored mailbox map, there hopefully won't be any missed entries in the final persisted map, even though the snapshot ofConcurrentHashMapbeing iterated through won't be fresh as long as new entries are simultaneously added.)--
The following intermittent error (which is possibly made more likely by application suspension part way through startup) was last seen in my logs a few weeks ago, during startup of the desktop app, which should hopefully be fixed by this PR: