diff --git a/common/src/androidTest/java/com/microsoft/identity/common/BrokerOAuth2TokenCacheTest.java b/common/src/androidTest/java/com/microsoft/identity/common/BrokerOAuth2TokenCacheTest.java index 4afe55f901..c32c0e280b 100644 --- a/common/src/androidTest/java/com/microsoft/identity/common/BrokerOAuth2TokenCacheTest.java +++ b/common/src/androidTest/java/com/microsoft/identity/common/BrokerOAuth2TokenCacheTest.java @@ -129,7 +129,7 @@ public void setUp() throws Exception { // Our test context final Context context = androidx.test.platform.app.InstrumentationRegistry.getInstrumentation().getTargetContext(); - mApplicationMetadataCache = new SharedPreferencesBrokerApplicationMetadataCache(context); + mApplicationMetadataCache = SharedPreferencesBrokerApplicationMetadataCache.getInstance(context); // Test Configs for caches... initFociCache(context); @@ -888,7 +888,7 @@ public MsalOAuth2TokenCache getTokenCache(Context context, int bindingProcessUid final BrokerOAuth2TokenCache brokerOAuth2TokenCache = new BrokerOAuth2TokenCache( context, TEST_APP_UID, - new SharedPreferencesBrokerApplicationMetadataCache(context) + SharedPreferencesBrokerApplicationMetadataCache.getInstance(context) ); assertEquals( @@ -899,7 +899,7 @@ public MsalOAuth2TokenCache getTokenCache(Context context, int bindingProcessUid final BrokerOAuth2TokenCache brokerOAuth2TokenCache2 = new BrokerOAuth2TokenCache( context, TEST_APP_UID, - new SharedPreferencesBrokerApplicationMetadataCache(context) + SharedPreferencesBrokerApplicationMetadataCache.getInstance(context) ); assertEquals( diff --git a/common/src/androidTest/java/com/microsoft/identity/common/SharedPreferencesBrokerApplicationMetadataCacheTest.java b/common/src/androidTest/java/com/microsoft/identity/common/SharedPreferencesBrokerApplicationMetadataCacheTest.java index 4d6edeae63..343787c028 100644 --- a/common/src/androidTest/java/com/microsoft/identity/common/SharedPreferencesBrokerApplicationMetadataCacheTest.java +++ b/common/src/androidTest/java/com/microsoft/identity/common/SharedPreferencesBrokerApplicationMetadataCacheTest.java @@ -25,32 +25,52 @@ import androidx.test.InstrumentationRegistry; import androidx.test.ext.junit.runners.AndroidJUnit4; +import com.android.dx.rop.code.Exceptions; import com.microsoft.identity.common.internal.cache.BrokerApplicationMetadata; import com.microsoft.identity.common.internal.cache.IBrokerApplicationMetadataCache; import com.microsoft.identity.common.internal.cache.SharedPreferencesBrokerApplicationMetadataCache; +import com.microsoft.identity.common.logging.Logger; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Random; +import java.util.Set; import java.util.UUID; +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import static junit.framework.TestCase.assertTrue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; @RunWith(AndroidJUnit4.class) public class SharedPreferencesBrokerApplicationMetadataCacheTest { - private IBrokerApplicationMetadataCache mMetadataCache; + public static final String M_CLIENT_ID = UUID.randomUUID().toString(); + public static final String M_ENVIRONMENT = UUID.randomUUID().toString(); + public static final int M_UID = new Random().nextInt(); + public static final int fixedIteration = 1000000; + private SharedPreferencesBrokerApplicationMetadataCache mMetadataCache; @Before public void setUp() { - mMetadataCache = new SharedPreferencesBrokerApplicationMetadataCache( + mMetadataCache = SharedPreferencesBrokerApplicationMetadataCache.getInstance( InstrumentationRegistry.getContext() ); } @@ -86,6 +106,134 @@ public void testInsert() { ); } + @Test + public void testMultipleInsertOrUpdate() throws Exception { + final ExecutorService executor = Executors.newFixedThreadPool(10); + final CountDownLatch latch = new CountDownLatch(1); + final CountDownLatch executionLatch = new CountDownLatch(10); + final Set set = Collections.synchronizedSet(new HashSet()); + List> list = new ArrayList<>(); + for (int i = 0; i <10; i++){ + list.add(executor.submit(new Callable() { + @Override + public Boolean call() throws Exception { + BrokerApplicationMetadata metadata = generateDuplicateMetadata(); + set.add(metadata); + return insertOrUpdateFutureHelper(metadata, latch, executionLatch); + } + })); + } + executionLatch.await(); + latch.countDown(); + + for(Future f : list){ + Assert.assertTrue(f.get()); + } + + final int size = mMetadataCache.getAll().size(); + + assertEquals("Expected 1 record, but got too many", + 1, + size + ); + + Assert.assertTrue(set.contains(mMetadataCache.getAll().get(0))); + } + + public boolean insertOrUpdateFutureHelper + (BrokerApplicationMetadata metadata, CountDownLatch latch, CountDownLatch executionLatch) + throws InterruptedException { + executionLatch.countDown(); + latch.await(); + return mMetadataCache.insertOrUpdate(metadata); + } + + public boolean insertOrUpdateHelper + (BrokerApplicationMetadata metadata, CountDownLatch latch, CountDownLatch executionLatch) + throws InterruptedException { + executionLatch.countDown(); + latch.await(); + return mMetadataCache.insertOrUpdate(metadata); + } + + @Test + public void testReadsWithInsertOrUpdate() throws Exception { + final String methodName = ":testReadsWithInsertOrUpdate"; + + final ExecutorService executor = Executors.newFixedThreadPool(10); + final CountDownLatch latch = new CountDownLatch(1); + final CountDownLatch executionWriteLatch = new CountDownLatch(5); + final CountDownLatch executionReadLatch = new CountDownLatch(5); + final AtomicInteger iterations = new AtomicInteger(0); + final AtomicBoolean failCheck = new AtomicBoolean(true); + final AtomicInteger reads = new AtomicInteger(0); + final AtomicInteger writes = new AtomicInteger(0); + + mMetadataCache.clear(); + //inserting one entry initially + mMetadataCache.insertOrUpdate(generateDuplicateMetadata()); + + assertEquals(1, mMetadataCache.getAll().size()); + + for (int i = 0; i <5; i++){ + executor.submit(new Runnable() { + @Override + public void run() { + while (iterations.getAndIncrement() < fixedIteration) { + BrokerApplicationMetadata metadata = generateDuplicateMetadata(); + try { + if (!insertOrUpdateHelper(metadata, latch, executionWriteLatch)){ + failCheck.set(false); + } + } catch (InterruptedException e) { + e.printStackTrace(); + } + writes.getAndIncrement(); + } + } + }); + + executor.submit(new Runnable() { + @Override + public void run() { + while (iterations.getAndIncrement() < fixedIteration) { + try { + if (!readsWithInsertOrUpdateHelper(latch, executionReadLatch)){ + failCheck.set(false); + } + } catch (InterruptedException e) { + e.printStackTrace(); + } + reads.getAndIncrement(); + } + } + }); + } + executionWriteLatch.await(); + executionReadLatch.await(); + latch.countDown(); + executor.shutdown(); + executor.awaitTermination(1, TimeUnit.HOURS); + + final int size = mMetadataCache.getAll().size(); + + assertEquals("Expected 1 record, but we got : " + size, + 1, + size + ); + + Assert.assertTrue(failCheck.get()); + Logger.info(methodName,"Reads to Writes Ratio : " + reads + ":" + writes); + } + + public boolean readsWithInsertOrUpdateHelper + (CountDownLatch latch, CountDownLatch executionLatch) + throws InterruptedException { + executionLatch.countDown(); + latch.await(); + return mMetadataCache.getAll().size() == 1; + } + @Test public void testInsertMultiple() { final int expected = 10; @@ -226,4 +374,15 @@ private static BrokerApplicationMetadata generateRandomMetadata() { return randomMetadata; } + + private static BrokerApplicationMetadata generateDuplicateMetadata() { + final BrokerApplicationMetadata randomMetadata = new BrokerApplicationMetadata(); + + randomMetadata.setClientId(M_CLIENT_ID); + randomMetadata.setEnvironment(M_ENVIRONMENT); + randomMetadata.setFoci(UUID.randomUUID().toString()); + randomMetadata.setUid(M_UID); + + return randomMetadata; + } } diff --git a/common/src/main/java/com/microsoft/identity/common/internal/cache/BrokerOAuth2TokenCache.java b/common/src/main/java/com/microsoft/identity/common/internal/cache/BrokerOAuth2TokenCache.java index 3b42f8964c..f19bcb2099 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/cache/BrokerOAuth2TokenCache.java +++ b/common/src/main/java/com/microsoft/identity/common/internal/cache/BrokerOAuth2TokenCache.java @@ -523,7 +523,8 @@ private void updateApplicationMetadataCache(@NonNull final String clientId, + "]" ); - final boolean success = mApplicationMetadataCache.insert(applicationMetadata); + //Insert or update was added to ensure that update operation was atomic + final boolean success = mApplicationMetadataCache.insertOrUpdate(applicationMetadata); Logger.info( TAG + methodName, diff --git a/common/src/main/java/com/microsoft/identity/common/internal/cache/IBrokerApplicationMetadataCache.java b/common/src/main/java/com/microsoft/identity/common/internal/cache/IBrokerApplicationMetadataCache.java index 64832de933..d619cc52a1 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/cache/IBrokerApplicationMetadataCache.java +++ b/common/src/main/java/com/microsoft/identity/common/internal/cache/IBrokerApplicationMetadataCache.java @@ -59,4 +59,6 @@ public interface IBrokerApplicationMetadataCache extends ISimpleCache implements IBrokerApplicationMetadataCache { + private static SharedPreferencesBrokerApplicationMetadataCache INSTANCE; + private static final String TAG = SharedPreferencesBrokerApplicationMetadataCache.class.getSimpleName(); private static final String DEFAULT_APP_METADATA_CACHE_NAME = "com.microsoft.identity.app-meta-cache"; private static final String KEY_CACHE_LIST = "app-meta-cache"; - public SharedPreferencesBrokerApplicationMetadataCache(@NonNull final Context context) { - super(context, DEFAULT_APP_METADATA_CACHE_NAME, KEY_CACHE_LIST); + public synchronized static SharedPreferencesBrokerApplicationMetadataCache getInstance(@NonNull final Context context){ + if(INSTANCE == null){ + INSTANCE = new SharedPreferencesBrokerApplicationMetadataCache(context); + } + + return INSTANCE; } - @Override - public boolean insert(@NonNull final BrokerApplicationMetadata brokerApplicationMetadata) { - // Before we insert a record, if we have a existing entry with the same client id, env, and - // processUid, we should remove that entry... - disposeOfDuplicateRecords( - brokerApplicationMetadata.getClientId(), - brokerApplicationMetadata.getEnvironment(), - brokerApplicationMetadata.getUid() - ); - return super.insert(brokerApplicationMetadata); + protected SharedPreferencesBrokerApplicationMetadataCache(@NonNull final Context context) { + super(context, DEFAULT_APP_METADATA_CACHE_NAME, KEY_CACHE_LIST); } public void remove(@NonNull final String clientId, final int processUid) { - final List allMetadata = getAll(); + Set allMetadata = null; + readLock.lock(); + try { + //Get a copy of list first + allMetadata = new HashSet(mList); + }finally{ + readLock.unlock(); + } for (final BrokerApplicationMetadata metadata : allMetadata) { if (clientId.equalsIgnoreCase(metadata.getClientId()) @@ -78,7 +84,10 @@ public void remove(@NonNull final String clientId, private void disposeOfDuplicateRecords(@NonNull final String clientId, @NonNull final String environment, final int uid) { - final List allMetadata = getAll(); + Set allMetadata = null; + + //Get a copy of list first + allMetadata = new HashSet(mList); for (final BrokerApplicationMetadata metadata : allMetadata) { if (clientId.equalsIgnoreCase(metadata.getClientId()) @@ -95,8 +104,13 @@ public Set getAllClientIds() { final Set allClientIds = new HashSet<>(); - for (final BrokerApplicationMetadata metadata : getAll()) { - allClientIds.add(metadata.getClientId()); + readLock.lock(); + try { + for (final BrokerApplicationMetadata metadata : mList) { + allClientIds.add(metadata.getClientId()); + } + }finally{ + readLock.unlock(); } Logger.verbose( @@ -121,19 +135,24 @@ public Set getAllNonFociClientIds() { @Override public List getAllFociApplicationMetadata() { - final Set fociClientIds = getAllFociClientIds(); - final List result = new ArrayList<>(); - final List allMetadata = getAll(); + //The following method has it's own read locks... don't want to nest + final Set fociClientIds = getAllFociClientIds(); - for (final BrokerApplicationMetadata metadata : allMetadata) { - if (fociClientIds.contains(metadata.getClientId())) { - result.add(metadata); + readLock.lock(); + try { + for (final BrokerApplicationMetadata metadata : mList) { + if (fociClientIds.contains(metadata.getClientId())) { + result.add(metadata); + } } + }finally{ + readLock.unlock(); } return result; + } /** @@ -147,16 +166,22 @@ private Set getAllFociClientIds(final boolean inverseMatch) { final Set allFociClientIds = new HashSet<>(); - for (final BrokerApplicationMetadata metadata : getAll()) { - if (!inverseMatch) { // match FoCI - if (!TextUtils.isEmpty(metadata.getFoci())) { - allFociClientIds.add(metadata.getClientId()); - } - } else { // match non FoCI - if (TextUtils.isEmpty(metadata.getFoci())) { - allFociClientIds.add(metadata.getClientId()); + readLock.lock(); + try { + + for (final BrokerApplicationMetadata metadata : mList) { + if (!inverseMatch) { // match FoCI + if (!TextUtils.isEmpty(metadata.getFoci())) { + allFociClientIds.add(metadata.getClientId()); + } + } else { // match non FoCI + if (TextUtils.isEmpty(metadata.getFoci())) { + allFociClientIds.add(metadata.getClientId()); + } } } + }finally{ + readLock.unlock(); } Logger.verbose( @@ -176,21 +201,25 @@ public BrokerApplicationMetadata getMetadata(@NonNull final String clientId, final int processUid) { final String methodName = ":getMetadata"; - final List allMetadata = getAll(); BrokerApplicationMetadata result = null; - for (final BrokerApplicationMetadata metadata : allMetadata) { - if (clientId.equals(metadata.getClientId()) - && environment.equals(metadata.getEnvironment()) - && processUid == metadata.getUid()) { - Logger.verbose( - TAG + metadata, - "Metadata located." - ); - - result = metadata; - break; + readLock.lock(); + try { + for (final BrokerApplicationMetadata metadata : mList) { + if (clientId.equals(metadata.getClientId()) + && environment.equals(metadata.getEnvironment()) + && processUid == metadata.getUid()) { + Logger.verbose( + TAG + metadata, + "Metadata located." + ); + + result = metadata; + break; + } } + }finally{ + readLock.unlock(); } if (null == result) { @@ -207,6 +236,21 @@ public BrokerApplicationMetadata getMetadata(@NonNull final String clientId, return result; } + @Override + public boolean insertOrUpdate(BrokerApplicationMetadata metadata) { + writeLock.lock(); + try { + disposeOfDuplicateRecords( + metadata.getClientId(), + metadata.getEnvironment(), + metadata.getUid() + ); + return super.insert(metadata); + }finally{ + writeLock.unlock(); + } + } + @Override protected Type getListTypeToken() { return new TypeToken>() { diff --git a/common/src/main/java/com/microsoft/identity/common/internal/cache/SharedPreferencesSimpleCacheImpl.java b/common/src/main/java/com/microsoft/identity/common/internal/cache/SharedPreferencesSimpleCacheImpl.java index 6c8491e24f..313b36758d 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/cache/SharedPreferencesSimpleCacheImpl.java +++ b/common/src/main/java/com/microsoft/identity/common/internal/cache/SharedPreferencesSimpleCacheImpl.java @@ -26,14 +26,18 @@ import android.content.SharedPreferences; import androidx.annotation.NonNull; +import androidx.annotation.RequiresPermission; import com.google.gson.Gson; import com.microsoft.identity.common.logging.Logger; import java.lang.reflect.Type; +import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantReadWriteLock; /** * A simple metadata store definition that uses SharedPreferences to persist, read, update, and @@ -53,7 +57,15 @@ public abstract class SharedPreferencesSimpleCacheImpl implements ISimpleCach private final String mKeySingleEntry; private final Gson mGson = new Gson(); - public SharedPreferencesSimpleCacheImpl(@NonNull final Context context, + //ReentrantReadWriteLock - https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html + protected final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + protected final Lock readLock = readWriteLock.readLock(); + protected final Lock writeLock = readWriteLock.writeLock(); + + //In memory copy of the cached list + protected List mList = null; + + protected SharedPreferencesSimpleCacheImpl(@NonNull final Context context, @NonNull final String prefsName, @NonNull final String singleKey) { Logger.verbose( @@ -65,6 +77,8 @@ public SharedPreferencesSimpleCacheImpl(@NonNull final Context context, Context.MODE_PRIVATE ); mKeySingleEntry = singleKey; + //Implementations of this class are singletons.... and construction is synchronized + mList = load(); } /** @@ -78,31 +92,42 @@ public SharedPreferencesSimpleCacheImpl(@NonNull final Context context, public boolean insert(T t) { final String methodName = ":insert"; - final Set allMetadata = new HashSet<>(getAll()); - Logger.verbose( - TAG + methodName, - "Existing metadata contained [" - + allMetadata.size() - + "] elements." - ); + boolean success = false; - allMetadata.add(t); + writeLock.lock(); - Logger.verbose( - TAG + methodName, - "New metadata set size: [" - + allMetadata.size() - + "]" - ); + try { - final String json = mGson.toJson(allMetadata); + final Set allMetadata = new HashSet<>(mList); + Logger.verbose( + TAG + methodName, + "Existing metadata contained [" + + allMetadata.size() + + "] elements." + ); - Logger.verbose( - TAG + methodName, - "Writing cache entry." - ); + allMetadata.add(t); + + Logger.verbose( + TAG + methodName, + "New metadata set size: [" + + allMetadata.size() + + "]" + ); - final boolean success = mSharedPrefs.edit().putString(mKeySingleEntry, json).commit(); + final String json = mGson.toJson(allMetadata); + + Logger.verbose( + TAG + methodName, + "Writing cache entry." + ); + + success = mSharedPrefs.edit().putString(mKeySingleEntry, json).commit(); + //Put the updated list in memory + mList = load(); + }finally { + writeLock.unlock(); + } if (success) { Logger.verbose( @@ -122,62 +147,83 @@ public boolean insert(T t) { @Override public boolean remove(T t) { final String methodName = ":remove"; + boolean removed = false; - final Set allMetadata = new HashSet<>(getAll()); - - Logger.verbose( - TAG + methodName, - "Existing metadata contained [" - + allMetadata.size() - + "] elements." - ); - - final boolean removed = allMetadata.remove(t); - - Logger.verbose( - TAG + methodName, - "New metadata set size: [" - + allMetadata.size() - + "]" - ); - - if (!removed) { - // Nothing to do, wasn't cached in the first place! - Logger.warn( - TAG + methodName, - "Nothing to delete -- cache entry is missing!" - ); + writeLock.lock(); - return true; - } else { - final String json = mGson.toJson(allMetadata); + try { + final Set allMetadata = new HashSet<>(mList); Logger.verbose( TAG + methodName, - "Writing new cache values..." + "Existing metadata contained [" + + allMetadata.size() + + "] elements." ); - final boolean written = mSharedPrefs.edit().putString(mKeySingleEntry, json).commit(); + removed = allMetadata.remove(t); Logger.verbose( TAG + methodName, - "Updated cache contents written? [" - + written + "New metadata set size: [" + + allMetadata.size() + "]" ); - return written; + if (!removed) { + // Nothing to do, wasn't cached in the first place! + Logger.warn( + TAG + methodName, + "Nothing to delete -- cache entry is missing!" + ); + + removed = true; + } else { + final String json = mGson.toJson(allMetadata); + + Logger.verbose( + TAG + methodName, + "Writing new cache values..." + ); + + removed = mSharedPrefs.edit().putString(mKeySingleEntry, json).commit(); + + Logger.verbose( + TAG + methodName, + "Updated cache contents written? [" + + removed + + "]" + ); + + } + //Put the updated list in memory + mList = load(); + }finally { + writeLock.unlock(); } + + return removed; } - @Override - public List getAll() { - final String methodName = ":getAll"; + public List getAll(){ + //Return copy of list + readLock.lock(); + try { + return new ArrayList(mList); + } finally { + readLock.unlock(); + } + } + + protected List load() { + final String methodName = ":load"; + List result = null; + final String jsonList = mSharedPrefs.getString(mKeySingleEntry, EMPTY_ARRAY); final Type listType = getListTypeToken(); - final List result = mGson.fromJson( + result = mGson.fromJson( jsonList, listType ); @@ -195,8 +241,17 @@ public List getAll() { @Override public boolean clear() { final String methodName = ":clear"; - - final boolean cleared = mSharedPrefs.edit().clear().commit(); + boolean cleared = false; + + writeLock.lock(); + try { + //Clear disk + cleared = mSharedPrefs.edit().clear().commit(); + //clear memory + mList.clear(); + }finally{ + writeLock.unlock(); + } if (!cleared) { Logger.warn( diff --git a/common/src/main/java/com/microsoft/identity/common/internal/cache/registry/DefaultBrokerApplicationRegistry.java b/common/src/main/java/com/microsoft/identity/common/internal/cache/registry/DefaultBrokerApplicationRegistry.java index 07040fcdbd..e62fb007e7 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/cache/registry/DefaultBrokerApplicationRegistry.java +++ b/common/src/main/java/com/microsoft/identity/common/internal/cache/registry/DefaultBrokerApplicationRegistry.java @@ -64,6 +64,7 @@ public BrokerApplicationRegistryData getMetadata(@NonNull final String clientId, final String methodName = ":getMetadata"; final List allMetadata = getAll(); + BrokerApplicationRegistryData result = null; for (final BrokerApplicationRegistryData metadata : allMetadata) { diff --git a/common/versioning/version.properties b/common/versioning/version.properties index 77a1b347a4..f30c1ed99b 100644 --- a/common/versioning/version.properties +++ b/common/versioning/version.properties @@ -1,4 +1,4 @@ #Tue Apr 06 22:55:08 UTC 2021 -versionName=3.6.3 +versionName=3.6.3-CacheSync-1 versionCode=1 latestPatchVersion=180