Explore the possibility of a new, async network stack #181
Comments
|
I'd be interested in helping out here. |
|
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. |
|
Very, very rough proof-of-concept here: jpd236@18acc6f 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. |
|
Things that aren't completed yet for this:
|
|
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). |
The current implementation is not thread-safe, and making it thread-safe while retaining an Asynchronous interface and allowing parallel reads would introduce significant complexity. It is also limited to Android O+ which limits its value at the moment. For now, we can rely on either: - AsyncRequestQueue's fallback support for regular Cache, which uses the blocking executor to perform reads. This is functional, but does make heavier use of the blocking executor than is ideal, and also does not currently permit parallel reads, although this could be added later as an improvement to DiskBasedCache. - Cronet's own HTTP cache (although at the moment, we call disableCache(); we can make this configurable). See google#181
The current implementation is not thread-safe, and making it thread-safe while retaining an Asynchronous interface and allowing parallel reads would introduce significant complexity. It is also limited to Android O+ which limits its value at the moment. For now, we can rely on either: - AsyncRequestQueue's fallback support for regular Cache, which uses the blocking executor to perform reads. This is functional, but does make heavier use of the blocking executor than is ideal, and also does not currently permit parallel reads, although this could be added later as an improvement to DiskBasedCache. - Cronet's own HTTP cache (although at the moment, we call disableCache(); we can make this configurable). See #181
|
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. |
|
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. |
|
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. |
- Add warning to new APIs noting that they are experimental and subject to change - Mark NetworkUtility as package-private since it should not be used externally (all methods were already package-private) This unblocks a 1.2.0 release while permitting us to refine the API further before we lock it down in a similar way to other Volley APIs. See google#181
- Add warning to new APIs noting that they are experimental and subject to change - Mark NetworkUtility as package-private since it should not be used externally (all methods were already package-private) This unblocks a 1.2.0 release while permitting us to refine the API further before we lock it down in a similar way to other Volley APIs. See #181
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.
The text was updated successfully, but these errors were encountered: