Validate stale connection to fix the bug: failed to respond #760
Conversation
|
Thanks @ryan-tu - excellent work and great summary! The default FYI, I rewrote JDBC driver in PR #747. Apache http client is now replaced by HttpURLConnection(similar as clickhouse4j), and it works just fine. JDK9+ HttpClient is apparently a better choice, but my point is that we probably don't need a 3rd party library like Apache http client and OKhttp anymore. |
|
UT test cases are added. It takes some time to reproduce the bug and prove the bug has been fixed.
I'm no expert of HTTP, but i am aware that Apache HttpClient provides a rich advanced feature set, like proxy, pool configuration, async API, HTTP2 etc.. I'm not sure if HttpURLConnection can fulfill all the requirements. |
|
Thanks again @ryan-tu for spending extra time adding the tests. We probably need Did you notice public PoolingHttpClientConnectionManager(
final HttpClientConnectionOperator httpClientConnectionOperator,
final HttpConnectionFactory<HttpRoute, ManagedHttpClientConnection> connFactory,
final long timeToLive, final TimeUnit timeUnit) {
super();
this.configData = new ConfigData();
this.pool = new CPool(new InternalConnectionFactory(
this.configData, connFactory), 2, 20, timeToLive, timeUnit);
this.pool.setValidateAfterInactivity(2000);
this.connectionOperator = Args.notNull(httpClientConnectionOperator, "HttpClientConnectionOperator");
this.isShutDown = new AtomicBoolean(false);
}Regarding http client, although HttpURLConnection does not support advanced feature like http trailer, it supports basic ones including chunking, making it suitable for a simple JDBC driver. Apache http client on the other hand is too heavy and it contains many features that we don't need. Having said that, because we now have JDBC driver and protocol implementation separated in different modules(e.g. |
|
Problem Statement:
A lot of work has been done to fix the "failed to respond" problem.
#290, #462, #531 report the problem,
#515 use the server side Keep-Alive timeout to replace the client side parameter, to reduce the possibility of "stale connection",
#505 fix the wrong connection reuse strategy, wrong Keep-Alive duration to make sure connection is closed immediately when a request fails,
#540 Add retry logic for idempotent SQL such as SELECT, and use a client side parser to decide whether an sql is idempotent or not. But, for non-idempotent SQL, such as INSERT, this problem still exists. And parsing each SQL in JDBC is a heavy cost, and it's hard to keep consistent with server side SQL syntax.
Proposal:
The root cause to "failed to respond" is half-closed tcp connections(closed by server-side when idle timeout reaches) are still kept in underlying connection pool of HttpClient.
The problem is a common issue to all connection pools like Druid, C3P0, DBCP, HikariCP, and each connection pool MUST provide a validation mechanism to evict stale connection.
The validation mechanism provided by Apache HttpClient before v4.4 requires check per request, thus is too heavy to use.
Since v4.4, a new validation mechanism is imported by:
PoolingHttpClientConnectionManager.setValidateAfterInactivity(), which is a middle ground between check per request and no check.This PR add a new connection setting
validateAfterInactivityMillisto enable it.The best solution should be that the client honors server-side Keep-Alive timeout to do best effort to reuse connections, and set
validateAfterInactivityMillisto a slightly shorter value than server-side Keep-Alive timeout to evict stale ones.