Reduce flakiness and document reasons for flakiness #1348
Conversation
| @@ -28,6 +28,11 @@ | |||
| /** | |||
| * Close the socket after the response. This is the default HTTP/1.0 | |||
| * behavior. | |||
| * | |||
| * <p>Note: Be careful if this used in a test and it is not the final queued request. The client | |||
There was a problem hiding this comment.
For what it's worth, triggering this flakiness is the entire motivation here.
There was a problem hiding this comment.
The tests appear to be testing "what happens if a pooled connection has gone bad?", not "what happens if the server closes the connection at an arbitrary point during the request / response".
You agree it's non-deterministic? Just to be clear in case my comments didn't explain:
I've seen this policy used in two ways:
- Advertise a body as being N bytes, but disconnect before delivering the full N bytes.
- Advertise a body as being N bytes, deliver the full N bytes, then (after time T) disconnect.
(1) is fine, since the client is blocked waiting for N bytes.
(2) is problematic if the test is trying to test "what happens if a pooled connection is no longer valid" : the client receives the N bytes, then the result depends on how long T is. Usually the test is expecting the connection to be closed by the time a second request is issued. However, because time T is varied, the test can move on to make another request and have the connection closed in the middle or after the client has issued the request.
If the server closes the connection before request 2 has been made the mock server needs to be set up to expect 2 requests. If it closes it after request 2 has been made and OkHttp retries, then it needs to be set up to expect 3 requests.
|
You fixed the tests, but I think the right solution is fixing OkHttp itself. |
Android has been receiving reports of some tests being flaky on what are probably lower-spec devices. This introduces delays into tests where sockets are being poisoned after the entire response body has been written to them *and* where there are follow-up requests. This change also improves the documentation for the problematic SocketPolicy values.
nfuller
force-pushed
the
FixFlakesOnDisconnect
branch
from
90ae2e5
to
432ca1e
Compare
|
Updated the documentation. It's possible there are also problems with OkHttp not retrying these when it should but I haven't investigated too much beyond working out a reason for why they were flaky. The problems manifest as problems when reading the response: 01-09 11:23:44 I/VGLV8S7999999999: com.squareup.okhttp.internal.http.URLConnectionTest#postFailsWithChunkedRequestForLargeRequest FAIL |
|
I still fear that we're fixing the tests when we should be fixing the implementation. The motivation for these wacky socket policies is to put the client in an awkward state, and to confirm that the client isn't flaky even when the server is flaky. |
|
Rebased & merged. |
|
Epic delay I know. |
Android has been receiving reports of some tests being flaky
on what are probably lower-spec devices.
This introduces delays into tests where sockets are being
poisoned after the entire response body has been written to
them and where there are follow-up requests.
This change also improves the documentation for the problematic
SocketPolicy values.