From 383f4f397b9be0e70c6a12c171c97678f80045dd Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Wed, 3 Jan 2018 17:21:09 -0800 Subject: [PATCH 1/2] Revert "Frameworks: Silently ignore InterruptedException" This reverts commit c8d5fc857208b08b984a802277807e9195b2f9a7. In preparation for different fix. Bug: 67986472 Bug: 70122540 Bug: 71533447 Test: m Test: Device boots Test: m cts && cts-tradefed run commandAndExit cts-dev --module CtsContentTestCases -c android.content.cts.SharedPreferencesTest Change-Id: I0b9e72d271725e15c20b68de981303c96ac1bd2a --- core/java/android/app/SharedPreferencesImpl.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/core/java/android/app/SharedPreferencesImpl.java b/core/java/android/app/SharedPreferencesImpl.java index 6dca4004cb46e..b18666613ea62 100644 --- a/core/java/android/app/SharedPreferencesImpl.java +++ b/core/java/android/app/SharedPreferencesImpl.java @@ -217,15 +217,10 @@ final class SharedPreferencesImpl implements SharedPreferences { } private @GuardedBy("mLock") Map getLoaded() { - // For backwards compatibility, we need to ignore any interrupts. b/70122540. - for (;;) { - try { - return mMap.get(); - } catch (ExecutionException e) { - throw new IllegalStateException(e); - } catch (InterruptedException e) { - // Ignore and try again. - } + try { + return mMap.get(); + } catch (InterruptedException | ExecutionException e) { + throw new IllegalStateException(e); } } private @GuardedBy("mLock") Map getLoadedWithBlockGuard() { From 158bde462e7b8b7b5061d343829bc04375ea736c Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Wed, 3 Jan 2018 17:22:35 -0800 Subject: [PATCH 2/2] Revert "Frameworks: Move SharedPreferencesImpl to Future" This reverts commit 70b600d45683b574104d10198da9bce49aa6be23. In preparation for a different fix. There are too many loopholes with updates vs commits. Bug: 67986472 Bug: 71533447 Test: m Test: Device boots Test: m cts && cts-tradefed run commandAndExit cts-dev --module CtsContentTestCases -c android.content.cts.SharedPreferencesTest Change-Id: I872a81ae1a26e1f77aad2a52daf88e093a686ec6 --- .../android/app/SharedPreferencesImpl.java | 128 ++++++++---------- 1 file changed, 54 insertions(+), 74 deletions(-) diff --git a/core/java/android/app/SharedPreferencesImpl.java b/core/java/android/app/SharedPreferencesImpl.java index b18666613ea62..8c47598fff34b 100644 --- a/core/java/android/app/SharedPreferencesImpl.java +++ b/core/java/android/app/SharedPreferencesImpl.java @@ -50,11 +50,6 @@ import java.util.Map; import java.util.Set; import java.util.WeakHashMap; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.Future; -import java.util.concurrent.FutureTask; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; final class SharedPreferencesImpl implements SharedPreferences { private static final String TAG = "SharedPreferencesImpl"; @@ -74,11 +69,15 @@ final class SharedPreferencesImpl implements SharedPreferences { private final Object mLock = new Object(); private final Object mWritingToDiskLock = new Object(); - private Future> mMap; + @GuardedBy("mLock") + private Map mMap; @GuardedBy("mLock") private int mDiskWritesInFlight = 0; + @GuardedBy("mLock") + private boolean mLoaded = false; + @GuardedBy("mLock") private StructTimespec mStatTimestamp; @@ -106,18 +105,27 @@ final class SharedPreferencesImpl implements SharedPreferences { mFile = file; mBackupFile = makeBackupFile(file); mMode = mode; + mLoaded = false; mMap = null; startLoadFromDisk(); } private void startLoadFromDisk() { - FutureTask> futureTask = new FutureTask<>(() -> loadFromDisk()); - mMap = futureTask; - new Thread(futureTask, "SharedPreferencesImpl-load").start(); + synchronized (mLock) { + mLoaded = false; + } + new Thread("SharedPreferencesImpl-load") { + public void run() { + loadFromDisk(); + } + }.start(); } - private Map loadFromDisk() { + private void loadFromDisk() { synchronized (mLock) { + if (mLoaded) { + return; + } if (mBackupFile.exists()) { mFile.delete(); mBackupFile.renameTo(mFile); @@ -150,14 +158,16 @@ final class SharedPreferencesImpl implements SharedPreferences { } synchronized (mLock) { + mLoaded = true; if (map != null) { + mMap = map; mStatTimestamp = stat.st_mtim; mStatSize = stat.st_size; } else { - map = new HashMap<>(); + mMap = new HashMap<>(); } + mLock.notifyAll(); } - return map; } static File makeBackupFile(File prefsFile) { @@ -216,37 +226,36 @@ final class SharedPreferencesImpl implements SharedPreferences { } } - private @GuardedBy("mLock") Map getLoaded() { - try { - return mMap.get(); - } catch (InterruptedException | ExecutionException e) { - throw new IllegalStateException(e); - } - } - private @GuardedBy("mLock") Map getLoadedWithBlockGuard() { - if (!mMap.isDone()) { + private void awaitLoadedLocked() { + if (!mLoaded) { // Raise an explicit StrictMode onReadFromDisk for this // thread, since the real read will be in a different // thread and otherwise ignored by StrictMode. BlockGuard.getThreadPolicy().onReadFromDisk(); } - return getLoaded(); + while (!mLoaded) { + try { + mLock.wait(); + } catch (InterruptedException unused) { + } + } } @Override public Map getAll() { - Map map = getLoadedWithBlockGuard(); synchronized (mLock) { - return new HashMap(map); + awaitLoadedLocked(); + //noinspection unchecked + return new HashMap(mMap); } } @Override @Nullable public String getString(String key, @Nullable String defValue) { - Map map = getLoadedWithBlockGuard(); synchronized (mLock) { - String v = (String) map.get(key); + awaitLoadedLocked(); + String v = (String)mMap.get(key); return v != null ? v : defValue; } } @@ -254,65 +263,66 @@ final class SharedPreferencesImpl implements SharedPreferences { @Override @Nullable public Set getStringSet(String key, @Nullable Set defValues) { - Map map = getLoadedWithBlockGuard(); synchronized (mLock) { - @SuppressWarnings("unchecked") - Set v = (Set) map.get(key); + awaitLoadedLocked(); + Set v = (Set) mMap.get(key); return v != null ? v : defValues; } } @Override public int getInt(String key, int defValue) { - Map map = getLoadedWithBlockGuard(); synchronized (mLock) { - Integer v = (Integer) map.get(key); + awaitLoadedLocked(); + Integer v = (Integer)mMap.get(key); return v != null ? v : defValue; } } @Override public long getLong(String key, long defValue) { - Map map = getLoadedWithBlockGuard(); synchronized (mLock) { - Long v = (Long) map.get(key); + awaitLoadedLocked(); + Long v = (Long)mMap.get(key); return v != null ? v : defValue; } } @Override public float getFloat(String key, float defValue) { - Map map = getLoadedWithBlockGuard(); synchronized (mLock) { - Float v = (Float) map.get(key); + awaitLoadedLocked(); + Float v = (Float)mMap.get(key); return v != null ? v : defValue; } } @Override public boolean getBoolean(String key, boolean defValue) { - Map map = getLoadedWithBlockGuard(); synchronized (mLock) { - Boolean v = (Boolean) map.get(key); + awaitLoadedLocked(); + Boolean v = (Boolean)mMap.get(key); return v != null ? v : defValue; } } @Override public boolean contains(String key) { - Map map = getLoadedWithBlockGuard(); synchronized (mLock) { - return map.containsKey(key); + awaitLoadedLocked(); + return mMap.containsKey(key); } } @Override public Editor edit() { - // TODO: remove the need to call getLoaded() when + // TODO: remove the need to call awaitLoadedLocked() when // requesting an editor. will require some work on the // Editor, but then we should be able to do: // // context.getSharedPreferences(..).edit().putString(..).apply() // // ... all without blocking. - getLoadedWithBlockGuard(); + synchronized (mLock) { + awaitLoadedLocked(); + } return new EditorImpl(); } @@ -466,43 +476,13 @@ final class SharedPreferencesImpl implements SharedPreferences { // a memory commit comes in when we're already // writing to disk. if (mDiskWritesInFlight > 0) { - // We can't modify our map as a currently + // We can't modify our mMap as a currently // in-flight write owns it. Clone it before // modifying it. // noinspection unchecked - mMap = new Future>() { - private Map mCopiedMap = - new HashMap(getLoaded()); - - @Override - public boolean cancel(boolean mayInterruptIfRunning) { - return false; - } - - @Override - public boolean isCancelled() { - return false; - } - - @Override - public boolean isDone() { - return true; - } - - @Override - public Map get() - throws InterruptedException, ExecutionException { - return mCopiedMap; - } - - @Override - public Map get(long timeout, TimeUnit unit) - throws InterruptedException, ExecutionException, TimeoutException { - return mCopiedMap; - } - }; + mMap = new HashMap(mMap); } - mapToWriteToDisk = getLoaded(); + mapToWriteToDisk = mMap; mDiskWritesInFlight++; boolean hasListeners = mListeners.size() > 0;