Fix soft TTL for duplicate requests #73

uhager · 2017-08-14T23:47:02Z

When a soft TTL cache entry exists, that response should be served immediately rather than having duplicate requests wait for the first request's network response. This is done by moving the list of waiting, duplicate requests from the RequestQueue to the CacheDispatcher.

…ts, that response should be served immediately rather than having duplicate requests wait for the first request's network response.

jjoslin

Looks good overall, just some minor concurrency issues to address.

jjoslin · 2017-09-08T22:12:44Z

src/main/java/com/android/volley/CacheDispatcher.java

+                                try {
+                                    mNetworkQueue.put(request);
+                                } catch (InterruptedException e) {
+                                    // Not much we can do about this.


Don't just swallow the InterruptedException, reset the interrupted status on the thread.

jjoslin · 2017-09-08T22:37:42Z

src/main/java/com/android/volley/CacheDispatcher.java

+            try {
+                mNetworkQueue.put(nextInLine);
+            } catch (InterruptedException iex) {
+                VolleyLog.e("Couldn't add request to queue. %s", iex.toString());


Don't just swallow the InterruptedException, reset the interrupted status on the thread.

jjoslin · 2017-09-08T22:45:20Z

src/main/java/com/android/volley/CacheDispatcher.java

+        synchronized (mWaitingRequests) {
+            waitingRequests = mWaitingRequests.remove(cacheKey);
+        }
+        if (waitingRequests != null) {


This is racy and you should expand the synchronized block above to also include the mWaitingRequests.put() call below. If you don't then the map can be modified by maybeAddToWaitingRequests() before the code hits the synchronized block below and any changes will be clobbered.

jjoslin · 2017-09-08T22:53:33Z

src/main/java/com/android/volley/Request.java

+     * @param response received from the network
+     */
+    /* package */ void notifyListenerResponseReceived(Response<?> response) {
+        if (mRequestCompleteListener != null) {


Access to mRequestCompleteListener should be synchronized. I believe the code works in its current state because mRequestCompleteListener is never concurrently accessed but if the code is ever rearranged then this could be a problem.

…ads.

joebowbeer

Great to see new unit tests. What is the code coverage of the added code?

joebowbeer · 2017-09-09T02:40:42Z

src/main/java/com/android/volley/CacheDispatcher.java

+     *          is <em>not</em> contained in that list. Is null if no requests are staged.</li>
+     * </ul>
+     */
+    private final Map<String, Queue<Request<?>>> mWaitingRequests = new HashMap<>();


Can you remove explicit synchronization of waiting requests and replace with ConcurrentHashMap instance? The downside is that you cannot use null as a marker, but that may be a good thing:-)

Is the CHM putIfAbsent method useful?

I think we should probably defer discussion of that kind of change to a separate PR / bug if desired. This portion of the code was just moved from one class to another, and to keep the scope of this PR manageable we should try to focus it solely on the bug it is trying to fix rather than introduce lots of other small fixes. (I do not think this would be a trivial change; often ConcurrentHashMap has significantly worse performance than a simple HashMap with a lock around it for low levels of concurrency).

@jpd236 Understood. Every time I see synchronized added, it raises my concern about maintainability, correctness and potential deadlock. I doubt ConcurrentHashMap would have any negative impact on the performance of this method at any level of concurrency. It might lead to a design that is easier to understand, though, and avoid questions about correctness. I felt obliged to raise this issue but I won't push back.

Wouldn't we still have to add a synchronized block in onNoUsableResponseReceived anyway?

@uhager synchronized can always be avoided :-) java.util.concurrent was written without a single synchronized. My advice to use CHM may be a bum lead, but the motivation is to make the synchronization more local and explicit. CHM may not be the best way to do that, but it feels like it may be. Seeing synchronized added to the code without explanation of which threads are involved, etc., worries me. Would moving the listener methods to a nested delegate, as I suggest, also allow the new synchronization to be more localized?

joebowbeer · 2017-09-09T02:54:43Z

src/main/java/com/android/volley/CacheDispatcher.java

+                // There is already a request in flight. Queue up.
+                Queue<Request<?>> stagedRequests = mWaitingRequests.get(cacheKey);
+                if (stagedRequests == null) {
+                    stagedRequests = new LinkedList<Request<?>>();


ArrayList is more performant here (and virtually everywhere). In other words, LinkedList is best avoided in Java.

This one was also just copied from the old code... but in this case I'm fine with making the change as I do think it's reasonable enough.

This is a Queue though. Should I make the Queue a List instead?

@uhager Ah. Use ArrayDeque:

This class is likely to be faster than Stack when used as a stack, and faster than LinkedList when used as a queue.

Null items are prohibited. I hope that's not a problem.

Volley's minSdkVersion is still 8, and ArrayDeque was added in API 9. Definitely a reasonable change to consider bumping minSdkVersion and making this change but it'd be out of the scope of the CL. So probably best to leave as is for now.

joebowbeer · 2017-09-09T02:57:29Z

src/main/java/com/android/volley/Request.java

+     */
+    /* package */ void setNetworkRequestCompleteListener(
+            NetworkRequestCompleteListener requestCompleteListener) {
+        synchronized(mLock) {


code style nit:

synchronized (mLock) {

joebowbeer · 2017-09-09T03:00:42Z

src/main/java/com/android/volley/CacheDispatcher.java

+    @Override
+    public void onNoUsableResponseReceived(Request<?> request) {
+        String cacheKey = request.getCacheKey();
+        Queue<Request<?>> waitingRequests;


Move this to where it is assigned.

joebowbeer · 2017-09-09T03:01:25Z

src/main/java/com/android/volley/CacheDispatcher.java

+                    VolleyLog.v("%d waiting requests for cacheKey=%s; resend to network",
+                        waitingRequests.size(), cacheKey);
+                }
+                Request<?> nextInLine = waitingRequests.remove();


How do you know nextInLine is not null? (A BlockingQueue does not accept null elements.)

Added a check.

jpd236

Regarding code coverage: we don't have that measured in our build at the moment, but it might be a reasonable feature request as a separate bug just for the extra information. (That being said, I don't think code coverage is a be-all end-all metric for test quality / sufficiency).

jpd236 · 2017-09-10T17:44:15Z

src/main/java/com/android/volley/CacheDispatcher.java

+     *          is <em>not</em> contained in that list. Is null if no requests are staged.</li>
+     * </ul>
+     */
+    private final Map<String, Queue<Request<?>>> mWaitingRequests = new HashMap<>();


I think we should probably defer discussion of that kind of change to a separate PR / bug if desired. This portion of the code was just moved from one class to another, and to keep the scope of this PR manageable we should try to focus it solely on the bug it is trying to fix rather than introduce lots of other small fixes. (I do not think this would be a trivial change; often ConcurrentHashMap has significantly worse performance than a simple HashMap with a lock around it for low levels of concurrency).

jpd236 · 2017-09-10T17:46:41Z

src/main/java/com/android/volley/CacheDispatcher.java

+                // There is already a request in flight. Queue up.
+                Queue<Request<?>> stagedRequests = mWaitingRequests.get(cacheKey);
+                if (stagedRequests == null) {
+                    stagedRequests = new LinkedList<Request<?>>();


This one was also just copied from the old code... but in this case I'm fine with making the change as I do think it's reasonable enough.

jpd236 · 2017-09-10T17:49:32Z

Also, @joebowbeer, thanks so much for the review! We appreciate your help :)

joebowbeer · 2017-09-10T18:59:50Z

src/main/java/com/android/volley/CacheDispatcher.java

+                            @Override
+                            public void run() {
+                                try {
+                                    mNetworkQueue.put(request);


Just wondering: What thread executes this blocking call? (I realize that this is just moving code around.)

I'm not quite sure which call you are referring to, but the CacheDispatcher runs on its own thread. In the default implementation, the Runnable is executed in ExecutorDelivery, which runs it on a thread that is set in ExecutorDelivery's instantiation by RequestQueue, where the default is the main looper. The network dispatcher then runs again on its own thread when it grabs the request from the NetworkQueue.

@uhager I was referring to the mNetworkQueue.put which is a blocking call (hence the InterruptedException).

Ah, that one then runs on the main thread (when using the default RequestQueue with the default ExecutorDelivery).

joebowbeer · 2017-09-10T19:05:42Z

src/main/java/com/android/volley/CacheDispatcher.java

@@ -29,7 +33,7 @@
 * refresh are enqueued on the specified network queue for processing
 * by a {@link NetworkDispatcher}.
 */
-public class CacheDispatcher extends Thread {
+public class CacheDispatcher extends Thread implements Request.NetworkRequestCompleteListener {


Can you push this listener responsibility into a delegate? Best practice is generally to limit scope/capability as much as possible... I bring this up because it is confusing to me to see new methods added to this Thread class that do not execute on this Thread.

Do you mean as a nested class in CacheDispatcher?

@uhager Yes. Move the listener methods, the mWaitingRequests instance and the maybeAdd helper method to a static nested class with an explicit reference to the CacheDispatcher instance. Then you can just synchronize the methods of the nested class, and it will be clear what state is being maintained.

joebowbeer · 2017-09-10T19:06:25Z

src/main/java/com/android/volley/CacheDispatcher.java

+                // Insert 'null' queue for this cacheKey, indicating there is now a request in
+                // flight.
+                mWaitingRequests.put(cacheKey, null);
+                request.setNetworkRequestCompleteListener(this);


Make this a dedicated listener instance? (See comment above.)

joebowbeer · 2017-09-10T19:14:34Z

@jpd236 regarding code coverage, in Android Studio right-click on com.android.volley (test) and select Run Tests with Coverage. The coverage of CacheDispatcher prior to this PR was nearly 100%. It would be cool to enable this in the Travis build.

joebowbeer · 2017-09-10T19:18:46Z

src/main/java/com/android/volley/CacheDispatcher.java

+                    VolleyLog.e("Couldn't add request to queue. %s", iex.toString());
+                    // Restore the interrupted status
+                    Thread.currentThread().interrupt();
+                    quit();


This double interrupt is confusing. Worth added a comment explaining which thread is executing this listener method, and pointing out that Thread.currentThread().interrupt() is reasserting the interrupt on the calling thread, while the call to interrupt() inside quit() is interrupting the CacheDispatcher thread?

…cher.

joebowbeer

I like these changes. Suggest static nested class to make the interactions between listener and dispatcher more explicit.

joebowbeer · 2017-09-10T20:55:43Z

src/main/java/com/android/volley/CacheDispatcher.java

@@ -48,6 +52,9 @@
    /** Used for telling us to die. */
    private volatile boolean mQuit = false;

+    /** Manage list of waiting requests and de-duplicate requests with same cache key. */
+    private WaitingRequestHandler mWaitingRequestHandler;


make this final

joebowbeer · 2017-09-10T20:57:40Z

src/main/java/com/android/volley/CacheDispatcher.java

@@ -64,6 +71,7 @@ public CacheDispatcher(
        mNetworkQueue = networkQueue;
        mCache = cache;
        mDelivery = delivery;
+	mWaitingRequestHandler = new WaitingRequestHandler();


Declare static nested so the interactions with CacheDispatcher are explicit: WaitingRequestHandler(this)

joebowbeer · 2017-09-10T20:59:01Z

src/main/java/com/android/volley/CacheDispatcher.java

@@ -154,4 +172,104 @@ public void run() {
            }
        }
    }
+
+    private class WaitingRequestHandler implements Request.NetworkRequestCompleteListener {


private static class - to make the interactions with CacheDispatcher explicit.

Thanks for all the great comments! I'm new to Java, so I'm learning a lot here

joebowbeer · 2017-09-10T20:59:25Z

src/main/java/com/android/volley/CacheDispatcher.java

+		return;
+	    }
+	    String cacheKey = request.getCacheKey();
+	    Queue<Request<?>> waitingRequests;


declaration and assignment on single line.

jjoslin

@uhager - the structure looks good overall and I like the introduction of the inner class. I think we can get this in as soon as the tests are passing.

jjoslin · 2017-09-11T18:16:55Z

src/main/java/com/android/volley/CacheDispatcher.java

@@ -64,6 +71,7 @@ public CacheDispatcher(
        mNetworkQueue = networkQueue;
        mCache = cache;
        mDelivery = delivery;
+	mWaitingRequestHandler = new WaitingRequestHandler(this);


nit: the indentation here seems to be off by 1 space.

jjoslin

Thanks for the changes.

joebowbeer

After looking at the use of Queue, I see that an ordinary List would work great, and would be more standard (meaning that's what most Java programmers would do in this situation), and then you can use the superior ArrayList class.

jjoslin · 2017-09-11T21:56:08Z

@joebowbeer I think that's a reasonable suggestion but I'd prefer to see it done as a separate pull request.

jpd236

+1 to keeping that separate.

Looks good here except for a couple nits and one observation that would be good to get your thoughts on; once addressed I can merge.

jpd236 · 2017-09-11T22:45:29Z

src/main/java/com/android/volley/CacheDispatcher.java

@@ -48,6 +52,9 @@
    /** Used for telling us to die. */
    private volatile boolean mQuit = false;

+    /** Manage list of waiting requests and de-duplicate requests with same cache key. */
+    private final WaitingRequestHandler mWaitingRequestHandler;


nit: Handler is a loaded term on Android (https://developer.android.com/reference/android/os/Handler.html), so for clarity's sake it would be good to avoid using it if we're not actually extending that class.

Perhaps "WaitingRequestManager" or "WaitingRequestDispatcher"?

Went with Manager.

jpd236 · 2017-09-11T22:52:08Z

src/main/java/com/android/volley/CacheDispatcher.java

+
+	/** Request received a valid response that can be used by other waiting requests. */
+	@Override
+	public synchronized void onResponseReceived(Request<?> request, Response<?> response) {


A key change here (since the last time I've looked at this) is that you are now holding a lock (via synchronized on the method) for all of the calls to postResponse. Previously it was not held during that time. I think this is probably reasonable given that a sane implementation will simply dispatch the response without actually waiting for the response to be handled (and that's the default behavior of ExecutorDelivery) but it's perhaps worth noting that a different delivery (i.e. ImmediateResponseDelivery, though this is only in the test code) would cause this lock to be held during response callback handling.

To summarize, no change requested here, but it'd be good to think through this angle one more time to make sure that there isn't a major increase in contention by holding a lock for each of these methods' entire bodies.

I was going to comment on this as well: "Strive for open calls to avoid deadlock" being the best practice. In other words, don't hold a lock when calling other code.

In this case, the lock can be shrunk quite a bit:

[Hey. There are HARD tabs in this file. Is that cool?]

Queue<Request<?>> waitingRequests; synchronized (this) { waitingRequests = mWaitingRequests.remove(cacheKey); } if (waitingRequests != null) { if (VolleyLog.DEBUG) { VolleyLog.v("Releasing %d waiting requests for cacheKey=%s.", waitingRequests.size(), cacheKey); } // Process all queued up requests. for (Request<?> waiting : waitingRequests) { mCacheDispatcher.mDelivery.postResponse(waiting, response); } }

Yeah, I think that's how it was in the initial PR, so we should probably move back to that unless there is a thread-safety issue. Just need to tread real carefully there :)

And for tabs, I'm not as used to GitHub's review tool so I haven't caught whitespace issues like that, but yes, we should be using spaces and not tabs. #1 mentions integrating google-java-format into the workflow here which would resolve that more automatically FYI.

I had that initially, but made it all synchronized for the inner class to avoid synchronized blocks. I thought that was part of the point of the nested class to have clearer synchronization by synchronizing the methods? But I can revert that.
Apologies for the tabs, I was working on that in emacs at home, I'll have to check my configurations.

@uhager sorry for the added confusion. The inner class separates concerns nicely, but the need for open calls adds a small complication. Thanks for addressing.

jpd236 · 2017-09-11T22:55:08Z

src/main/java/com/android/volley/CacheDispatcher.java

+		    mCacheDispatcher.mNetworkQueue.put(nextInLine);
+		} catch (InterruptedException iex) {
+		    VolleyLog.e("Couldn't add request to queue. %s", iex.toString());
+		    // Restore the interrupted status of the calling thread (i.e. NetworkDIspatcher)


nit: NetworkDispatcher

…tManager, fixed some tabs.

uhager added 3 commits August 14, 2017 15:47

Fix soft TTL for duplicate requests: when a soft TTL cache entry exis…

1980493

…ts, that response should be served immediately rather than having duplicate requests wait for the first request's network response.

Fix soft TTL for duplicate requests: when a soft TTL cache entry exis…

bb5edb2

…ts, that response should be served immediately rather than having duplicate requests wait for the first request's network response.

Fix soft TTL for duplicate requests: when a soft TTL cache entry exis…

cf36875

…ts, that response should be served immediately rather than having duplicate requests wait for the first request's network response.

jpd236 requested a review from yilanl August 15, 2017 22:36

jpd236 requested a review from jjoslin September 8, 2017 20:16

jjoslin suggested changes Sep 8, 2017

View reviewed changes

uhager added 2 commits September 8, 2017 17:30

Fixed some issues with synchronization and resetting interrupted thre…

c000907

…ads.

Fixed bug introduced in previous commit.

a7e7a4f

joebowbeer suggested changes Sep 9, 2017

View reviewed changes

jpd236 reviewed Sep 10, 2017

View reviewed changes

joebowbeer reviewed Sep 10, 2017

View reviewed changes

uhager added 2 commits September 10, 2017 12:56

Some minor review fixes.

620d2ab

Moved NetworkRequestCompleteListener into nested class in CacheDispat…

b3bfd99

…cher.

joebowbeer suggested changes Sep 10, 2017

View reviewed changes

Make nested class WaitingRequestHandler static.

0247408

jjoslin suggested changes Sep 11, 2017

View reviewed changes

Fixed indentation.

41ba89f

jjoslin approved these changes Sep 11, 2017

View reviewed changes

joebowbeer reviewed Sep 11, 2017

View reviewed changes

jpd236 requested changes Sep 11, 2017

View reviewed changes

Narrowed synchronization in onResponseReceived, renamed WaitingReques…

4202334

…tManager, fixed some tabs.

jpd236 approved these changes Sep 12, 2017

View reviewed changes

jpd236 merged commit b2bb59a into google:master Sep 12, 2017

Fix soft TTL for duplicate requests #73

Fix soft TTL for duplicate requests #73

Conversation

uhager commented Aug 14, 2017

jjoslin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joebowbeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joebowbeer Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpd236 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpd236 commented Sep 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joebowbeer Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joebowbeer commented Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

joebowbeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoslin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoslin left a comment

Choose a reason for hiding this comment

joebowbeer left a comment

Choose a reason for hiding this comment

jjoslin commented Sep 11, 2017

jpd236 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpd236 Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joebowbeer Sep 10, 2017 •

edited

Loading

joebowbeer Sep 10, 2017 •

edited

Loading

joebowbeer commented Sep 10, 2017 •

edited

Loading

jpd236 Sep 11, 2017 •

edited

Loading