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

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

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.

Done.

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.

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?

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.

code style nit:

synchronized (mLock) {

Move this to where it is assigned.

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

Added a check.

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).

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.

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).

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.

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

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?

make this final

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

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

declaration and assignment on single line.

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

Done.

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.

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.

nit: NetworkDispatcher