Modified base class for BrokerAppMetadata cache. Implemented as sing… #1638
Conversation
shoatman
commented
Nov 19, 2021
shoatman
commented
- Added ReentrantReadWriteLock to base class
- Added member variable to in-memory list to reduce the number of times reading from SharedPreferences and deserializing from JSON
- Added readLock around read operations
- Added writeLock around update/remove/clear operations
- Converted getAll to use a copy constructor and return a copy of the in-memory list/set
| @@ -78,7 +96,14 @@ public void remove(@NonNull final String clientId, | |||
| private void disposeOfDuplicateRecords(@NonNull final String clientId, | |||
| @NonNull final String environment, | |||
| final int uid) { | |||
| final List<BrokerApplicationMetadata> allMetadata = getAll(); | |||
| Set<BrokerApplicationMetadata> allMetadata = null; | |||
| readLock.lock(); | |||
There was a problem hiding this comment.
You'll need to hold the write lock for the entire duration of this activity. Read locks aren't exclusive.
There was a problem hiding this comment.
You're going to need to be far more pessimistic about your locking here. Read locks aren't exclusive, so you're not actually preventing concurrent, overlapping modification unless you hold the write lock. You have to hold that write lock through the entire read-modify-write operation unless you want to implement an optimistic locking optimization... which is almost surely unjustified.
…h was in effect insertOrUpdate
|
@tanmaymanolkar1 - Can you take a look at this PR as discussed and propose some tests for this work. Thanks |
|
Will have look at it @shoatman, thanks for working on this fix :) |
| if(INSTANCE == null){ | ||
| INSTANCE = new SharedPreferencesBrokerApplicationMetadataCache(context); | ||
| } |
There was a problem hiding this comment.
nit: consider lazy-holder pattern instead.
