Okhttp: Default behaviour of retryOnConnectionFailure is dangerous for POST requests

[ktchernov]

Default behaviour of OkHttpClient.Builder to set retryOnConnectionFailure may result in POST requests being silently resent on network timeouts.

While this may be typically alright for GET requests, POST/PATCH/DELETE requests modify data, and re-submitting them without knowing whether they have been received at the other end.

[ktchernov]

This is also something that is a significant change in OkHttp v3 over v2. It would be good to include this in CHANGES.md

[swankjesse]

This has been our behavior forever.

[ktchernov]

I have investigated this further, and there is a change in behaviour between v2 and v3 here, which is in the way SocketConnectionTimeout is handled. In v2 the request was always dropped, in v3 the connection is retried unless retryOnConnectionFailure is set.

Compare https://github.com/square/okhttp/blob/parent-3.0.0-RC1/okhttp/src/main/java/okhttp3/internal/http/StreamAllocation.java line 336 to https://github.com/square/okhttp/blob/parent-2.7.5/okhttp/src/main/java/com/squareup/okhttp/internal/http/StreamAllocation.java line 358

This means that if the client sent a POST request, then the server received it but the response got lost due to network issues, the client will resend it again. While in v2 it would fail.

[ktchernov]

This is the change - https://github.com/square/okhttp/commit/3b44fa37675c1d68298283ae4b8b45cd5771888d and https://github.com/square/okhttp/issues/1736

[dave-r12]

@ktchernov could you write a test case? There are great examples in CallTest, https://github.com/square/okhttp/blob/master/okhttp-tests/src/test/java/okhttp3/CallTest.java

[renaudcerrato]

Along with https://github.com/square/okhttp/issues/2443, that's exactly what caused my development backend to explode: my multipart upload tooks a few hundred of milliseconds more than my read timeout, causing Retrofit to silently retry over and over because of the SocketTimeoutException (which is still a bug BTW) - thus filling my storage with junks since the request did succeed....

[JakeWharton]

Retrofit doesn't retry.

On Sat, Mar 26, 2016, 3:40 PM Cerrato Renaud [email protected]
wrote:

Along with #2443 https://github.com/square/okhttp/issues/2443, that's
exactly what caused my development backend to explode: the multipart upload
tooks slightly more time than my read timeout, causing Retrofit to silently
retry over and over...

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
https://github.com/square/okhttp/issues/2394#issuecomment-201918305

[renaudcerrato]

Retrofit doesn't retry.

I meaned the underlying OkHttp3, used by Retrofit 2.

[ktchernov]

I have created two unit tests, a passing test on OkHttp 2.7 - demonstrating that the behaviour there was as expected: https://gist.github.com/ktchernov/2a1b6ad8a0b0f12634a55b9e2551395e - when a timeout occurs on a POST request body, OkHttp 2 does not try to re-submit the POST.

Second is the same test adapted to OkHttp 3.2 https://gist.github.com/ktchernov/5785e6d6664a9f306c01632b95492fdd - this test fails. OkHttp 3.2 does re-submit the POST body - which could result in unexpected duplicate submissions.

[swankjesse]

Thanks for the test. I’ll take a look!

[ktchernov]

Sorry there was a mistake in those tests, which I've corrected now.

[swankjesse]

And this is the bug that we fixed when we changed this behavior.
https://github.com/square/okhttp/issues/1736

[swankjesse]

I think the nuanced fix we want is to recover from timeouts that occur during connect, but not during read.