Get Implementation in AsyncCache that takes in a callback function #344

sphill99 · 2020-06-15T23:01:41Z

Implemented the get function in AsyncCache that takes in a callback function in two different ways. In the one (DiskBasedAsyncCache), it uses an asynchronous file channel, and for the fallback version, it works very similarly to the current DiskBasedCache. Most of the code in those files are from that, aside from the get method. There are not tests yet for this, as not enough has really been implemented to test it, but was more to check the direction of the code.

jbiral · 2020-06-16T15:40:37Z

Hi Scott, I'd recommend to update the PR title to be a bit more descriptive. I find that the first sentence of your description provides much more context on what this PR is about, so feel free to use it as the title.

Descriptive titles are important is it allows to quickly get a sense of what the PR is about.

jbiral · 2020-06-16T15:54:30Z

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

+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicReference;
+
+public abstract class AsyncCache implements Cache {


I'd recommend to add a descriptive doc on how to use this cache implementation. It has a very different intended usage over the original interface (e.g. it's expected to use the async get/put/etc. with a callback rather than using what the Cache interface defines), so it's worth clarifying in the class documentation.

Here's a link to how it's recommended to write javadoc for Google projects: https://google.github.io/styleguide/javaguide.html#s7-javadoc

In addition to adding top-level class javadoc, could you document each public interface, class, methods, etc. that are part of this file? It's OK not to write doc for what's define by the Cache interface, though I'd still suggest to add a note to use the async methods instead.

jbiral · 2020-06-16T16:01:02Z

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

+        final AtomicReference<Entry> entryRef = new AtomicReference<>();
+        get(
+                key,
+                new OnGetCompleteCallback() {


nit: we could use a lambda expression here for simplicity (I believe all version of Android can now use a lambda when using a recent Android Gradle plugin)

This would convert your code into:

get( key, entry -> { entryRef.set(entry); latch.countDown(); } );

For context, lambdas are like a more concise way to write an anonymous class. Feel free to take a look at the official java doc about lambdas if it's the first time you hear about them / use them: https://docs.oracle.com/javase/tutorial/java/javaOO/lambdaexpressions.html

When I tried to make that change, it says lambda expressions aren't allowed in Java 7. Even though I thought Volley uses java8. I'll look into this.

Volley actually is built with minSdkVersion = 8, which is actually Java 6 :)

Lambdas have been backported in principle per https://developer.android.com/studio/write/java8-support - not sure offhand how far back this goes. But I'm not 100% sure what the story is offhand in terms of exposing a library that relies on this support.

While it's not ideal, unless we can pretty quickly and confidently find an answer suggesting we can safely use lambdas without introducing major complexity for apps depending on the library using this desugaring support, we should probably avoid them except when writing code that is guaranteed to run on the right minSdkVersion.

Note that this also includes try-with-resources.

FWIW, I'm open to considering increasing the minSdkVersion somewhat. At this point it's exceedingly rare to target anything under API 14, which is what the Android support library requires; that seems like a reasonable point to increase it, if that'd be helpful. But it'd be nice to have a clear benefit to doing so, and 14 won't get us native try-with-resources or lambdas AFAIK.

To be clear, the current Java target will support try-with-resources if sdk >= 19

compileOptions { sourceCompatibility JavaVersion.VERSION_1_7 targetCompatibility JavaVersion.VERSION_1_7 }

Though I think try-with-resources can obscure what's going on, esp. in nested and other complex situations, so no loss of any significance.

https://stackoverflow.com/a/19470405/901597

Java 8 is needed for lambda.

jbiral · 2020-06-16T16:04:12Z

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

+    // TODO: Handle callback never being invoked? Match behavior when sync cache throws
+    // RuntimeException.
+
+    @Override


Could you also annotate the method as @Nullable? It acts a documentation to tell the caller that the return value could be null.

jbiral · 2020-06-16T16:05:28Z

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

+public abstract class AsyncCache implements Cache {
+
+    public interface OnGetCompleteCallback {
+        void onGetComplete(Entry entry);


Since entry can be null, please annotate the parameter as @Nullable, so whoever implements the class will be aware that the method parameter can be null.

jbiral · 2020-06-16T16:07:02Z

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

+        }
+    }
+
+    // TODO: Implement the rest.


nit: did you mean to move this below put?

jbiral · 2020-06-16T16:14:22Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                            cb.onGetComplete(null);
+                        }
+                    });
+        } catch (Exception e) {


The DiskBasedCache only catchtes IOExceptions. Is there a reason why we want to catch any Exceptions in here?

Oh no, I think I just put that there when I first wrote the try/catch, and never changed it. I'll fix that now.

jbiral · 2020-06-16T16:16:28Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+
+    /** Handles holding onto the cache headers for an entry. */
+    @VisibleForTesting
+    static class CacheHeader {


Could CacheHeader be shared between this class and DiskBaseCache? The implementations don't look the same at this time, but just wondering what's the long term plan and if it would make sense to extract the class to a top-level class so it can be shared.

Yeah, I was thinking about making another abstract class that these two extend. This would allow us to not repeat the shared methods/class. Currently, I think the write and read methods would be implemented differently, so I don't know how that'd work. But I think that's definitely better to try to do that.

jbiral · 2020-06-16T16:18:27Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCacheFallback.java

+import java.util.List;
+import java.util.Map;
+
+public class DiskBasedAsyncCacheFallback extends AsyncCache {


Could you also add doc here to explain how the class differs from DiskBasedAsyncCache?

jbiral · 2020-06-16T16:19:20Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCacheFallback.java

+        File file = getFileForKey(key);
+
+        try {
+            DiskBasedAsyncCacheFallback.CountingInputStream cis =


I believe you can use a try-with-resources here as well.

It says that it requires an API of 19 or higher to do a try-with-resources.

jbiral · 2020-06-16T16:19:59Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCacheFallback.java

+        File file = getFileForKey(key);
+
+        try {
+            DiskBasedAsyncCacheFallback.CountingInputStream cis =


nit: I think you may not need to add the DiskBaseAsyncCacheFallback prefix since it's an inner class of this class. (same below)

yep, just fixed this.

sophiahaim · 2020-06-16T16:15:35Z

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

+
+    public abstract void get(String key, OnGetCompleteCallback callback);
+
+    // TODO: Handle callback never being invoked? Match behavior when sync cache throws


nit: Add the issue in parentheses:
// TODO(#181): Handle...

sophiahaim · 2020-06-16T16:23:32Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+    @Override
+    public void get(String key, OnGetCompleteCallback callback) {
+        final OnGetCompleteCallback cb = callback;
+        final DiskBasedAsyncCache.CacheHeader entry = mEntries.get(key);


Check that entry is non-null

sophiahaim · 2020-06-16T16:27:13Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+            afc.read(
+                    buffer,
+                    0,
+                    buffer,


Add args to these two to clarify what they are:
/* position= / 0,
/ attachment= */ buffer

added those

sophiahaim · 2020-06-16T16:31:42Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+        // TODO (sphill99): Implement
+    }
+
+    /**


I think some of these common functions that are the same between your new Cache implementations and the existing one could be factored out into a Util that all three classes could call

You may be able to do the same with some of the CONSTANTS at the top of the file that seem to be the same for all three

Yeah, I want to extract some of the stuff that's repeated out to a higher level class. Should I do that for this PR?

joebowbeer · 2020-06-16T17:21:27Z

@sphill99 What is test coverage for this new code? I don't see any added tests.

I recommend high test coverage. A failure in an edge case can take years to find and correct (#12). Please don't lose any of the existing coverage in a refactor.

sphill99 · 2020-06-16T19:10:16Z

@joebowbeer I still haven't written tests for this function. I'm going to write tests for the AsyncCache as a whole, including this implementation. I just wanted to get the general structure reviewed before I move further with the implementations. Currently these implementations aren't being used.

I'll make sure to keep the existing coverage.

…ions. Still need to add javadocs

sophiahaim · 2020-06-18T15:24:40Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                    AsynchronousFileChannel.open(path, StandardOpenOption.READ);
+            ByteBuffer buffer = ByteBuffer.allocate((int) file.length());
+            afc.read(
+                    /* destination */ buffer,


nit: add "="
e.g. /* destination= */ buffer

sophiahaim · 2020-06-18T15:33:15Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCacheFallback.java

+    /** The maximum size of the cache in bytes. */
+    private final int mMaxCacheSizeInBytes;
+
+    /** Default maximum disk usage in bytes. */


Are these used anywhere yet? If not, I think hold off on including them until they're used.

When you do need to add them, I think you can put them in your new Utility class since they are the same for both implementations (async and fallback)

I removed these for now

sophiahaim · 2020-06-18T15:39:18Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCacheFallback.java

+
+    /** Returns the cache entry with the specified key if it exists, null otherwise. */
+    @Override
+    public synchronized void get(String key, OnGetCompleteCallback callback) {


This is really similar to the existing DiskBasedCache, except instead of returning null or entry.toCacheEntry, you call callback.onGetComplete(null/entry.toCacheEntry)

I'm wondering if it would be possible to use the existing DiskBasedCache in here and just call #get, then do callback.onGetComplete with whatever it returns. There may be a couple ways of approaching this, so happy to discuss more offline too.

I think it would generally simplify this class if you can leverage the existing implementation.

+1 - for the fallback, we should try to share as much code with possible as the existing implementation rather than duplicating it.

In fact, though, I wonder if we really want to have a second class at all. The reality is that it's confusing to have a class extend AsyncCache but not actually be Async - how should the user of this object determine whether the cache supports true Async operation (and thus doesn't need to be called on a blocking thread) or not (and thus does)?

The class which uses this will ultimately need to make a decision based on the capabilities of the cache to either use the fully async API or else send the task to a blocking thread pool. But in the latter case, there's actually no reason to use the async API methods, since they're blocking anyway.

This might become clearer when we actually implement the async cache handling step, but my instinct here is that we actually don't need this class, and should instead have the dispatcher accept either a vanilla Cache (which defaults to DiskBasedCache) or an AsyncCache (which defaults to DiskBasedAsyncCache, where supported). If the cache is an instance of AsyncCache, it knows it can safely use the async methods. If not, it should use the fallback and schedule the read for a blocking background thread.

I changed it so the fallback now doesn't repeat the code. But yeah, that makes sense.

That's certainly better, but then why would we need it at all per my comment? Would it make sense to hold off on adding it unless/until we see how it would be helpful/needed, or do you see a reason that the next level up would make use of this, given that it will need to know how to dispatch the cache operations?

oh okay, I understand what you're saying now. I just deleted it in the last commit.

jpd236 · 2020-06-18T16:20:02Z

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

+
+    @Nullable
+    @Override
+    public Entry get(String key) {


I think we should mark the legacy methods as final - subclasses of AsyncClass should not be modifying these implementations, only implementing the async versions. (At least until someone can provide a valid use case for it; until then, the tighter we keep the API, the stronger the guarantees the implementation can offer).

jpd236 · 2020-06-18T16:23:09Z

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

+        try {
+            latch.await();
+            return entryRef.get();
+        } catch (InterruptedException e) {


We should probably log the exception here as well (with VolleyLog). Otherwise, the operation might not have gone through, but apart from the thread's interrupt status (which is often unchecked by callers), there'd be no visible indication that something had been canceled.

Consider awaitUninteruptibly. Is this thread exposed enough to be interrupted? If not, this handling is just another section of untested code.

awaitInterruptibly isn't a method on CountDownLatch.

While I agree interrupt handling is a mess in Java, I think we can at least ensure we handle them correctly to the extent possible within Volley (we do have RequestQueue#stop which would stop the executors if we control them).

jpd236 · 2020-06-18T16:30:23Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCacheFallback.java

+
+    /** Returns the cache entry with the specified key if it exists, null otherwise. */
+    @Override
+    public synchronized void get(String key, OnGetCompleteCallback callback) {


+1 - for the fallback, we should try to share as much code with possible as the existing implementation rather than duplicating it.

In fact, though, I wonder if we really want to have a second class at all. The reality is that it's confusing to have a class extend AsyncCache but not actually be Async - how should the user of this object determine whether the cache supports true Async operation (and thus doesn't need to be called on a blocking thread) or not (and thus does)?

The class which uses this will ultimately need to make a decision based on the capabilities of the cache to either use the fully async API or else send the task to a blocking thread pool. But in the latter case, there's actually no reason to use the async API methods, since they're blocking anyway.

This might become clearer when we actually implement the async cache handling step, but my instinct here is that we actually don't need this class, and should instead have the dispatcher accept either a vanilla Cache (which defaults to DiskBasedCache) or an AsyncCache (which defaults to DiskBasedAsyncCache, where supported). If the cache is an instance of AsyncCache, it knows it can safely use the async methods. If not, it should use the fallback and schedule the read for a blocking background thread.

jpd236 · 2020-06-18T16:31:21Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+    private final int mMaxCacheSizeInBytes;
+
+    /** Default maximum disk usage in bytes. */
+    private static final int DEFAULT_DISK_USAGE_BYTES = 5 * 1024 * 1024;


can we share magic constants like this with DiskBasedCache at least until we need to fork them? e.g. by putting them in DiskBasedCacheUtility

jpd236 · 2020-06-18T16:33:34Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                        @Override
+                        public void completed(Integer result, ByteBuffer attachment) {
+                            if (attachment.hasArray()) {
+                                int offset = attachment.arrayOffset();


nit: looks like this variable is unused

For some reason, you can't call attachment.array() without having called hasArray() and arrayOffset() beforehand. Some bytebuffers don't have a backing array, but since we always pass one in that does, it always will. I'm not sure why simply checking if it hasArray doesn't suffice, but it doesn't build otherwise.

jpd236 · 2020-06-18T16:36:27Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                    /* attachment */ buffer,
+                    new CompletionHandler<Integer, ByteBuffer>() {
+                        @Override
+                        public void completed(Integer result, ByteBuffer attachment) {


wondering whether for safety, we need to read the value of result and truncate the entry to that size (/ handle if it is -1, per the Javadoc).

With the old cache, we'd occasionally see strange bugs due to files being corrupted, possibly due to parallel access. While that's a misuse of Volley if so, it is good if we can gracefully fall back to the extent possible. And it's possible the size changes between when we read the length / allocate the buffer and when we complete the actual data read. So the buffer might have garbage data at the end (if the file was truncated), or might be incomplete (if it grew). In both cases we should probably assume the entry is invalid and avoid returning it.

I added a check to see if the file length is the same as the size allocated before the read.

jpd236 · 2020-06-18T16:36:46Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                        }
+
+                        @Override
+                        public void failed(Throwable exc, ByteBuffer attachment) {


log the exception so the failure is visible

jpd236 · 2020-06-18T16:36:52Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                        }
+                    });
+        } catch (IOException e) {
+            cb.onGetComplete(null);


log exception

better, but see above note regarding how to handle errors

Line 106 is still potentially a second invocation of callback.

See comment above, and this example of another way to structure this:

https://github.com/google/volley/blob/master/src/main/java/com/android/volley/toolbox/DiskBasedCache.java#L148

sphill99 · 2020-06-18T20:21:41Z

I should have made appropriate edits for all the PR comments, or left questions on them. It is ready to be reviewed again.

jbiral · 2020-06-18T20:48:09Z

src/main/java/com/android/volley/toolbox/CacheHeader.java

+    /**
+     * The size of the data identified by this CacheHeader on disk (both header and data).
+     *
+     * <p>Must be set by the caller after it has been calculated.
+     *
+     * <p>This is not serialized to disk.
+     */


This comment looks intended for long size; below

jbiral · 2020-06-18T20:49:03Z

src/main/java/com/android/volley/toolbox/CacheHeader.java

+    final String key;
+
+    /** ETag for cache coherence. */
+    final String etag;


nit: please annotate as @Nullable

jbiral · 2020-06-18T21:08:14Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+        File file = getFileForKey(key);
+        Path path = Paths.get(file.getPath());
+        try {
+            AsynchronousFileChannel afc =


Since we can't use try-with-resources. make sure to close the file channel when the try/catch block is completed

jpd236 · 2020-06-18T22:29:25Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                        public void completed(Integer result, ByteBuffer attachment) {
+                            // if the file size changes, return null
+                            if (size != file.length()) {
+                                cb.onGetComplete(null);


maybe log a warning in this case since it implies the file changed while we were reading it.

Can this mean that the read terminated early? I would treat this as a failure.

I think it means the file changed between the time we opened the channel and finished the reading. The size allocated and the file length shouldn't be affected by the success of the read. I think if the read fails, it would never reach the completed block.

In either case, it sounds like an error to me. I don't think it serves much purpose to check the file length if it will be ignored for all practical purposes. I would either ignore the file length and use the bytes read, if this is reliable, or use the file length as a correctness check, if not.

I think the confusion here is - this should say "if (size != result)". We shouldn't compare the file size before and after - we should compare the size of the file when we allocated the buffer to the number of bytes we actually read.

but the number of bytes we actually read is dependent upon the size of the buffer we allocate. This would work in the case of the read bytes being less than the allocated, but it will never be more.

true. but we should still look at the number of bytes read and make sure it's not less, which could happen even if the file size is now the same.

I think checking for result != size || current file size != size would be reasonable. Or, just the former, and let the latter be handled by integrity checks on the cache header.

Okay, I'll make that fix

joebowbeer

Fix potential second callback.

joebowbeer · 2020-06-19T15:59:19Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                    afc.close();
+                } catch (IOException e) {
+                    VolleyLog.d("%s: %s", file.getAbsolutePath(), e.toString());
+                    cb.onGetComplete(null);


Here you are calling the callback a second time.

Consider a nested try-finally try-catch. One to make sure afc is closed and one to callback on IOException. There may be an example of this pattern in the disk based cache.

oh thanks, just fixed

sphill99 · 2020-06-19T16:37:19Z

I think I addressed all PR comments. It is ready to be reviewed again.

jpd236

Thanks - most of my comments now are about rejiggering small pieces rather than major architectural changes, so I think this is getting closer. Once we stamp out the general patterns here, hopefully the remaining methods won't need a lot of round trips.

jpd236 · 2020-06-19T20:05:03Z

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

+        try {
+            latch.await();
+            return entryRef.get();
+        } catch (InterruptedException e) {


awaitInterruptibly isn't a method on CountDownLatch.

While I agree interrupt handling is a mess in Java, I think we can at least ensure we handle them correctly to the extent possible within Volley (we do have RequestQueue#stop which would stop the executors if we control them).

jpd236 · 2020-06-19T20:10:44Z

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

+                new OnPutCompleteCallback() {
+                    @Override
+                    public void onPutComplete(boolean success) {
+                        if (success) {


AFAICT we need to count down the latch regardless of whether the put succeeds or fails. If we don't, the caller will just be blocking indefinitely even though nothing is still happening.

put doesn't provide a way to return an error, so we should make sure it's logged somewhere in the stack, but otherwise we should just return control to the caller once the put is done.

would it be logged in this method, or in the async implementation, where we know what happened?

jpd236 · 2020-06-19T20:14:06Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+
+/**
+ * AsyncCache implementation that uses Java NIO's AsynchronousFileChannel to perform asynchronous
+ * disk reads and writes. This should only be used by devices with an API level of 26 or above.


I think you can remove this sentence and the @SuppressLint annotation and just add a @RequiresApi(Build.VERSION_CODES.O) instead. This is preferable because:

Instead of suppressing all lint checks, it only disables them for APIs added up through O. If you make an API call to something added in P, it will still fail, which is good as we would need additional checks.

It will enforce that the caller checks for O+ before using the class (or that it has a RequiresApi annotation of its own)

Its self-documenting - no need to repeat it in the Javadoc.

jpd236 · 2020-06-19T20:16:25Z

src/main/java/com/android/volley/toolbox/CacheHeader.java

+import java.util.List;
+
+/** Handles holding onto the cache headers for an entry. */
+@VisibleForTesting


this annotation doesn't make sense here; this class needs to be package-visible in order for other classes in this package to use it.

We use this annotation when we increase the visibility beyond what it otherwise would be for the sole purpose of making it accessible in a unit test.

You should see Android Studio complaining about this, e.g. where DiskBasedAsyncCache is calling:

cb.onGetComplete(entry.toCacheEntry(data));

because it thinks toCacheEntry is only visible for testing, but here we're using it from prod code.

jpd236 · 2020-06-19T20:18:27Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                            public void completed(Integer result, ByteBuffer attachment) {
+                                // if the file size changes, return null
+                                if (size != file.length()) {
+                                    VolleyLog.d(


logging stuff:

use e instead of d - this is an error, not debugging info

You can do VolleyLog.e("File changed while reading: %s", file.getAbsolutePath()) - no need to use format args for a constant string. Also, I think it's clearer to have the error be the first thing, and specific details like the file name come second.

Here and throughout, it should be "%s", not "s%", no?

jpd236 · 2020-06-19T20:19:51Z

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

+                        if (success) {
+                            latch.countDown();
+                        } else {
+                            return;


no need for "return" statement as the last statement in a void method, here and below.

AS highlights these in yellow for me - do you not see that too? In general, we should aim to have no warnings in new code.

jpd236 · 2020-06-19T20:23:14Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                                    return;
+                                }
+                                if (attachment.hasArray()) {
+                                    final int offset = attachment.arrayOffset();


what's the exact error message you get when you remove this line?

I think it's probably an ErrorProne thing which should probably be suppressed by adding a suppression for that, not by introducing a new variable that is itself unused and a compiler warning. arrayOffset() does nothing internally; we know there is a backing array because we allocated with ByteBuffer.allocate() above.

The error is: "ByteBuffer.array() shouldn't be called unless ByteBuffer.arrayOffset() is used or if the ByteBuffer was initialized using ByteBuffer.wrap() or ByteBuffer.allocate().
byte[] data = attachment.array();
^
(see https://errorprone.info/bugpattern/ByteBufferBackingArray)"

I believe there is a way to just suppress the warning though. So I'll figure that out.

One option would be to just pass Void/null for the attachment parameter and access buffer directly - that would prove to error prone that it's actually the same object which came from ByteBuffer.allocate. As it stands, ErrorProne has no way of knowing this.

Yeah, I think that should work

jpd236 · 2020-06-19T20:24:20Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+
+                            @Override
+                            public void failed(Throwable exc, ByteBuffer attachment) {
+                                VolleyLog.d("%s: %s", file.getAbsolutePath(), exc.toString());


read failures are abnormal - use e, and log the full stack trace:

VolleyLog.e(exc, "Failed to read file %s", file.getAbsolutePath());

jpd236 · 2020-06-19T20:24:40Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                        }
+                    });
+        } catch (IOException e) {
+            cb.onGetComplete(null);


better, but see above note regarding how to handle errors

joebowbeer

Changes:

Remove AtomicReference
Fix double callback

Consider replacing CountDownLatch with Future or CompletionStage.

joebowbeer · 2020-06-20T14:58:11Z

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

+    @Override
+    public final Entry get(String key) {
+        final CountDownLatch latch = new CountDownLatch(1);
+        final AtomicReference<Entry> entryRef = new AtomicReference<>();


This use of AtomicReference is unnecessary and should be removed. (In general, AtomicReference is rarely needed except in lock-free code.)

CountDownLatch is also seldom needed because there are usually better choices in java.util.concurrent (see JCIP), esp. when the count is 1.

Looking at the intent of this synchronization, I think a Future "value" would be a better match here, and it can be canceled.

CompletableFuture is available in Android API 26. It provides a settable value that can be waited on. In fact, CompletableFuture's CompletionStage may be a great interface for this enhancement, as it supports composition and chaining, and enables better exception handling.

Ended up not having AsyncCache implement Cache at all, so there was no long a need for this.

joebowbeer · 2020-06-20T15:06:50Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+        final File file = getFileForKey(key);
+        final int size = (int) file.length();
+        Path path = Paths.get(file.getPath());
+        AsynchronousFileChannel afc = null;


I would move this inside the outer try, which should be a try-catch(IOException), then open a try-finally to close the afc on the next line:

try { final AsynchronousFileChannel afc = open() try { // I/O } finally { afc.close() } } catch (IOException e) { // handle e }

See https://github.com/google/volley/blob/master/src/main/java/com/android/volley/toolbox/DiskBasedCache.java#L148

Since the class now requires a higher android api, I switched it to use a try-with-resources. I think this should allow it to just be a simple try-catch, and gets rid of these issues.

joebowbeer · 2020-06-20T15:14:15Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                        }
+                    });
+        } catch (IOException e) {
+            cb.onGetComplete(null);


Line 106 is still potentially a second invocation of callback.

See comment above, and this example of another way to structure this:

https://github.com/google/volley/blob/master/src/main/java/com/android/volley/toolbox/DiskBasedCache.java#L148

…rs and other small changes

sphill99 · 2020-06-22T18:25:21Z

I've addressed all the comments and concerns. I updated the try-try finally-catch block, into a try-with-resources, now that we are requiring that SDK. I also made AsyncCache not implement Cache anymore.

jpd236

Just a few nits left.

For context - we decided to remove the blocking methods in AsyncCache because I don't think we'll need them. Since the calling code already needs to be able to support Cache or AsyncCache differently since we won't easily be able to provide an AsyncCache implementation pre-O (due to lack of nio classes in the platform), there isn't much benefit to having AsyncCache implement Cache; we're still going to want to keep a separate DiskBasedCache implementation as currently stands.

jpd236 · 2020-06-23T00:22:16Z

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

+import androidx.annotation.Nullable;
+
+/**
+ * AsyncCache is an abstract class which implements the Cache interface for backwards compatibility,


nit: update Javadoc now that we no longer implement Cache. My suggestion would just be:

/** Asynchronous equivalent to the {@link Cache} interface. */

As that is a simple summary of what this class is, and links to Cache in a more structured form so you can leverage the existing docs there if someone is looking for more detail.

jpd236 · 2020-06-23T00:27:05Z

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

+public abstract class AsyncCache {
+
+    public interface OnGetCompleteCallback {
+        void onGetComplete(@Nullable Cache.Entry entry);


nit: Could you add a Javadoc for this method? It doesn't have to be super-detailed, but one thing I think would be good to include is when/why entry would be null, and what we do in the event of an error. Something like:

/** * Invoked when the read from the cache is complete. * * @param entry The entry read from the cache, or null if the cache did not contain an entry for the given key or * if the entry couldn't be read. */

jpd236 · 2020-06-23T00:28:52Z

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

+    public abstract void get(String key, OnGetCompleteCallback callback);
+
+    public interface OnPutCompleteCallback {
+        void onPutComplete(boolean success);


Cache#put returns void today, so the introduction of a success boolean is a new concept that I'm not sure we should introduce. Since the focus here is get, I might suggest leaving this out for now.

jpd236 · 2020-06-23T00:30:15Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+    /** Returns the cache entry with the specified key if it exists, null otherwise. */
+    @Override
+    public void get(String key, OnGetCompleteCallback callback) {
+        final OnGetCompleteCallback cb = callback;


I think this line of code is only needed to mark this as final so the inner class can use it? If so, you can just mark the callback param as final (this doesn't require changes to AsyncCache) and use that directly.

yep, I didn't realize I could do that

jpd236 · 2020-06-23T00:31:45Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+                        }
+                    });
+        } catch (IOException e) {
+            VolleyLog.e("%s %s", file.getAbsolutePath(), e.toString());


VolleyLog.e(e, "Failed to read file %s", file.getAbsolutePath())

jpd236

Sorry, one other thing I realized - we have some common code moved out / copied from DiskBasedCache now so we can share it in DiskBasedAsyncCache, but DiskBasedCache is unchanged in this PR. Shouldn't it also be updated to leverage the new common classes? Otherwise we still have two copies of the logic. Or is the plan to do that separately? (That's probably reasonable, though we should plan to do it soon).

joebowbeer

I would like to see the copied code unit tested before this is merged, which is probably easiest done by refactoring DiskBasedCache and its unit tests to leverage/test the code in its new location.

In particular, there should only be one implementation of the version-stamped cache header, and it should be thoroughly tested.

I also hate to see any code moved/copied from under test to out-from-under test, even for a short period of time.

These tests are already written and run automatically. Why delay?

…ctored to reflect this change.

sphill99 · 2020-06-23T16:31:42Z

DiskBasedCache and DiskBasedCache tests now uses the shared classes. I moved some stuff (like CountingInputStream) back from DiskBasedCacheUtility to DiskBasedCache, since the async version does not use it. The tests were also refactored and test the code in its new location.

All comments should have been addressed now.

jbiral

Looks good to me, just a few nits! Please update the PR title now that AsyncCache only has one get method.

jbiral · 2020-06-23T16:42:08Z

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

+
+    public interface OnGetCompleteCallback {
+        /**
+         * Callback that's invoked when the read from the cache is complete.


Small nitpick, but prefer starting a method javadoc with a verb: Invoked when the read...

jbiral · 2020-06-23T16:45:15Z

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

+     * Retrieves an entry from the cache and send it back through the callback function
+     *
+     * @param key Cache key
+     * @param callback Callback function that sets the retrieved entry and lets the program know it


nit: Callback that will be notified when the information has been retrieved is a bit more concise

jbiral · 2020-06-23T16:56:53Z

src/main/java/com/android/volley/toolbox/DiskBasedCache.java

@@ -374,14 +369,21 @@ private void removeEntry(String key) {
        }
    }

+    InputStream createInputStream(File file) throws FileNotFoundException {


Why are these no longer @VisibleForTesting?

I had moved them outside of DiskBasedCache, but now since they're there again, I'll add it back

nit: it would lower the diff output if you moved them back to the same location they were in before, which helps in reviewing

jbiral · 2020-06-23T16:58:55Z

src/main/java/com/android/volley/toolbox/CacheHeader.java

+/** Handles holding onto the cache headers for an entry. */
+class CacheHeader {
+    /** Magic number for current version of cache file format. */
+    private static final int CACHE_MAGIC = 0x20150306;


Could you use the CACHE_MAGIC from DiskBasedCacheUtility instead? This way, we avoid duplicate code and potential mismatches in the future if one is modified but not the other.

jbiral · 2020-06-23T17:00:14Z

src/main/java/com/android/volley/toolbox/DiskBasedCacheUtility.java

+    @VisibleForTesting static final float HYSTERESIS_FACTOR = 0.9f;
+
+    /** Magic number for current version of cache file format. */
+    static final int CACHE_MAGIC = 0x20150306;


Note that I don't see this being used outside of CacheHeader, so you could consider removing it from here and keep the one in CacheHeader

Yeah, that's smart. I'll make that change.

jbiral · 2020-06-23T17:09:12Z

src/main/java/com/android/volley/toolbox/DiskBasedAsyncCache.java

+    }
+
+    /** Returns a file object for the given cache key. */
+    public File getFileForKey(String key) {


Should this be private?

joebowbeer · 2020-06-23T17:17:21Z

DiskBasedCache and DiskBasedCache tests now uses the shared classes. I moved some stuff (like CountingInputStream) back from DiskBasedCacheUtility to DiskBasedCache, since the async version does not use it. The tests were also refactored and test the code in its new location.

Thanks!

joebowbeer

Thanks for preserving the tests! I don't have any more changes to request. If you find an outstanding request, please decline it.

jpd236 · 2020-06-23T21:20:25Z

src/main/java/com/android/volley/toolbox/DiskBasedCache.java

@@ -374,14 +369,21 @@ private void removeEntry(String key) {
        }
    }

+    InputStream createInputStream(File file) throws FileNotFoundException {


nit: it would lower the diff output if you moved them back to the same location they were in before, which helps in reviewing

jpd236 · 2020-06-23T21:22:21Z

src/test/java/com/android/volley/toolbox/DiskBasedCacheTest.java

@@ -501,86 +503,87 @@ public void testCountingInputStreamByteCount() throws IOException {
    public void testEmptyReadThrowsEOF() throws IOException {
        ByteArrayInputStream empty = new ByteArrayInputStream(new byte[] {});
        exception.expect(EOFException.class);
-        DiskBasedCache.readInt(empty);
+        DiskBasedCacheUtility.readInt(empty);


For tests which solely exercise methods in DiskBasedCacheUtility, we should move their tests to a new DiskBasedCacheUtilityTest rather than having them in here, since they aren't specific to DiskBasedCache any more.

Scott, did you upload a new DiskBasedCacheUtilityTest class for the tests you removed from here? I don't see one currently

…here they were.

sophiahaim · 2020-06-24T12:42:14Z

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

+
+    /**
+     * Retrieves an entry from the cache and send it back through the callback function
+     *


nit: "sends it back"

Also would be good to link to the callback – {@link OnGetCompleteCallback#onGetComplete}

changed this. I realized I never added the DiskBasedCacheUtilityTest class to the commit, since it was untracked. Should have just fixed that as well.

sophiahaim · 2020-06-24T12:52:33Z

src/test/java/com/android/volley/toolbox/DiskBasedCacheTest.java

@@ -501,86 +503,87 @@ public void testCountingInputStreamByteCount() throws IOException {
    public void testEmptyReadThrowsEOF() throws IOException {
        ByteArrayInputStream empty = new ByteArrayInputStream(new byte[] {});
        exception.expect(EOFException.class);
-        DiskBasedCache.readInt(empty);
+        DiskBasedCacheUtility.readInt(empty);


Scott, did you upload a new DiskBasedCacheUtilityTest class for the tests you removed from here? I don't see one currently

sphill99 · 2020-06-24T13:59:34Z

Uploaded the new test class and fixed the javadocs mistakes.

joebowbeer · 2020-06-24T16:21:03Z

By the way, there are 100+ warnings in the Travis build mostly/all about annotations:

https://travis-ci.org/github/google/volley/builds/701650084

I wish the Travis build (or GitHub Action?) reported code coverage.

jpd236 · 2020-06-24T23:19:03Z

Filed #345 for the warnings - these are unrelated to this PR and not a major problem since they only impact the Javadoc (if at all). Coverage is a nice to have, but I don't see it as super critical since we can watch for it in PRs; the project is not that big and doesn't have many people approving changes.

jpd236

Will wait for @jbir789 to sign off in case he has further comments before merging, but LGTM. Thanks!

Scott Phillips added 6 commits June 15, 2020 17:41

changed log prefix

34c5cac

linked javadocs, changed log prefix, and switched parameter name

e638859

added async cache, and two implementations of it. FULL

63ca3b5

empty initialization of async implementations

71c3851

style fixes

51f1fce

took out unnecessary code for review

17859ac

sphill99 requested review from jbiral, jpd236 and sophiahaim June 15, 2020 23:01

sphill99 changed the title ~~Get Implementation~~ Get Implementation in AsyncCache that takes in a callback function in two different ways. Jun 16, 2020

jbiral requested changes Jun 16, 2020

View reviewed changes

sophiahaim reviewed Jun 16, 2020

View reviewed changes

Scott Phillips added 3 commits June 16, 2020 15:18

fixed most issues in pull request. Added comments and correct annotat…

396e6be

…ions. Still need to add javadocs

added javadocs to AsyncCache and its implementations

771780b

refactoring. added shared methods to DiskBasedCacheUtility.

28a4a22

sophiahaim reviewed Jun 18, 2020

View reviewed changes

jpd236 reviewed Jun 18, 2020

View reviewed changes

Scott Phillips added 2 commits June 18, 2020 15:19

refactored fallback to not repeat code. Also fixes small errors.

d29af11

added check for if file size changes in process

890d0f1

jbiral reviewed Jun 18, 2020

View reviewed changes

jbiral requested changes Jun 18, 2020

View reviewed changes

jpd236 reviewed Jun 18, 2020

View reviewed changes

closed channel, added logging

6838149

joebowbeer suggested changes Jun 19, 2020

View reviewed changes

deleted fallback cache, closed channel properly

7dcfec9

return when fails

88c66be

jpd236 reviewed Jun 19, 2020

View reviewed changes

joebowbeer suggested changes Jun 20, 2020

View reviewed changes

Scott Phillips added 2 commits June 22, 2020 11:02

added try-with-resouces since requires higher API. Fixed logging erro…

7253e55

…rs and other small changes

AsyncCache doesn't implement Cache anymore.

b8d30ef

jpd236 reviewed Jun 23, 2020

View reviewed changes

joebowbeer reviewed Jun 23, 2020

View reviewed changes

Scott Phillips added 2 commits June 23, 2020 11:10

added javadocs and other small PR fixes

b10b298

shared code between caches are now in one place. tests have been refa…

7d412c7

…ctored to reflect this change.

jbiral requested changes Jun 23, 2020

View reviewed changes

joebowbeer reviewed Jun 23, 2020

View reviewed changes

sphill99 changed the title ~~Get Implementation in AsyncCache that takes in a callback function in two different ways.~~ Get Implementation in AsyncCache that takes in a callback function Jun 23, 2020

fixed javadocs and small refactoring

12393fe

jpd236 reviewed Jun 23, 2020

View reviewed changes

Made DiskBasedCacheUtilityTest class and moved some methods back to w…

1f2fafc

…here they were.

sophiahaim reviewed Jun 24, 2020

View reviewed changes

actually added the test class and changed javadoc.

02487b1

sophiahaim approved these changes Jun 24, 2020

View reviewed changes

jpd236 approved these changes Jun 24, 2020

View reviewed changes

jbiral approved these changes Jun 24, 2020

View reviewed changes

jpd236 merged commit a791d09 into google:master Jun 25, 2020

sphill99 deleted the async2 branch July 17, 2020 19:19


		public abstract void get(String key, OnGetCompleteCallback callback);

		// TODO: Handle callback never being invoked? Match behavior when sync cache throws

Get Implementation in AsyncCache that takes in a callback function #344

Get Implementation in AsyncCache that takes in a callback function #344

Conversation

sphill99 commented Jun 15, 2020

jbiral commented Jun 16, 2020

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

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

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

joebowbeer commented Jun 16, 2020 • edited Loading

sphill99 commented Jun 16, 2020

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

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

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 Jun 20, 2020 • edited Loading

Choose a reason for hiding this comment

sphill99 commented Jun 18, 2020

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

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

joebowbeer commented Jun 16, 2020 •

edited

Loading

joebowbeer Jun 20, 2020 •

edited

Loading