Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a328f538 authored by Yuichiro Hanada's avatar Yuichiro Hanada
Browse files

Fix PrioritizedSerialExecutor.

It was possible that fetchNextTasks() would be called by multiple
threads concurrently.
If it happens, some tasks in the task queues might be ignored.

Change-Id: Idc81c43c45e382da3850cc55b9a42c281548d2a8
parent 781feb74
Loading
Loading
Loading
Loading
+1 −1
Original line number Diff line number Diff line
@@ -646,7 +646,7 @@ abstract public class ExpandableBinaryDictionary extends Dictionary {
                        holder.set(mBinaryDictionary.isValidWord(word));
                    } else {
                        holder.set(((DynamicPersonalizationDictionaryWriter) mDictionaryWriter)
                                .isInDictionaryForTests(word));
                                .isInBigramListForTests(word));
                    }
                }
            }
+4 −4
Original line number Diff line number Diff line
@@ -79,7 +79,7 @@ public class DynamicPersonalizationDictionaryWriter extends AbstractDictionaryWr
    public void addUnigramWord(final String word, final String shortcutTarget, final int frequency,
            final boolean isNotAWord) {
        if (mBigramList.size() > mMaxHistoryBigrams * 2) {
            // Too many entries: just stop adding new vocabrary and wait next refresh.
            // Too many entries: just stop adding new vocabulary and wait next refresh.
            return;
        }
        mExpandableDictionary.addWord(word, shortcutTarget, frequency);
@@ -90,7 +90,7 @@ public class DynamicPersonalizationDictionaryWriter extends AbstractDictionaryWr
    public void addBigramWords(final String word0, final String word1, final int frequency,
            final boolean isValid, final long lastModifiedTime) {
        if (mBigramList.size() > mMaxHistoryBigrams * 2) {
            // Too many entries: just stop adding new vocabrary and wait next refresh.
            // Too many entries: just stop adding new vocabulary and wait next refresh.
            return;
        }
        if (lastModifiedTime > 0) {
@@ -176,8 +176,8 @@ public class DynamicPersonalizationDictionaryWriter extends AbstractDictionaryWr
    }

    @UsedForTesting
    public boolean isInDictionaryForTests(final String word) {
    public boolean isInBigramListForTests(final String word) {
        // TODO: Use native method to determine whether the word is in dictionary or not
        return mBigramList.containsKey(word);
        return mBigramList.containsKey(word) || mBigramList.getBigrams(null).containsKey(word);
    }
}
+37 −33
Original line number Diff line number Diff line
@@ -16,8 +16,11 @@

package com.android.inputmethod.latin.utils;

import java.util.ArrayDeque;
import java.util.Queue;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

/**
 * An object that executes submitted tasks using a thread.
@@ -27,19 +30,20 @@ public class PrioritizedSerialExecutor {

    private final Object mLock = new Object();

    // The default value of capacities of task queues.
    private static final int TASK_QUEUE_CAPACITY = 1000;
    private final Queue<Runnable> mTasks;
    private final Queue<Runnable> mPrioritizedTasks;
    private boolean mIsShutdown;
    private final ThreadPoolExecutor mThreadPoolExecutor;

    // The task which is running now.
    private Runnable mActive;

    public PrioritizedSerialExecutor() {
        mTasks = new ArrayDeque<Runnable>(TASK_QUEUE_CAPACITY);
        mPrioritizedTasks = new ArrayDeque<Runnable>(TASK_QUEUE_CAPACITY);
        mTasks = new ConcurrentLinkedQueue<Runnable>();
        mPrioritizedTasks = new ConcurrentLinkedQueue<Runnable>();
        mIsShutdown = false;
        mThreadPoolExecutor = new ThreadPoolExecutor(1 /* corePoolSize */, 1 /* maximumPoolSize */,
                0 /* keepAliveTime */, TimeUnit.MILLISECONDS, new ArrayBlockingQueue<Runnable>(1));
    }

    /**
@@ -59,7 +63,16 @@ public class PrioritizedSerialExecutor {
    public void execute(final Runnable r) {
        synchronized(mLock) {
            if (!mIsShutdown) {
                mTasks.offer(r);
                mTasks.offer(new Runnable() {
                    @Override
                    public void run() {
                        try {
                            r.run();
                        } finally {
                            scheduleNext();
                        }
                    }
                });
                if (mActive == null) {
                    scheduleNext();
                }
@@ -74,7 +87,16 @@ public class PrioritizedSerialExecutor {
    public void executePrioritized(final Runnable r) {
        synchronized(mLock) {
            if (!mIsShutdown) {
                mPrioritizedTasks.offer(r);
                mPrioritizedTasks.offer(new Runnable() {
                    @Override
                    public void run() {
                        try {
                            r.run();
                        } finally {
                            scheduleNext();
                        }
                    }
                });
                if (mActive == null) {
                    scheduleNext();
                }
@@ -82,37 +104,19 @@ public class PrioritizedSerialExecutor {
        }
    }

    private boolean fetchNextTasks() {
        synchronized(mLock) {
    private boolean fetchNextTasksLocked() {
        mActive = mPrioritizedTasks.poll();
        if (mActive == null) {
            mActive = mTasks.poll();
        }
        return mActive != null;
    }
    }

    private void scheduleNext() {
        synchronized(mLock) {
            if (!fetchNextTasks()) {
                return;
            }
            new Thread(new Runnable() {
                @Override
                public void run() {
                    try {
                        do {
                            synchronized(mLock) {
                                if (mActive != null) {
                                    mActive.run();
                                }
                            }
                        } while (fetchNextTasks());
                    } finally {
                        scheduleNext();
                    }
            if (fetchNextTasksLocked()) {
                mThreadPoolExecutor.execute(mActive);
            }
            }).start();
        }
    }

+4 −9
Original line number Diff line number Diff line
@@ -84,29 +84,24 @@ public class UserHistoryDictionaryTests extends AndroidTestCase {
    }

    /**
     * @param checksContents if true, checks whether written words are actually in the dictionary
     * @param checkContents if true, checks whether written words are actually in the dictionary
     * or not.
     */
    private void addAndWriteRandomWords(final String testFilenameSuffix, final int numberOfWords,
            final Random random, final boolean checksContents) {
            final Random random, final boolean checkContents) {
        final List<String> words = generateWords(numberOfWords, random);
        final UserHistoryPredictionDictionary dict =
                PersonalizationHelper.getUserHistoryPredictionDictionary(getContext(),
                        testFilenameSuffix /* locale */, mPrefs);
        // Add random words to the user history dictionary.
        addToDict(dict, words);
        if (checksContents) {
        if (checkContents) {
            try {
                Thread.sleep(TimeUnit.MILLISECONDS.convert(5L, TimeUnit.SECONDS));
            } catch (InterruptedException e) {
            }
            // Limit word count to check when using a Java on memory dictionary.
            final int wordCountToCheck =
                    ExpandableBinaryDictionary.ENABLE_BINARY_DICTIONARY_DYNAMIC_UPDATE ?
                            numberOfWords : 10;
            for (int i = 0; i < wordCountToCheck; ++i) {
            for (int i = 0; i < numberOfWords; ++i) {
                final String word = words.get(i);
                // This may fail as long as we use tryLock on inserting the bigram words
                assertTrue(dict.isInDictionaryForTests(word));
            }
        }