google / volley

https://google.github.io/volley
Apache License 2.0
3.38k stars 754 forks source link

Explore the possibility of a new, async network stack #181

Open jpd236 opened 6 years ago

jpd236 commented 6 years ago

While Volley's client interface is asynchronous, the internals it uses to perform requests are not - there are fixed-size thread pools for cache and network interactions which get blocked for the duration of any requests. This is inherent to the public APIs that Volley currently exposes.

If we wanted to resolve this and support asynchronous HTTP client stacks, we would either need to accept a breaking API change to Volley, or else explore building an alternative network stack API structure that would coexist with the existing synchronous stack. We'd need AsyncRequestQueue/AsyncNetwork/AsyncCache at a minimum.

Notably, there is no method for sending asynchronous HTTP requests built into the Android SDK. (Disk caching might be doable with the nio package). So we wouldn't actually be able to provide an out-of-the-box implementation in Volley's core (at least without taking on a new dependency). We could consider starting separate projects e.g. volley-okhttp or volley-cronet which provide implementations of async queues.

Filing this for tracking ideas, but at the moment this is not on the roadmap.

jjoslin commented 6 years ago

I'd be interested in helping out here.

jpd236 commented 6 years ago

One random piece: per #80, the way our RetryPolicy interface is intended to work is by increasing request timeouts on each subsequent attempt; however, it doesn't introduce any time delay between request attempts. Currently, it's "safe" to throw a sleep() in the retry method to effectively introduce this gap, since it's the NetworkDispatcher thread which is calling it, and that thread is already blocking on network traffic. But in a world with Async dispatchers, we'd want another way to trigger a retry after a delay.

This may involve a similar change as we did for HttpStack -> BaseHttpStack, since RetryPolicy is an interface - deprecate RetryPolicy, introduce a new class which implements it and provide a new default method to return the delay in between requests which defaults to 0.

jpd236 commented 6 years ago

Very, very rough proof-of-concept here: https://github.com/jpd236/volley/commit/18acc6f3fc1ac85caad6688728cdc366cb9d82ce

There are certainly some details still to be worked out, but I'm reasonably confident that there is a path forward here which would avoid breaking existing clients while still allowing development of a new async layer. The next step here will be to write a design doc.

sphill99 commented 4 years ago

Things that aren't completed yet for this:

jpd236 commented 4 years ago

I'm currently leaning towards reverting the DiskBasedCache changes and punting the asynchronous cache implementation to a later date. Trying to achieve thread safety while allowing parallel reads/writes seems very challenging to do correctly, even with something like ReentrantReadWriteLock, given the combination of global in-memory state with per-entry on-disk state and the need to support an asynchronous API surface. The value is also somewhat diminished given that these NIO APIs require API 26+, which currently excludes about 40% of devices, and maintaining two code paths here is a maintenance burden that will be hard to test well. And if you're using Cronet, there's already an HTTP cache which can optionally be configured/used there, so Volley's cache layer is probably not adding much value at all.

We can complete the rest of the stack and possibly retain support for the AsyncCache interface without too much trouble; we just won't provide an implementation out of the box from day 1, and will use the fallback path instead. Longer term, we can see if we can improve the parallelization of DiskBasedCache by using ReentrantReadWriteLock, and/or provide an implementation of AsyncCache on top of another I/O platform (perhaps LevelDB, which has at least some degree of asynchronicity).

jpd236 commented 4 years ago

This can now be considered feature complete; the async stack is functional and can be integrated into applications. I've documented how to use it and the limitations at:

https://github.com/google/volley/wiki/Asynchronous-Volley

It is likely that we will continue to resolve bugs, expand the feature set slightly around timeouts and caches, tweak the API surface, and/or validate the stack in real applications before we consider it production-ready.

jpd236 commented 4 years ago

One specific question for code cleanup - should AsyncHttpStack extend BaseHttpStack? It's impossible to use CronetHttpStack as one today because the executors never get set, and the methods to do so are marked as internal to the library. The value of doing so seems pretty small, e.g. you want to make use of the Cronet based stack or some other HTTP implementation but for whatever reason aren't migrating to AsyncRequestQueue altogether. Since it's possible to migrate and still use DiskBasedCache and the same RequestQueue APIs, it's hard to see this being worth supporting.

jpd236 commented 3 years ago

To unblock the 1.2.0 release - we'll document that these APIs are experimental (linking to https://github.com/google/volley/wiki/Asynchronous-Volley) and subject to breaking changes in future releases.

We'll move further cleanup/extensions to the next release milestone.