From 78efeafb6425956bfb98a7a54134b0e5d9b4e5d7 Mon Sep 17 00:00:00 2001 From: Floens Date: Sun, 28 Jun 2015 11:46:35 +0200 Subject: [PATCH] Improvements for concurrency Marked some Post members for when they may be modified. Closed, archived, sticky etc. is now only modified on the main thread. BoardManager.getBoardByValue still gets called from multiple threads, next thing to improve. --- .../java/org/floens/chan/chan/ChanLoader.java | 176 ++++++++++-------- .../java/org/floens/chan/chan/ChanParser.java | 9 +- .../chan/core/manager/BoardManager.java | 1 + .../java/org/floens/chan/core/model/Post.java | 50 ++--- .../chan/core/net/ChanReaderRequest.java | 131 +++++-------- .../floens/chan/core/watch/PinWatcher.java | 2 +- .../floens/chan/database/DatabaseManager.java | 43 +++-- .../org/floens/chan/ui/cell/PostCell.java | 4 +- 8 files changed, 216 insertions(+), 200 deletions(-) diff --git a/Clover/app/src/main/java/org/floens/chan/chan/ChanLoader.java b/Clover/app/src/main/java/org/floens/chan/chan/ChanLoader.java index 5bb70a13..127dca1f 100644 --- a/Clover/app/src/main/java/org/floens/chan/chan/ChanLoader.java +++ b/Clover/app/src/main/java/org/floens/chan/chan/ChanLoader.java @@ -19,6 +19,7 @@ package org.floens.chan.chan; import android.text.TextUtils; +import com.android.volley.RequestQueue; import com.android.volley.Response; import com.android.volley.VolleyError; @@ -39,7 +40,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; -public class ChanLoader { +public class ChanLoader implements Response.ErrorListener, Response.Listener { private static final String TAG = "ChanLoader"; private static final ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); @@ -47,6 +48,7 @@ public class ChanLoader { private final List listeners = new ArrayList<>(); private final Loadable loadable; + private final RequestQueue volleyRequestQueue; private ChanThread thread; private boolean destroyed = false; @@ -64,6 +66,8 @@ public class ChanLoader { if (loadable.mode == Loadable.Mode.BOARD) { loadable.mode = Loadable.Mode.CATALOG; } + + volleyRequestQueue = Chan.getVolleyRequestQueue(); } /** @@ -149,11 +153,7 @@ public class ChanLoader { public void requestMoreData() { clearTimer(); - if (loadable.isThreadMode()) { - if (request != null) { - return; - } - + if (loadable.isThreadMode() && request == null) { request = getData(); } } @@ -166,9 +166,6 @@ public class ChanLoader { requestMoreData(); } - /** - * @return Returns if this loader is currently loading - */ public boolean isLoading() { return request != null; } @@ -193,10 +190,102 @@ public class ChanLoader { return thread; } + @Override + public void onResponse(ChanReaderRequest.ChanReaderResponse response) { + request = null; + if (destroyed) + return; + + if (response.posts.size() == 0) { + onErrorResponse(new VolleyError("Post size is 0")); + return; + } + + if (thread == null) { + thread = new ChanThread(loadable, new ArrayList()); + } + + thread.posts.clear(); + thread.posts.addAll(response.posts); + + processResponse(response); + + if (TextUtils.isEmpty(loadable.title)) { + loadable.title = PostHelper.getTitle(thread.op, loadable); + } + + for (Post post : thread.posts) { + post.title = loadable.title; + } + + lastLoadTime = Time.get(); + + if (loadable.isThreadMode()) { + setTimer(response.posts.size()); + } + + for (ChanLoaderCallback l : listeners) { + l.onChanLoaderData(thread); + } + } + + @Override + public void onErrorResponse(VolleyError error) { + request = null; + if (destroyed) + return; + + Logger.i(TAG, "Loading error", error); + + clearTimer(); + + for (ChanLoaderCallback l : listeners) { + l.onChanLoaderError(error); + } + } + + /** + * Final processing af a response that needs to happen on the main thread. + * + * @param response Response to process + */ + private void processResponse(ChanReaderRequest.ChanReaderResponse response) { + if (loadable.isThreadMode() && thread.posts.size() > 0) { + // Replace some op parameters to the real op (index 0). + // This is done on the main thread to avoid race conditions. + Post realOp = thread.posts.get(0); + thread.op = realOp; + Post fakeOp = response.op; + if (fakeOp != null) { + thread.closed = realOp.closed = fakeOp.closed; + thread.archived = realOp.archived = fakeOp.archived; + realOp.sticky = fakeOp.sticky; + realOp.replies = fakeOp.replies; + realOp.images = fakeOp.images; + realOp.uniqueIps = fakeOp.uniqueIps; + } else { + Logger.e(TAG, "Thread has no op!"); + } + } + + for (Post sourcePost : thread.posts) { + sourcePost.repliesFrom.clear(); + + for (Post replyToSource : thread.posts) { + if (replyToSource != sourcePost) { + if (replyToSource.repliesTo.contains(sourcePost.no)) { + sourcePost.repliesFrom.add(replyToSource.no); + } + } + } + } + } + private void setTimer(int postCount) { clearTimer(); if (postCount > lastPostCount) { + lastPostCount = postCount; currentTimeout = 0; } else { currentTimeout++; @@ -209,8 +298,6 @@ public class ChanLoader { currentTimeout = 4; // At least 60 seconds in the background } - lastPostCount = postCount; - if (autoReload) { Runnable pendingRunnable = new Runnable() { @Override @@ -243,76 +330,13 @@ public class ChanLoader { Logger.d(TAG, "Requested " + loadable.board + ", " + loadable.no); List cached = thread == null ? new ArrayList() : thread.posts; - ChanReaderRequest request = ChanReaderRequest.newInstance(loadable, cached, - new Response.Listener>() { - @Override - public void onResponse(List list) { - ChanLoader.this.request = null; - onData(list); - } - }, new Response.ErrorListener() { - @Override - public void onErrorResponse(VolleyError error) { - ChanLoader.this.request = null; - onError(error); - } - } - ); + ChanReaderRequest request = ChanReaderRequest.newInstance(loadable, cached, this, this); - Chan.getVolleyRequestQueue().add(request); + volleyRequestQueue.add(request); return request; } - private void onData(List result) { - if (destroyed) - return; - - if (thread == null) { - thread = new ChanThread(loadable, new ArrayList()); - } - - thread.posts.clear(); - thread.posts.addAll(result); - - if (loadable.isThreadMode() && thread.posts.size() > 0) { - thread.op = thread.posts.get(0); - thread.closed = thread.op.closed; - thread.archived = thread.op.archived; - } - - if (TextUtils.isEmpty(loadable.title)) { - loadable.title = PostHelper.getTitle(thread.op, loadable); - } - - for (Post post : thread.posts) { - post.title = loadable.title; - } - - lastLoadTime = Time.get(); - - if (loadable.isThreadMode()) { - setTimer(result.size()); - } - - for (ChanLoaderCallback l : listeners) { - l.onChanLoaderData(thread); - } - } - - private void onError(VolleyError error) { - if (destroyed) - return; - - Logger.e(TAG, "Loading error"); - - clearTimer(); - - for (ChanLoaderCallback l : listeners) { - l.onChanLoaderError(error); - } - } - public interface ChanLoaderCallback { void onChanLoaderData(ChanThread result); diff --git a/Clover/app/src/main/java/org/floens/chan/chan/ChanParser.java b/Clover/app/src/main/java/org/floens/chan/chan/ChanParser.java index 397acb6d..c1d81de4 100644 --- a/Clover/app/src/main/java/org/floens/chan/chan/ChanParser.java +++ b/Clover/app/src/main/java/org/floens/chan/chan/ChanParser.java @@ -33,6 +33,7 @@ import org.floens.chan.Chan; import org.floens.chan.core.model.Post; import org.floens.chan.core.model.PostLinkable; import org.floens.chan.core.settings.ChanSettings; +import org.floens.chan.database.DatabaseManager; import org.floens.chan.ui.theme.Theme; import org.floens.chan.ui.theme.ThemeHelper; import org.floens.chan.utils.Logger; @@ -60,6 +61,11 @@ public class ChanParser { private static final Pattern colorPattern = Pattern.compile("color:#([0-9a-fA-F]*)"); private static ChanParser instance = new ChanParser(); + private final DatabaseManager databaseManager; + + public ChanParser() { + databaseManager = Chan.getDatabaseManager(); + } public static ChanParser getInstance() { return instance; @@ -404,8 +410,7 @@ public class ChanParser { } // Append You when it's a reply to an saved reply - // todo synchronized - if (Chan.getDatabaseManager().isSavedReply(post.board, id)) { + if (databaseManager.isSavedReply(post.board, id)) { key += " (You)"; } } diff --git a/Clover/app/src/main/java/org/floens/chan/core/manager/BoardManager.java b/Clover/app/src/main/java/org/floens/chan/core/manager/BoardManager.java index 4dde2ed7..3d581388 100644 --- a/Clover/app/src/main/java/org/floens/chan/core/manager/BoardManager.java +++ b/Clover/app/src/main/java/org/floens/chan/core/manager/BoardManager.java @@ -52,6 +52,7 @@ public class BoardManager { loadFromServer(); } + // TODO: synchronize public Board getBoardByValue(String value) { return allBoardsByValue.get(value); } diff --git a/Clover/app/src/main/java/org/floens/chan/core/model/Post.java b/Clover/app/src/main/java/org/floens/chan/core/model/Post.java index 5c976980..5f95b9e8 100644 --- a/Clover/app/src/main/java/org/floens/chan/core/model/Post.java +++ b/Clover/app/src/main/java/org/floens/chan/core/model/Post.java @@ -21,20 +21,25 @@ import android.text.SpannableString; import android.text.TextUtils; import org.floens.chan.Chan; -import org.floens.chan.chan.ChanUrls; import org.floens.chan.chan.ChanParser; +import org.floens.chan.chan.ChanUrls; import org.jsoup.parser.Parser; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Random; +import java.util.concurrent.atomic.AtomicBoolean; /** - * Contains all data needed to represent a single post. + * Contains all data needed to represent a single post.
+ * Call {@link #finish()} to parse the comment etc. The post data is invalid if finish returns false.
+ * This class has members that are threadsafe and some that are not, see the source for more info. */ public class Post { private static final Random random = new Random(); + // *** These next members don't get changed after finish() is called. Effectively final. *** public String board; public int no = -1; public int resto = -1; @@ -46,42 +51,25 @@ public class Post { public long tim = -1; public String ext; public String filename; - public int replies = -1; public int imageWidth; public int imageHeight; public boolean hasImage = false; public String thumbnailUrl; public String imageUrl; - public boolean sticky = false; - public boolean closed = false; - public boolean archived = false; public String tripcode = ""; public String id = ""; public String capcode = ""; public String country = ""; public String countryName = ""; public long time = -1; - public boolean isSavedReply = false; - public String title = ""; public int fileSize; - public int images = -1; public String rawComment; public String countryUrl; public boolean spoiler = false; - public int uniqueIps = 1; - - public boolean deleted = false; - /** - * This post replies to the these ids + * This post replies to the these ids. Is an unmodifiable list after finish(). */ public List repliesTo = new ArrayList<>(); - - /** - * These ids replied to this post - */ - public List repliesFrom = new ArrayList<>(); - public final ArrayList linkables = new ArrayList<>(); public boolean parsedSpans = false; public SpannableString subjectSpan; @@ -91,11 +79,25 @@ public class Post { public SpannableString capcodeSpan; public CharSequence nameTripcodeIdCapcodeSpan; - public Post() { - } + // *** These next members may only change on the main thread after finish(). *** + public boolean sticky = false; + public boolean closed = false; + public boolean archived = false; + public int replies = -1; + public int images = -1; + public int uniqueIps = 1; + public String title = ""; + /** + * These ids replied to this post. Only modify this on the main thread. + */ + public List repliesFrom = new ArrayList<>(); + + // *** Threadsafe members, may be read and modified on any thread. *** + public AtomicBoolean deleted = new AtomicBoolean(false); + public AtomicBoolean isSavedReply = new AtomicBoolean(false); /** - * Finish up the data + * Finish up the data: parse the comment, check if the data is valid etc. * * @return false if this data is invalid */ @@ -134,6 +136,8 @@ public class Post { ChanParser.getInstance().parse(this); + repliesTo = Collections.unmodifiableList(repliesTo); + return true; } } diff --git a/Clover/app/src/main/java/org/floens/chan/core/net/ChanReaderRequest.java b/Clover/app/src/main/java/org/floens/chan/core/net/ChanReaderRequest.java index f634dac5..779e0657 100644 --- a/Clover/app/src/main/java/org/floens/chan/core/net/ChanReaderRequest.java +++ b/Clover/app/src/main/java/org/floens/chan/core/net/ChanReaderRequest.java @@ -26,22 +26,23 @@ import org.floens.chan.Chan; import org.floens.chan.chan.ChanUrls; import org.floens.chan.core.model.Loadable; import org.floens.chan.core.model.Post; +import org.floens.chan.utils.Logger; -import java.io.IOException; import java.util.ArrayList; -import java.util.HashSet; import java.util.List; -import java.util.Set; -public class ChanReaderRequest extends JsonReaderRequest> { +public class ChanReaderRequest extends JsonReaderRequest { + private static final String TAG = "ChanReaderRequest"; + private Loadable loadable; private List cached; + private Post op; - private ChanReaderRequest(String url, Listener> listener, ErrorListener errorListener) { + private ChanReaderRequest(String url, Listener listener, ErrorListener errorListener) { super(url, listener, errorListener); } - public static ChanReaderRequest newInstance(Loadable loadable, List cached, Listener> listener, ErrorListener errorListener) { + public static ChanReaderRequest newInstance(Loadable loadable, List cached, Listener listener, ErrorListener errorListener) { String url; if (loadable.isThreadMode()) { @@ -67,7 +68,7 @@ public class ChanReaderRequest extends JsonReaderRequest> { } @Override - public List readJson(JsonReader reader) throws Exception { + public ChanReaderResponse readJson(JsonReader reader) throws Exception { List list; if (loadable.isThreadMode()) { @@ -81,11 +82,14 @@ public class ChanReaderRequest extends JsonReaderRequest> { return processPosts(list); } - private List processPosts(List serverList) throws Exception { - List totalList = new ArrayList<>(serverList.size()); + private ChanReaderResponse processPosts(List serverList) throws Exception { + ChanReaderResponse response = new ChanReaderResponse(); + response.posts = new ArrayList<>(serverList.size()); + response.op = op; if (cached.size() > 0) { - totalList.addAll(cached); + // Add all posts that were parsed before + response.posts.addAll(cached); // If there's a cached post but it's not in the list received from the server, mark it as deleted if (loadable.isThreadMode()) { @@ -99,7 +103,7 @@ public class ChanReaderRequest extends JsonReaderRequest> { } } - cache.deleted = !serverHas; + cache.deleted.set(!serverHas); } } @@ -115,68 +119,19 @@ public class ChanReaderRequest extends JsonReaderRequest> { } } - // serverPost is not in finalList if (!known) { - totalList.add(post); + response.posts.add(post); } } - - // Replace OPs - if (totalList.get(0).isOP && serverList.size() > 0 && serverList.get(0).isOP) { - totalList.set(0, serverList.get(0)); - } - - // Sort if it got out of order due to posts disappearing/reappearing - /*if (loadable.isThreadMode()) { - Collections.sort(totalList, new Comparator() { - @Override - public int compare(Post lhs, Post rhs) { - return lhs.time == rhs.time ? 0 : (lhs.time < rhs.time ? -1 : 1); - } - }); - }*/ - } else { - totalList.addAll(serverList); - } - - Set postsReplyingToDeleted = new HashSet<>(); - for (Post post : totalList) { - if (!post.deleted) { - post.repliesFrom.clear(); - - for (Post other : totalList) { - if (other.repliesTo.contains(post.no) && !other.deleted) { - post.repliesFrom.add(other.no); - } - } - } else { - post.repliesTo.clear(); - - for (int no : post.repliesFrom) { - postsReplyingToDeleted.add(no); - } - - post.repliesFrom.clear(); - } + response.posts.addAll(serverList); } - for (int no : postsReplyingToDeleted) { - for (Post post : totalList) { - if (post.no == no) { - if (!post.finish()) { - throw new IOException("Incorrect data about post received."); - } - break; - } - } + for (Post post : response.posts) { + post.isSavedReply.set(Chan.getDatabaseManager().isSavedReply(post.board, post.no)); } - for (Post post : totalList) { - post.isSavedReply = Chan.getDatabaseManager().isSavedReply(post.board, post.no); - } - - return totalList; + return response; } private List loadThread(JsonReader reader) throws Exception { @@ -191,7 +146,10 @@ public class ChanReaderRequest extends JsonReaderRequest> { // Thread array while (reader.hasNext()) { // Thread object - list.add(readPostObject(reader)); + Post post = readPostObject(reader); + if (post != null) { + list.add(post); + } } reader.endArray(); } else { @@ -216,7 +174,10 @@ public class ChanReaderRequest extends JsonReaderRequest> { reader.beginArray(); // Threads array while (reader.hasNext()) { - list.add(readPostObject(reader)); + Post post = readPostObject(reader); + if (post != null) { + list.add(post); + } } reader.endArray(); @@ -324,28 +285,28 @@ public class ChanReaderRequest extends JsonReaderRequest> { break; default: // Unknown/ignored key - // log("Unknown/ignored key: " + key + "."); reader.skipValue(); break; } } reader.endObject(); + if (post.resto == 0) { + // Update OP fields later on the main thread + op = new Post(); + op.closed = post.closed; + op.archived = post.archived; + op.sticky = post.sticky; + op.replies = post.replies; + op.images = post.images; + op.uniqueIps = post.uniqueIps; + } + Post cached = null; for (Post item : this.cached) { if (item.no == post.no) { cached = item; - if (post.resto == 0) { - // Update OP fields - cached.sticky = post.sticky; - cached.closed = post.closed; - cached.archived = post.archived; - cached.replies = post.replies; - cached.images = post.images; - cached.uniqueIps = post.uniqueIps; - } - break; } } @@ -354,10 +315,18 @@ public class ChanReaderRequest extends JsonReaderRequest> { return cached; } else { if (!post.finish()) { - throw new IOException("Incorrect data about post received."); + Logger.e(TAG, "Incorrect data about post received for post " + post.no); + return null; + } else { + return post; } - - return post; } } + + public static class ChanReaderResponse { + // Op Post that is created new each time.
+ // Used to later copy members like image count to the real op on the main thread. + public Post op; + public List posts; + } } diff --git a/Clover/app/src/main/java/org/floens/chan/core/watch/PinWatcher.java b/Clover/app/src/main/java/org/floens/chan/core/watch/PinWatcher.java index 6df7e77c..ae354aa2 100644 --- a/Clover/app/src/main/java/org/floens/chan/core/watch/PinWatcher.java +++ b/Clover/app/src/main/java/org/floens/chan/core/watch/PinWatcher.java @@ -147,7 +147,7 @@ public class PinWatcher implements ChanLoader.ChanLoaderCallback { for (Post item : thread.posts) { // saved.title = pin.loadable.title; - if (item.isSavedReply) { + if (item.isSavedReply.get()) { savedReplies.add(item); } } diff --git a/Clover/app/src/main/java/org/floens/chan/database/DatabaseManager.java b/Clover/app/src/main/java/org/floens/chan/database/DatabaseManager.java index f3ffc508..3475ddf8 100644 --- a/Clover/app/src/main/java/org/floens/chan/database/DatabaseManager.java +++ b/Clover/app/src/main/java/org/floens/chan/database/DatabaseManager.java @@ -47,11 +47,12 @@ public class DatabaseManager { private final DatabaseHelper helper; - private List savedReplies = new ArrayList<>(); - private HashSet savedRepliesIds = new HashSet<>(); + private final Object savedRepliesLock = new Object(); + private final List savedReplies = new ArrayList<>(); + private final HashSet savedRepliesIds = new HashSet<>(); - private List threadHides = new ArrayList<>(); - private HashSet threadHidesIds = new HashSet<>(); + private final List threadHides = new ArrayList<>(); + private final HashSet threadHidesIds = new HashSet<>(); public DatabaseManager(Context context) { helper = new DatabaseHelper(context); @@ -60,6 +61,7 @@ public class DatabaseManager { /** * Save a reply to the savedreply table. + * Threadsafe. * * @param saved the {@link SavedReply} to save */ @@ -70,22 +72,27 @@ public class DatabaseManager { Logger.e(TAG, "Error saving reply", e); } - savedReplies.add(saved); - savedRepliesIds.add(saved.no); + synchronized (savedRepliesLock) { + savedReplies.add(saved); + savedRepliesIds.add(saved.no); + } } /** * Searches a saved reply. This is done through caching members, no database lookups. + * Threadsafe. * * @param board board for the reply to search * @param no no for the reply to search * @return A {@link SavedReply} that matches {@code board} and {@code no}, or {@code null} */ public SavedReply getSavedReply(String board, int no) { - if (savedRepliesIds.contains(no)) { - for (SavedReply r : savedReplies) { - if (r.no == no && r.board.equals(board)) { - return r; + synchronized (savedRepliesLock) { + if (savedRepliesIds.contains(no)) { + for (SavedReply r : savedReplies) { + if (r.no == no && r.board.equals(board)) { + return r; + } } } } @@ -95,6 +102,7 @@ public class DatabaseManager { /** * Searches if a saved reply exists. This is done through caching members, no database lookups. + * Threadsafe. * * @param board board for the reply to search * @param no no for the reply to search @@ -324,15 +332,20 @@ public class DatabaseManager { loadThreadHides(); } + /** + * Threadsafe. + */ private void loadSavedReplies() { try { trimTable(helper.savedDao, "savedreply", SAVED_REPLY_TRIM_TRIGGER, SAVED_REPLY_TRIM_COUNT); - savedReplies.clear(); - savedReplies.addAll(helper.savedDao.queryForAll()); - savedRepliesIds.clear(); - for (SavedReply reply : savedReplies) { - savedRepliesIds.add(reply.no); + synchronized (savedRepliesLock) { + savedReplies.clear(); + savedReplies.addAll(helper.savedDao.queryForAll()); + savedRepliesIds.clear(); + for (SavedReply reply : savedReplies) { + savedRepliesIds.add(reply.no); + } } } catch (SQLException e) { Logger.e(TAG, "Error loading saved replies", e); diff --git a/Clover/app/src/main/java/org/floens/chan/ui/cell/PostCell.java b/Clover/app/src/main/java/org/floens/chan/ui/cell/PostCell.java index e8c9b1d3..18c48f57 100644 --- a/Clover/app/src/main/java/org/floens/chan/ui/cell/PostCell.java +++ b/Clover/app/src/main/java/org/floens/chan/ui/cell/PostCell.java @@ -273,7 +273,7 @@ public class PostCell extends RelativeLayout implements PostCellInterface, PostL if (highlighted) { setBackgroundColor(theme.highlightedColor); - } else if (post.isSavedReply) { + } else if (post.isSavedReply.get()) { setBackgroundColor(theme.savedReplyColor); } else if (threadMode) { setBackgroundResource(0); @@ -320,7 +320,7 @@ public class PostCell extends RelativeLayout implements PostCellInterface, PostL iconsSpannable = PostHelper.addIcon(iconsSpannable, PostHelper.closedIcon, iconsTextSize); } - if (post.deleted) { + if (post.deleted.get()) { iconsSpannable = PostHelper.addIcon(iconsSpannable, PostHelper.trashIcon, iconsTextSize); }