Volley doesn't ensure that the "timeout" has elapsed before retrying

[CapnSpellcheck]

The property "getCurrentTimeout" in RetryPolicy is used only to set a timeout on the HTTP connection. If the server returns an error code rapidly, the request engine will retry immediately.

There is no enforcement that the value provided by "getCurrentTimeout" has elapsed, which caught me quite offguard. This will, for example, cause me to drain battery if I set a high maxRetries with a high timeout, thinking that the interval between requests would respect the timeout / backoff multiplier.

[jpd236]

Yeah, I'm not quite sure why the timeout was implemented this way - only applying to how long we give the server to respond rather than what to do if the server actually responds. I can only speculate that the focus at the time was more on dealing with poor connectivity rather than servers that transiently return errors. But we can't safely change default behavior here since apps may be relying on it.

I think apps could possibly accomplish this by having their RetryPolicy block for any remaining time until the next timeout if that behavior is desired. (It's not particularly efficient, but Volley already blocks on network requests rather than handle them asynchronously, so worst-case behavior is unchanged).

On the Volley side, a potential option is to provide an opt-in boolean in Request to flag whether BasicNetwork should ensure it waits at least this long between tries if the request itself doesn't take that long. I still think we'd end up blocking the network dispatcher thread though as anything asynchronous here would be a pretty big deviation from what we've had in the past.

I'll leave this open to think about it, but given the need to opt-in on a per-app basis anyway, I'm currently slightly leaning towards the position that this would be up to the RetryPolicy implementation to handle (and improving the Javadoc to note this behavior).

[CapnSpellcheck]

It would be great to have an example implementation, if you do choose that route. Plus, such a RetryPolicy could be bundled with Volley for convenience's sake.

Are you suggesting that something like 'sleep' be used in RetryPolicy.retry? Seems reasonable, although the RetryPolicy would have to be aware of when the connection started. Looks like getCurrentTimeout is called right before connecting, so that could be abused to note the start time if absolutely necessary.

[vidia]

I was looking for this functionality as well. I am working against an API with a rather aggressive Rate limit (about 1 request per second max) and was trying to use Volley's RetryPolicy to respect the Retry-After header. But I was running into the same issue as above where setting the Timeout was not causing a delay before making the next request like I expected.

It would be nice to see updated docs on this at the very least.

I will attempt the sleep() to see if it will work in the meantime.

[CapnSpellcheck]

Might as well paste my code for this :)

class BetterRetryPolicy : DefaultRetryPolicy {
    constructor() : super()
    constructor(initialTimeoutMs: Int, maxNumRetries: Int, backoffMultiplier: Float) :
        super(initialTimeoutMs, maxNumRetries, backoffMultiplier)

    var requestStartTime: Long = 0

    override fun getCurrentTimeout(): Int {
        requestStartTime = System.nanoTime()
        return super.getCurrentTimeout()
    }

    override fun retry(error: VolleyError?) {
        val now = System.nanoTime()
        super.retry(error)
        val elapsedMs = TimeUnit.NANOSECONDS.toMillis(now - requestStartTime)
        if (elapsedMs < super.getCurrentTimeout()) {
            try {
                Thread.sleep(super.getCurrentTimeout() - elapsedMs)
            } catch (ex: InterruptedException) {}
        }
    }
}```

[CapnSpellcheck]

Comment on that code posted a few days ago. It definitely isn't perfect, because it blocks the Volley processing thread that it runs on, while it's waiting for the timeout. If we were to see an implementation of this in the library, it really should use asynchronous scheduling to get this job done.

[jpd236]

Yep - as noted above: "It's not particularly efficient, but Volley already blocks on network requests rather than handle them asynchronously, so worst-case behavior is unchanged"

While Volley is good about exposing an async interface to clients, it's not the most efficient about actually doing things asynchronously under the covers, thanks to the network/cache dispatcher threads. The Network interface must either return the final response after all retries or throw an error, and this interface couldn't be changed without breaking Volley's public API or building an alternative entire asynchronous stack. I filed #181 to capture some thoughts there, but at the moment there are no plans to pursue this.

[jpd236]

BTW - if the delays here are ~seconds long or longer, it would probably be more appropriate to use a higher-level abstraction to perform any retries, like JobScheduler or Firebase JobDispatcher.

[jpd236]

OK. Thinking about it a little more, I'm hesitant to encourage the direction of blocking in retry() in the long term, as it would tie us more tightly to this synchronous network model. So I don't think I'd want to suggest that in the documentation, but as noted above, you can still do this in the mean time, and it will work as long as you're comfortable with one of the (limited) NetworkDispatcher threads being stalled during the sleep.

For 1.1.1 I'll thus update the documentation to explain the behavior so it's understood, without suggesting a block in retry().

I haven't fully thought this out, but we might be able to swing this in the current architecture by re-adding the failed request to the queue after the desired delay. We could use a main thread handler to post a delayed message. Since the retry policy is an object attached to the request, and there's no other context outside of the request in the current retry loop, this should roughly accomplish what we're after, although the request may end up being delayed more than the requested amount of time if other requests come in and fill the queue in the interim. An API change would still be required to specify the delay between retries, so that's not until at least 1.2.0.