Skip to content

Don't recover if a non-GET request fails #1043

Closed
@ValleZ

Description

@ValleZ

Currently it can silently retry requests, but now it's forbidden:

http://tools.ietf.org/html/rfc7230

A user agent MUST NOT automatically retry a request with a non-
idempotent method unless it has some means to know that the request
semantics are actually idempotent, regardless of the method, or some
means to detect that the original request was never applied.

related to #258

Activity

ValleZ

ValleZ commented on Sep 4, 2014

@ValleZ
Author

When connection timeout set to 6 seconds OkHttp retries requests (even POST requests) in 6 seconds and throws InterruptedIOException in 12 seconds.

ValleZ

ValleZ commented on Sep 5, 2014

@ValleZ
Author

Seems in my case it retries because it tries TLSv1 first and then SSLv3...

swankjesse

swankjesse commented on Sep 5, 2014

@swankjesse
Collaborator

What does the application layer do when it encounters a flaky network? Does it just retry anyway?

ValleZ

ValleZ commented on Sep 5, 2014

@ValleZ
Author

No, it doesn't. Middle layer is Volley with explicit zero numbers of retries set for each request. Top application layer doesn't do retries as well without user's consent. I'm still investigating issue - for now I can only stable reproduce the retry in a "controlled environment" with mocked server, where I see fall backs to SSLv3 for second call invocation. The behavior makes sense from one point of view - it's reasonable to try another transport if first one fails, but not in case when the server is actually busy processing first request and the call changes a state on backend (i.e. it's not a GET request).

swankjesse

swankjesse commented on Sep 5, 2014

@swankjesse
Collaborator

Right now HttpEngine.recover() is quite aggressive about recovering from problems. We assume everyone is using idempotence in their application layer; that's the best way to mitigate connectivity problems.

It's possible some applications are using queries to see if a broken attempt was completed and not retrying if it was. That's clumsy.

ValleZ

ValleZ commented on Sep 5, 2014

@ValleZ
Author

The thing is that apache http implementation is much better for serious applications because it's quite hard task to implement the idempotence for case when you, for example, purchase something. In same time ALL applications expect that network request may fail, but there are very few apps which expect that request will be sent more than once. It's probably okay for regular downloads, but is is inacceptable behavior for requests which change state. Think about chat applications - how fun is to send 2 messages instead one only because you switched from apache to okhttp? How fun is to spend money twice? How great is to get response "sorry, you already did it" instead expected response? Is is much bigger problem then to break some garbage which doesn't expect that network requests fails eventually.

ValleZ

ValleZ commented on Sep 5, 2014

@ValleZ
Author

one more time, http://tools.ietf.org/html/rfc7230#section-6.3.1:
"A user agent MUST NOT automatically retry a request with a non-
idempotent method unless it has some means to know that the request
semantics are actually idempotent
, regardless of the method, or some
means to detect that the original request was never applied."
okhttp don't know if a request is actually idempotent. Only app layer knows that, that's why say Volley have ability to retry calls. OkHttp have no idea if request is idempotent in therefore must not retry it silently.

ValleZ

ValleZ commented on Sep 6, 2014

@ValleZ
Author

my quick fix is https://github.com/ValleZ/okhttp/commit/7b108596e531fc367ebd7840228939b3a862f6ed
For better backward comparability there probably should be a switch which turns the retries off for a request, but seems I cannot quickly implement it with grateful switching route for next requests.

ValleZ

ValleZ commented on Sep 10, 2014

@ValleZ
Author

The fix above is bad because it breaks recovery of closed connections :-(

ValleZ

ValleZ commented on Sep 10, 2014

@ValleZ
Author

This should work better but it doesn't guarantee that a request will not be sent twice
https://github.com/ValleZ/okhttp/commit/a58b2f27eda3862a75d3f5adeb89f4565e542e32

briancarlstrom

briancarlstrom commented on Sep 22, 2014

@briancarlstrom

okhttp implements the silent fallback from TLS to SSL v3.0 similar to Chrome as part of supporting TLS intolerance: https://www.imperialviolet.org/2011/02/04/oppractices.html However, it probably is a good thing to let clients talking to known TLS tolerant servers disable this behavior.

swankjesse

swankjesse commented on Sep 28, 2014

@swankjesse
Collaborator

I think we want to write tests to require that non-GET requests are never retried (possibly breaking compatibility with HttpURLConnection). And GET requests will be retried, so that users don't see broken images etc. when connection reuse is in play.

changed the title [-]OkHttp must not silently retry requests[/-] [+]Don't recover if a non-GET request fails[/+] on Sep 28, 2014
swankjesse

swankjesse commented on Sep 28, 2014

@swankjesse
Collaborator

(Fixing this may also permit us to nuke RetryableSink and all of its related complexity)

10 remaining items

removed this from the 2.1 milestone on Nov 5, 2014
yogurtearl

yogurtearl commented on Nov 12, 2014

@yogurtearl

Any change of getting this into 2.1? :)

In general, it would be nice to have a configurable retry policy. Something like:

interface RetryPolicy {
     RetryMode retryModeFor( Request request );
}

enum RetryMode {
      NO_RETRY, RETRY_ON_CONNECT_FAIL, RETRY_ON_ZERO_BYTES_IN_REPONSE 
}
swankjesse

swankjesse commented on Nov 12, 2014

@swankjesse
Collaborator

You've missed the boat on 2.1. We'll conquer this very soon.

Qubitium

Qubitium commented on Nov 27, 2014

@Qubitium

+1 this. Would love a global RetryPolicy setting for both GET/POST/ANY...We are in the same boat using Okhttp behind Volley and despite us disabling retries in volley, Okhttp is still retrying requests that we don't want retried.

yogurtearl

yogurtearl commented on Dec 1, 2014

@yogurtearl

2.2? :)

passsy

passsy commented on Dec 3, 2014

@passsy

+1
I really like the RetryPolicy idea from volley. But it should be possible to get the maximum request duration for every request.

added a commit that references this issue on Dec 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugBug in existing code

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      Participants

      @codefromthecrypt@swankjesse@fgutmann@Qubitium@ValleZ

      Issue actions

        Don't recover if a non-GET request fails · Issue #1043 · square/okhttp