Use ReentrantReadWriteLock in ExpandableBinaryDictionary.

Bug: 8187060
Change-Id: Ic665f0a5e940708aa9addedac068a64900b307e7

java/src/com/android/inputmethod/latin/ContactsBinaryDictionary.java

@@ -87,9 +87,6 @@ public class ContactsBinaryDictionary extends ExpandableBinaryDictionary {
         mLocale = locale;
         mUseFirstLastBigrams = useFirstLastBigramsForLocale(locale);
         registerObserver(context);
-
-        // Load the current binary dictionary from internal storage. If no binary dictionary exists,
-        // reloadDictionaryIfRequired will start a new thread to generate one asynchronously.
         reloadDictionaryIfRequired();
     }
 

java/src/com/android/inputmethod/latin/ExpandableBinaryDictionary.java

@@ -26,7 +26,6 @@ import com.android.inputmethod.latin.makedict.FormatSpec;
 import com.android.inputmethod.latin.makedict.UnsupportedFormatException;
 import com.android.inputmethod.latin.makedict.WordProperty;
 import com.android.inputmethod.latin.SuggestedWords.SuggestedWordInfo;
-import com.android.inputmethod.latin.utils.AsyncResultHolder;
 import com.android.inputmethod.latin.utils.CombinedFormatUtils;
 import com.android.inputmethod.latin.utils.ExecutorUtils;
 import com.android.inputmethod.latin.utils.FileUtils;
@@ -40,6 +39,8 @@ import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
  * Abstract base class for an expandable dictionary that can be created and updated dynamically
@@ -59,7 +60,6 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
     private static final boolean DBG_STRESS_TEST = false;
 
     private static final int TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS = 100;
-    private static final int TIMEOUT_FOR_READ_OPS_FOR_TESTS_IN_MILLISECONDS = 10000;
 
     private static final int DEFAULT_MAX_UNIGRAM_COUNT = 10000;
     private static final int DEFAULT_MAX_BIGRAM_COUNT = 10000;
@@ -93,9 +93,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
     /** Dictionary file */
     private final File mDictFile;
 
-    private final AtomicBoolean mProcessingLargeTask;
+    /** Indicates whether a task for reloading the dictionary has been scheduled. */
+    private final AtomicBoolean mIsReloading;
+
+    /** Indicates whether the current dictionary needs to be reloaded. */
     private boolean mNeedsToReload;
 
+    private final ReentrantReadWriteLock mLock;
 
     /* A extension for a binary dictionary file. */
     protected static final String DICT_FILE_EXTENSION = ".dict";
@@ -144,8 +148,9 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
         mLocale = locale;
         mDictFile = getDictFile(context, dictName, dictFile);
         mBinaryDictionary = null;
-        mProcessingLargeTask = new AtomicBoolean();
+        mIsReloading = new AtomicBoolean();
         mNeedsToReload = false;
+        mLock = new ReentrantReadWriteLock();
     }
 
     public static File getDictFile(final Context context, final String dictName,
@@ -159,12 +164,30 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
         return dictFile != null ? dictFile.getName() : name + "." + locale.toString();
     }
 
+    private void asyncExecuteTaskWithWriteLock(final Runnable task) {
+        asyncExecuteTaskWithLock(mLock.writeLock(), task);
+    }
+
+    private void asyncExecuteTaskWithLock(final Lock lock, final Runnable task) {
+        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+            @Override
+            public void run() {
+                lock.lock();
+                try {
+                    task.run();
+                } finally {
+                    lock.unlock();
+                }
+            }
+        });
+    }
+
     /**
      * Closes and cleans up the binary dictionary.
      */
     @Override
     public void close() {
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
             @Override
             public void run() {
                 if (mBinaryDictionary != null) {
@@ -188,6 +211,15 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
         return attributeMap;
     }
 
+    private void removeBinaryDictionary() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
+            @Override
+            public void run() {
+                removeBinaryDictionaryLocked();
+            }
+        });
+    }
+
     private void removeBinaryDictionaryLocked() {
         if (mBinaryDictionary != null) {
             mBinaryDictionary.close();
@@ -211,7 +243,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
     }
 
     public void clear() {
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
             @Override
             public void run() {
                 removeBinaryDictionaryLocked();
@@ -224,13 +256,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
      * Check whether GC is needed and run GC if required.
      */
     protected void runGCIfRequired(final boolean mindsBlockByGC) {
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
             @Override
             public void run() {
                 if (mBinaryDictionary == null) {
                     return;
                 }
-                runGCAfterAllPrioritizedTasksIfRequiredLocked(mindsBlockByGC);
+                runGCIfRequiredLocked(mindsBlockByGC);
             }
         });
     }
@@ -241,25 +273,6 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
         }
     }
 
-    private void runGCAfterAllPrioritizedTasksIfRequiredLocked(final boolean mindsBlockByGC) {
-        // needsToRunGC() have to be called with lock.
-        if (mBinaryDictionary.needsToRunGC(mindsBlockByGC)) {
-            if (setProcessingLargeTaskIfNot()) {
-                // Run GC after currently existing time sensitive operations.
-                ExecutorUtils.getExecutor(mDictName).executePrioritized(new Runnable() {
-                    @Override
-                    public void run() {
-                        try {
-                            mBinaryDictionary.flushWithGC();
-                        } finally {
-                            mProcessingLargeTask.set(false);
-                        }
-                    }
-                });
-            }
-        }
-    }
-
     /**
      * Dynamically adds a word unigram to the dictionary. May overwrite an existing entry.
      */
@@ -267,13 +280,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
             final String shortcutTarget, final int shortcutFreq, final boolean isNotAWord,
             final boolean isBlacklisted, final int timestamp) {
         reloadDictionaryIfRequired();
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
             @Override
             public void run() {
                 if (mBinaryDictionary == null) {
                     return;
                 }
-                runGCAfterAllPrioritizedTasksIfRequiredLocked(true /* mindsBlockByGC */);
+                runGCIfRequiredLocked(true /* mindsBlockByGC */);
                 addWordDynamicallyLocked(word, frequency, shortcutTarget, shortcutFreq,
                         isNotAWord, isBlacklisted, timestamp);
             }
@@ -293,13 +306,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
     public void addBigramDynamically(final String word0, final String word1,
             final int frequency, final int timestamp) {
         reloadDictionaryIfRequired();
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
             @Override
             public void run() {
                 if (mBinaryDictionary == null) {
                     return;
                 }
-                runGCAfterAllPrioritizedTasksIfRequiredLocked(true /* mindsBlockByGC */);
+                runGCIfRequiredLocked(true /* mindsBlockByGC */);
                 addBigramDynamicallyLocked(word0, word1, frequency, timestamp);
             }
         });
@@ -315,13 +328,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
      */
     public void removeBigramDynamically(final String word0, final String word1) {
         reloadDictionaryIfRequired();
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
             @Override
             public void run() {
                 if (mBinaryDictionary == null) {
                     return;
                 }
-                runGCAfterAllPrioritizedTasksIfRequiredLocked(true /* mindsBlockByGC */);
+                runGCIfRequiredLocked(true /* mindsBlockByGC */);
                 mBinaryDictionary.removeBigramWords(word0, word1);
             }
         });
@@ -338,14 +351,13 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
             final ArrayList<LanguageModelParam> languageModelParams,
             final AddMultipleDictionaryEntriesCallback callback) {
         reloadDictionaryIfRequired();
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
             @Override
             public void run() {
+                try {
                     if (mBinaryDictionary == null) {
                         return;
                     }
-                final boolean locked = setProcessingLargeTaskIfNot();
-                try {
                     mBinaryDictionary.addMultipleDictionaryEntries(
                             languageModelParams.toArray(
                                     new LanguageModelParam[languageModelParams.size()]));
@@ -353,9 +365,6 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
                     if (callback != null) {
                         callback.onFinished();
                     }
-                    if (locked) {
-                        mProcessingLargeTask.set(false);
-                    }
                 }
             }
         });
@@ -367,29 +376,33 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
             final boolean blockOffensiveWords, final int[] additionalFeaturesOptions,
             final int sessionId, final float[] inOutLanguageWeight) {
         reloadDictionaryIfRequired();
-        if (processingLargeTask()) {
+        boolean lockAcquired = false;
+        try {
+            lockAcquired = mLock.readLock().tryLock(
+                    TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS, TimeUnit.MILLISECONDS);
+            if (lockAcquired) {
+                if (mBinaryDictionary == null) {
                     return null;
                 }
-        final AsyncResultHolder<ArrayList<SuggestedWordInfo>> holder =
+                final ArrayList<SuggestedWordInfo> suggestions =
-                new AsyncResultHolder<ArrayList<SuggestedWordInfo>>();
-        ExecutorUtils.getExecutor(mDictName).executePrioritized(new Runnable() {
-            @Override
-            public void run() {
-                if (mBinaryDictionary == null) {
-                    holder.set(null);
-                    return;
-                }
-                final ArrayList<SuggestedWordInfo> binarySuggestion =
                         mBinaryDictionary.getSuggestionsWithSessionId(composer, prevWord,
                                 proximityInfo, blockOffensiveWords, additionalFeaturesOptions,
                                 sessionId, inOutLanguageWeight);
-                holder.set(binarySuggestion);
                 if (mBinaryDictionary.isCorrupted()) {
-                    removeBinaryDictionaryLocked();
+                    Log.i(TAG, "Dictionary (" + mDictName +") is corrupted. "
+                            + "Remove and regenerate it.");
+                    removeBinaryDictionary();
+                }
+                return suggestions;
+            }
+        } catch (final InterruptedException e) {
+            Log.e(TAG, "Interrupted tryLock() in getSuggestionsWithSessionId().", e);
+        } finally {
+            if (lockAcquired) {
+                mLock.readLock().unlock();
             }
         }
-        });
+        return null;
-        return holder.get(null, TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS);
     }
 
     @Override
@@ -404,17 +417,24 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
     @Override
     public boolean isValidWord(final String word) {
         reloadDictionaryIfRequired();
-        if (processingLargeTask()) {
+        boolean lockAcquired = false;
+        try {
+            lockAcquired = mLock.readLock().tryLock(
+                    TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS, TimeUnit.MILLISECONDS);
+            if (lockAcquired) {
+                if (mBinaryDictionary == null) {
                     return false;
                 }
-        final AsyncResultHolder<Boolean> holder = new AsyncResultHolder<Boolean>();
+                return isValidWordLocked(word);
-        ExecutorUtils.getExecutor(mDictName).executePrioritized(new Runnable() {
-            @Override
-            public void run() {
-                holder.set(isValidWordLocked(word));
             }
-        });
+        } catch (final InterruptedException e) {
-        return holder.get(false, TIMEOUT_FOR_READ_OPS_IN_MILLISECONDS);
+            Log.e(TAG, "Interrupted tryLock() in isValidWord().", e);
+        } finally {
+            if (lockAcquired) {
+                mLock.readLock().unlock();
+            }
+        }
+        return false;
     }
 
     protected boolean isValidWordLocked(final String word) {
@@ -484,13 +504,14 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
     }
 
     /**
-     * Reloads the dictionary if required.
+     * Load the current binary dictionary from internal storage. If the dictionary file doesn't
+     * exists or needs to be regenerated, the new dictionary file will be asynchronously generated.
+     * However, the dictionary itself is accessible even before the new dictionary file is actually
+     * generated. It may return a null result for getSuggestions() in that case by design.
      */
     public final void reloadDictionaryIfRequired() {
         if (!isReloadRequired()) return;
-        if (setProcessingLargeTaskIfNot()) {
+        asyncReloadDictionary();
-            reloadDictionary();
-        }
     }
 
     /**
@@ -500,38 +521,24 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
         return mBinaryDictionary == null || mNeedsToReload;
     }
 
-    private boolean processingLargeTask() {
-        return mProcessingLargeTask.get();
-    }
-
-    // Returns whether the dictionary is being used for a large task. If true, we should not use
-    // this dictionary for latency sensitive operations.
-    private boolean setProcessingLargeTaskIfNot() {
-        return mProcessingLargeTask.compareAndSet(false /* expect */ , true /* update */);
-    }
-
     /**
-     * Reloads the dictionary. Access is controlled on a per dictionary file basis and supports
+     * Reloads the dictionary. Access is controlled on a per dictionary file basis.
-     * concurrent calls from multiple instances that share the same dictionary file.
      */
-    private final void reloadDictionary() {
+    private final void asyncReloadDictionary() {
-        // Ensure that only one thread attempts to read or write to the shared binary dictionary
+        if (mIsReloading.compareAndSet(false, true)) {
-        // file at the same time.
+            mNeedsToReload = false;
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+            asyncExecuteTaskWithWriteLock(new Runnable() {
                 @Override
                 public void run() {
                     try {
                         if (!dictionaryFileExists() || haveContentsChanged()) {
-                        // If the shared dictionary file does not exist or is out of date and
+                            // If the shared dictionary file does not exist or contents have been
-                        // contents have been updated, the first instance that acquires the lock
+                            // updated, generate a new one.
-                        // will generate a new one
                             createNewDictionaryLocked();
                         } else if (mBinaryDictionary == null) {
-                        // Otherwise, if the local dictionary is older than the shared dictionary,
+                            // Otherwise, load the existing dictionary.
-                        // load the shared dictionary.
                             loadBinaryDictionaryLocked();
                         }
-                    mNeedsToReload = false;
                         if (mBinaryDictionary != null && !(isValidDictionaryLocked()
                                 // TODO: remove the check below
                                 && matchesExpectedBinaryDictFormatVersionForThisType(
@@ -542,11 +549,12 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
                             createNewDictionaryLocked();
                         }
                     } finally {
-                    mProcessingLargeTask.set(false);
+                        mIsReloading.set(false);
                     }
                 }
             });
         }
+    }
 
     // TODO: cache the file's existence so that we avoid doing a disk access each time.
     private boolean dictionaryFileExists() {
@@ -557,7 +565,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
      * Flush binary dictionary to dictionary file.
      */
     public void asyncFlushBinaryDictionary() {
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithWriteLock(new Runnable() {
             @Override
             public void run() {
                 flushDictionaryLocked();
@@ -568,16 +576,15 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
     // TODO: Implement BinaryDictionary.isInDictionary().
     @UsedForTesting
     public boolean isInUnderlyingBinaryDictionaryForTests(final String word) {
-        final AsyncResultHolder<Boolean> holder = new AsyncResultHolder<Boolean>();
+        mLock.readLock().lock();
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        try {
-            @Override
+            if (mBinaryDictionary != null && mDictType == Dictionary.TYPE_USER_HISTORY) {
-            public void run() {
+                return mBinaryDictionary.isValidWord(word);
-                if (mDictType == Dictionary.TYPE_USER_HISTORY) {
-                    holder.set(mBinaryDictionary.isValidWord(word));
             }
+            return false;
+        } finally {
+            mLock.readLock().unlock();
         }
-        });
-        return holder.get(false, TIMEOUT_FOR_READ_OPS_FOR_TESTS_IN_MILLISECONDS);
     }
 
     @UsedForTesting
@@ -599,7 +606,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
     @UsedForTesting
     public void dumpAllWordsForDebug() {
         reloadDictionaryIfRequired();
-        ExecutorUtils.getExecutor(mDictName).execute(new Runnable() {
+        asyncExecuteTaskWithLock(mLock.readLock(), new Runnable() {
             @Override
             public void run() {
                 Log.d(TAG, "Dump dictionary: " + mDictName);

tests/src/com/android/inputmethod/latin/personalization/UserHistoryDictionaryTests.java

@@ -264,6 +264,7 @@ public class UserHistoryDictionaryTests extends AndroidTestCase {
         for (final String word : words) {
             UserHistoryDictionary.addToDictionary(dict, prevWord, word, true, mCurrentTime);
             prevWord = word;
+            dict.waitAllTasksForTests();
             assertTrue(dict.isInUnderlyingBinaryDictionaryForTests(word));
         }
         forcePassingShortTime();