Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The DefaultRetryPolicy method retry() doesn't do what it is intended to do. #26

Closed
Ericliu001 opened this issue Apr 28, 2017 · 2 comments

Comments

@Ericliu001
Copy link
Contributor

Ericliu001 commented Apr 28, 2017

Hi, I was going to through the code on the retry mechanism today and came across the RetryPolicy. The following code is from the class DefaultRetryPolicy.

/**
 * Prepares for the next retry by applying a backoff to the timeout.
 * @param error The error code of the last attempt.
 */
@Override
public void retry(VolleyError error) throws VolleyError {
    mCurrentRetryCount++;
    mCurrentTimeoutMs += (mCurrentTimeoutMs * mBackoffMultiplier);
    if (!hasAttemptRemaining()) {
        throw error;
    }
}

Under the circumstance where the NetworkDispatcher wants to retry a request, it calls retry() on the RetryPolicy instance of that request. I would assume that the same request should be added to the RequestQueue as an retry attempt.

But the retry() method actually doesn't do much other than updating a few values, which doesn't schedule another call to fire the failed request again.

If we look at the test case in BasicNetworkTest, it seems that we should assume that a retry request has been made as long as the retry() method of the RetryPolicy instance is called. But the currently implementation of DefaultRetryPolicy doesn't really do that.

@Test public void serverError_enableRetries() throws Exception {
    for (int i = 500; i <= 599; i++) {
        MockHttpStack mockHttpStack = new MockHttpStack();
        BasicHttpResponse fakeResponse =
                new BasicHttpResponse(new ProtocolVersion("HTTP", 1, 1), i, "");
        mockHttpStack.setResponseToReturn(fakeResponse);
        BasicNetwork httpNetwork =
                new BasicNetwork(mockHttpStack, new ByteArrayPool(4096));
        Request<String> request = buildRequest();
        request.setRetryPolicy(mMockRetryPolicy);
        request.setShouldRetryServerErrors(true);
        doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
        try {
            httpNetwork.performRequest(request);
        } catch (VolleyError e) {
            // expected
        }
        // should retry all 500 errors
        verify(mMockRetryPolicy).retry(any(ServerError.class));
        reset(mMockRetryPolicy);
    }
} 
@xesam
Copy link

xesam commented Apr 28, 2017

DefaultRetryPolicy#retry() will not add a new(or the same) request to the current RequestQueue .this method just swallowed the 'VolleyError', so BasicNetwork#performRequest(request) can continue the 'while(true)' loop

@Ericliu001
Copy link
Contributor Author

Thank you for explaining. @xesam

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants