Clean up cache initialization in AsyncRequestQueue. #380
Conversation
The previous implementation had two key flaws: - It performed a blocking task in the non-blocking thread pool - It failed to prevent requests from attempting cache reads prior to initialization completing, assuming the relevant thread pool has more than one available thread. To resolve this, we perform initialization asynchronously. Until it completes, we hold all queued requests in a separate queue. In practice, initialization should finish quickly, and it should be rare for this to have any impact on the request flow. Chained futures would make for a cleaner implementation - we could store a future tied to initialization and add listeners upon future completion to begin the request, avoiding the need to track these requests ourselves and ensure thread safety - but they are unfortunately not usable in Volley, which only supports Java 7. Fixes google#379
| if (mAsyncCache != null) { | ||
| final CountDownLatch latch = new CountDownLatch(1); | ||
| // Kick off cache initialization, which must complete before any requests can be processed. | ||
| if (mAsyncCache != null) { |
There was a problem hiding this comment.
It's out of scope here, but it would be a bit easier to follow if the notion of initializing cache was abstracted, so start() doesn't have to deal directly with checking which cache is in use and just try to initialize "a" cache.
Since it requires a refactor of the class, I don't think we should do it now. But something to keep in mind as a future clean up, when we do further modifications to the async code (like making it thread safe?)
There was a problem hiding this comment.
It could make sense to fork off some helper classes from AsyncRequestQueue for the cache and network layers, similar to how CacheDispatcher / NetworkDispatcher end up abstracting those details away from RequestQueue. I'm hesitant to push it all the way down to the cache layer unless we can find a way to avoid having to duplicate logic in each implementation. (We're somewhat hampered by Cache being an interface rather than an abstract class, which prevents us from being able to expose equivalent initialization methods in both.)
| @Test | ||
| public void requestsQueuedBeforeCacheInitialization_asyncCache() { | ||
| AsyncCache mockAsyncCache = mock(AsyncCache.class); | ||
| queue = createRequestQueue(mockAsyncCache); |
There was a problem hiding this comment.
Could queue be a local var instead? Since it's not the one set during setup?
The previous implementation had two key flaws:
initialization completing, assuming the relevant thread pool has
more than one available thread.
To resolve this, we perform initialization asynchronously. Until
it completes, we hold all queued requests in a separate queue.
In practice, initialization should finish quickly, and it should
be rare for this to have any impact on the request flow.
Chained futures would make for a cleaner implementation - we could
store a future tied to initialization and add listeners upon
future completion to begin the request, avoiding the need to track
these requests ourselves and ensure thread safety - but they are
unfortunately not usable in Volley, which only supports Java 7.
Fixes #379