Read the response even if writing the request fails #6295
Conversation
| @@ -20,6 +20,7 @@ import java.net.ProtocolException | |||
| import okhttp3.Interceptor | |||
| import okhttp3.Response | |||
| import okhttp3.internal.EMPTY_RESPONSE | |||
| import okhttp3.internal.http2.ConnectionShutdownException | |||
| import okio.buffer | |||
There was a problem hiding this comment.
the diffs on this file are worth viewing in 'ignore whitespace' mode because there’s a lot of indentation change
| return response | ||
| } catch (e: IOException) { | ||
| if (sendRequestException != null) { | ||
| sendRequestException.addSuppressed(e) |
| .build()) | ||
|
|
||
| call.execute().use { response -> | ||
| assertThat(response.body!!.string()).isEqualTo("abc") |
| @@ -1118,7 +1118,7 @@ private void writeChunk(BufferedSink sink) throws IOException { | |||
| assertThat(listener.recordedEventTypes()).containsExactly( | |||
| "CallStart", "ProxySelectStart", "ProxySelectEnd", "DnsStart", "DnsEnd", "ConnectStart", | |||
| "ConnectEnd", "ConnectionAcquired", "RequestHeadersStart", "RequestHeadersEnd", | |||
| "RequestBodyStart", "RequestFailed", "ConnectionReleased", "CallFailed"); | |||
| "RequestBodyStart", "RequestFailed", "ResponseFailed", "ConnectionReleased", "CallFailed"); | |||
There was a problem hiding this comment.
Should we have a test for this where it does successfully read the response? That is, even though server didn't read the request?
There was a problem hiding this comment.
Yep, the new test case includes that.
| expectedEvents += "RequestHeadersStart" | ||
| expectedEvents += "RequestHeadersEnd" | ||
| expectedEvents += "RequestBodyStart" | ||
| expectedEvents += "RequestFailed" |
There was a problem hiding this comment.
maybe a trailing comment here highlighting that the important part is we read the response headers and body even on error.
swankjesse
force-pushed
the
jwilson.1001.read_response_after_fail
branch
from
d1ee62d
to
5687532
Compare
|
I've just verified this fix E2E in a project that uses okhttp. Thank you, @swankjesse |
|
We've been waiting for this for quite a long time, Is there any chance of a 4.x release that includes this fix? |
|
@ricellis 5.0.0-alpha.3 is out as of yesterday and it has it. I don't think we'll backport to 4.x because it's a riskier change. (We're tracking one regression related to internet disconnects.) |
Ah that's a shame because we missed 3.x to 4.x for the same reason once before, guess this fix has unlucky timing! Thank you for the update. |
Closes: 1001 (cherry picked from commit 9533117)
Closes: 1001 (cherry picked from commit 9533117) Co-authored-by: Jesse Wilson <[email protected]>
Closes: #1001