Give up when ExchangeFinder exhausts all routes. #5888
Conversation
|
There is a (extremely rare?) scenario where we may make a connection to a server that is then flagged as unhealthy. Currently 4.4.0 and master will throw a I ran this test against 4.3.1 and we appear to go an infinite loop. I'm not sure the right action to take in this scenario. Let me know your thoughts. |
|
Not sure about the right fix. Seems like worth fixing in 4.5, probably commit this behind @ignore first? |
|
Great test |
|
This PR is the hit sequel to
|
dave-r12
force-pushed
the
unhealthy-conn
branch
from
541dc65 to
7735111
Compare
dave-r12
changed the title
|
|
||
| // Make sure we have some routes left to try. One example where we may exhaust all the routes | ||
| // would happen if we made a new connection and it immediately is detected as unhealthy. | ||
| synchronized(connectionPool) { |
There was a problem hiding this comment.
Should we put a
connectionPool.assertThreadDoesntHoldLock()
At the top of this method? I find it really really useful to document the current expected state of locks whenever we then open into a lock.
| // Make sure we have some routes left to try. One example where we may exhaust all the routes | ||
| // would happen if we made a new connection and it immediately is detected as unhealthy. | ||
| synchronized(connectionPool) { | ||
| val routesLeft = routeSelection?.hasNext() ?: true |
There was a problem hiding this comment.
It seems like line 108 must have made routeSelection non null, so can we change to !!
Scratch that, I'm less sure of that.
n.b. it passes tests when both are !!, so we are either
a) safe to make 127 and 128 !!
b) missing test coverage
There was a problem hiding this comment.
These will be null if the call had a previous connection we could use. Here's a test that demonstrates it:
server.enqueue(new MockResponse().setResponseCode(401));
server.enqueue(new MockResponse().setResponseCode(200));
AtomicReference<Connection> ref = new AtomicReference<>();
EventListener listener = new EventListener() {
@Override public void connectionAcquired(Call call, Connection connection) {
ref.set(connection);
}
};
client = client.newBuilder()
.eventListener(listener)
.authenticator((route, response) -> {
Util.closeQuietly(ref.get().socket());
return response.request();
})
.build();
executeSynchronously("/").assertCode(200);
| val routesLeft = routeSelection?.hasNext() ?: true | ||
| val routesSelectionLeft = routeSelector?.hasNext() ?: true | ||
| if (nextRouteToTry == null && !routesLeft && !routesSelectionLeft) { | ||
| throw IOException("exhausted all routes") |
There was a problem hiding this comment.
We can probably be more expressive in this error, it's an interesting case when routes are immediately unhealthy. Can we log the success and error count maybe?
|
Nice PR! |
| synchronized(connectionPool) { | ||
| val routesLeft = routeSelection?.hasNext() ?: true | ||
| val routesSelectionLeft = routeSelector?.hasNext() ?: true | ||
| if (nextRouteToTry == null && !routesLeft && !routesSelectionLeft) { |
There was a problem hiding this comment.
I’d be tempted to change this to short circuit more aggressively?
// Make sure we have some routes left to try. One example where we may exhaust all the routes
// would happen if we made a new connection and it immediately is detected as unhealthy.
synchronized(connectionPool) {
if (nextRouteToTry != null) continue
val routesLeft = routeSelection?.hasNext() ?: true
if (routesLeft) continue
val routesSelectionLeft = routeSelector?.hasNext() ?: true
if (routesSelectionLeft) continue
throw IOException("exhausted all routes")
}
|
I’m gonna merge this as-is because it fixes a big bug, and is already great. Please address feedback in follow-up PRs as necessary! |
|
Hi @swankjesse & others, We are also facing this issue with okhttp3 3.12.8 version, Could you please confirm if this issue fixed in any latest okhttp3 version? okhttp3- 3.12.8 Logs:- Thanks in advance. |
#3308