From 272d9c48b1d72a7605953b6c94d760f2895afafb Mon Sep 17 00:00:00 2001 From: Zhichun Wu Date: Mon, 25 Oct 2021 20:24:16 +0800 Subject: [PATCH 1/3] Drop for no usage and security reason --- .github/workflows/verify.yml | 158 ----------------------------------- 1 file changed, 158 deletions(-) delete mode 100644 .github/workflows/verify.yml diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml deleted file mode 100644 index 91725e5fb..000000000 --- a/.github/workflows/verify.yml +++ /dev/null @@ -1,158 +0,0 @@ -name: Verify - -on: - workflow_dispatch: - inputs: - clickhouse: - description: "ClickHouse version" - required: true - default: "latest" - java: - description: "Java version" - required: true - default: "8" - chTz: - description: "ClickHouse timezone" - required: true - default: "Asia/Chongqing" - javaTz: - description: "Java timezone" - required: true - default: "America/Los_Angeles" - pr: - description: "Pull request#" - required: false - pull_request: - types: - - opened - - synchronize - - reopened - paths-ignore: - - "**.md" - - "docs/**" - - "**/CHANGELOG" - issue_comment: - types: - - created - - edited - -jobs: - verify-commented-pr: - runs-on: ubuntu-latest - name: Verify commented PR - if: github.event_name == 'issue_comment' && github.event.issue.pull_request - steps: - - uses: zhicwu/pull-request-comment-trigger@master - id: check - with: - trigger: '@verify' - reaction: rocket - env: - GITHUB_TOKEN: '${{ secrets.GITHUB_TOKEN }}' - - name: Extra parameters from PR comment - if: steps.check.outputs.triggered == 'true' - uses: actions/github-script@v3 - id: commented - env: - COMMENT_BODY: '${{ github.event.comment.body }}' - with: - script: | - const keyword = '@verify'; - let buildArgs = process.env.COMMENT_BODY; - core.info(`Got commented body: ${buildArgs}`); - buildArgs = buildArgs.substring(buildArgs.lastIndexOf(keyword) + keyword.length); - const args = buildArgs.match(/[^\s]+/g); - core.info(`Got commented arguments: ${args}`); - - return { - pr: context.issue.number, - clickhouse: args && args.length > 0 ? args[0] : "latest", - java: args && args.length > 1 ? args[1] : "8" - }; - - name: Check out repository - uses: actions/checkout@v2 - - name: Check out commented PR - if: steps.check.outputs.triggered == 'true' - run: | - git fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 \ - origin pull/${{ fromJSON(steps.commented.outputs.result).pr }}/merge:merged-pr && git checkout merged-pr - - name: Set up JDK - if: steps.check.outputs.triggered == 'true' - uses: actions/setup-java@v1 - with: - java-version: ${{ fromJSON(steps.commented.outputs.result).java }} - continue-on-error: true - - name: Verify with Maven - if: steps.check.outputs.triggered == 'true' - run: | - mvn --batch-mode --update-snapshots \ - -DclickhouseVersion=${{ fromJSON(steps.commented.outputs.result).clickhouse }} \ - -DclickhouseTimezone=Asia/Chongqing \ - -Duser.timezone=America/Los_Angeles verify - id: maven - continue-on-error: true - - name: Comment PR - uses: actions/github-script@v3 - if: steps.check.outputs.triggered == 'true' - env: - CLICKHOUSE_VERSION: ${{ fromJSON(steps.commented.outputs.result).clickhouse }} - JAVA_VERSION: ${{ fromJSON(steps.commented.outputs.result).java }} - PREV_STEP_RESULT: '${{ steps.maven.outcome }}' - with: - github-token: ${{ secrets.GITHUB_TOKEN }} - script: | - const { issue: { number: issue_number }, repo: { owner, repo } } = context; - const result = process.env.PREV_STEP_RESULT; - const buildUrl = `https://github.com/${owner}/${repo}/actions/runs/${context.runId}`; - const flag = result === 'success' - ? ':green_circle:' - : (result === 'failure' ? ':red_circle:' : ':yellow_circle:'); - const msg = `${flag} verify using JDK [${process.env.JAVA_VERSION}] and ClickHouse [${process.env.CLICKHOUSE_VERSION}]: [${result}](${buildUrl})`; - github.issues.createComment({ issue_number, owner, repo, body: msg }); - - verify-on-demand: - runs-on: ubuntu-latest - name: Verify on demand - if: github.event_name == 'workflow_dispatch' - steps: - - name: Check out repository - uses: actions/checkout@v2 - - name: Check out PR - run: | - git fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin pull/${{ github.event.inputs.pr }}/merge:merged-pr && git checkout merged-pr - if: github.event.inputs.pr != '' - - name: Set up JDK ${{ github.event.inputs.java }} - uses: actions/setup-java@v1 - with: - java-version: ${{ github.event.inputs.java }} - continue-on-error: true - - name: Verify with Maven - run: | - mvn --batch-mode --update-snapshots \ - -DclickhouseVersion=${{ github.event.inputs.clickhouse }} \ - -DclickhouseTimezone=${{ github.event.inputs.chTz }} \ - -Duser.timezone=${{ github.event.inputs.javaTz }} verify - id: maven - continue-on-error: true - - name: Comment PR - uses: actions/github-script@v3 - if: github.event.inputs.pr != '' - env: - PR_NO: ${{ github.event.inputs.pr }} - CLICKHOUSE_TIMEZONE: ${{ github.event.inputs.chTz }} - CLICKHOUSE_VERSION: ${{ github.event.inputs.clickhouse }} - JAVA_TIMEZONE: ${{ github.event.inputs.javaTz }} - JAVA_VERSION: ${{ github.event.inputs.java }} - PREV_STEP_RESULT: '${{ steps.maven.outcome }}' - with: - github-token: ${{ secrets.GITHUB_TOKEN }} - script: | - const issue_number = process.env.PR_NO; - const { repo: { owner, repo } } = context; - const result = process.env.PREV_STEP_RESULT; - const buildUrl = `https://github.com/${owner}/${repo}/actions/runs/${context.runId}`; - const flag = result === 'success' - ? ':green_circle:' - : (result === 'failure' ? ':red_circle:' : ':yellow_circle:'); - const msg = `${flag} verify using JDK [${process.env.JAVA_VERSION}, timezone=${process.env.JAVA_TIMEZONE}] and ClickHouse [${process.env.CLICKHOUSE_VERSION}, timezone=${process.env.CLICKHOUSE_TIMEZONE}]: [${result}](${buildUrl})`; - github.issues.createComment({ issue_number, owner, repo, body: msg }); From 7f09be8ac6c9a8f69142d6abac7a9cc7b6845def Mon Sep 17 00:00:00 2001 From: Ryan Tu Date: Wed, 24 Nov 2021 10:58:21 +0800 Subject: [PATCH 2/3] Validate stale connection in HTTPClient pool. --- .../settings/ClickHouseConnectionSettings.java | 1 + .../clickhouse/settings/ClickHouseProperties.java | 12 ++++++++++++ .../clickhouse/util/ClickHouseHttpClientBuilder.java | 1 + 3 files changed, 14 insertions(+) diff --git a/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseConnectionSettings.java b/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseConnectionSettings.java index 1b59f6d66..09129e534 100644 --- a/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseConnectionSettings.java +++ b/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseConnectionSettings.java @@ -22,6 +22,7 @@ public enum ClickHouseConnectionSettings implements DriverPropertyCreator { /** * for ConnectionManager */ + VALIDATE_AFTER_INACTIVITY_MILLIS("validateAfterInactivityMillis", 3 * 1000, "period of inactivity in milliseconds after which persistent connections must be re-validated, this check helps detect connections that have become stale (half-closed) while kept inactive in the pool. "), TIME_TO_LIVE_MILLIS("timeToLiveMillis", 60 * 1000, ""), DEFAULT_MAX_PER_ROUTE("defaultMaxPerRoute", 500, ""), MAX_TOTAL("maxTotal", 10000, ""), diff --git a/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseProperties.java b/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseProperties.java index 9e8d831d7..d54ffdd55 100644 --- a/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseProperties.java +++ b/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/settings/ClickHouseProperties.java @@ -17,6 +17,7 @@ public class ClickHouseProperties { private int socketTimeout; private int connectionTimeout; private int timeToLiveMillis; + private int validateAfterInactivityMillis; private int defaultMaxPerRoute; private int maxTotal; private int maxRetries; @@ -110,6 +111,7 @@ public ClickHouseProperties(Properties info) { this.socketTimeout = (Integer)getSetting(info, ClickHouseConnectionSettings.SOCKET_TIMEOUT); this.connectionTimeout = (Integer)getSetting(info, ClickHouseConnectionSettings.CONNECTION_TIMEOUT); this.timeToLiveMillis = (Integer)getSetting(info, ClickHouseConnectionSettings.TIME_TO_LIVE_MILLIS); + this.validateAfterInactivityMillis = (Integer)getSetting(info, ClickHouseConnectionSettings.VALIDATE_AFTER_INACTIVITY_MILLIS); this.defaultMaxPerRoute = (Integer)getSetting(info, ClickHouseConnectionSettings.DEFAULT_MAX_PER_ROUTE); this.maxTotal = (Integer)getSetting(info, ClickHouseConnectionSettings.MAX_TOTAL); this.maxRetries = (Integer)getSetting(info, ClickHouseConnectionSettings.MAX_RETRIES); @@ -177,6 +179,7 @@ public Properties asProperties() { ret.put(ClickHouseConnectionSettings.SOCKET_TIMEOUT.getKey(), String.valueOf(socketTimeout)); ret.put(ClickHouseConnectionSettings.CONNECTION_TIMEOUT.getKey(), String.valueOf(connectionTimeout)); ret.put(ClickHouseConnectionSettings.TIME_TO_LIVE_MILLIS.getKey(), String.valueOf(timeToLiveMillis)); + ret.put(ClickHouseConnectionSettings.VALIDATE_AFTER_INACTIVITY_MILLIS.getKey(), String.valueOf(validateAfterInactivityMillis)); ret.put(ClickHouseConnectionSettings.DEFAULT_MAX_PER_ROUTE.getKey(), String.valueOf(defaultMaxPerRoute)); ret.put(ClickHouseConnectionSettings.MAX_TOTAL.getKey(), String.valueOf(maxTotal)); ret.put(ClickHouseConnectionSettings.MAX_RETRIES.getKey(), String.valueOf(maxRetries)); @@ -247,6 +250,7 @@ public ClickHouseProperties(ClickHouseProperties properties) { setSocketTimeout(properties.socketTimeout); setConnectionTimeout(properties.connectionTimeout); setTimeToLiveMillis(properties.timeToLiveMillis); + setValidateAfterInactivityMillis(properties.validateAfterInactivityMillis); setDefaultMaxPerRoute(properties.defaultMaxPerRoute); setMaxTotal(properties.maxTotal); setMaxRetries(properties.maxRetries); @@ -566,6 +570,14 @@ public void setTimeToLiveMillis(int timeToLiveMillis) { this.timeToLiveMillis = timeToLiveMillis; } + public int getValidateAfterInactivityMillis() { + return validateAfterInactivityMillis; + } + + public void setValidateAfterInactivityMillis(int validateAfterInactivityMillis) { + this.validateAfterInactivityMillis = validateAfterInactivityMillis; + } + public int getDefaultMaxPerRoute() { return defaultMaxPerRoute; } diff --git a/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/util/ClickHouseHttpClientBuilder.java b/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/util/ClickHouseHttpClientBuilder.java index a7fd7956d..782d07896 100644 --- a/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/util/ClickHouseHttpClientBuilder.java +++ b/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/util/ClickHouseHttpClientBuilder.java @@ -151,6 +151,7 @@ private PoolingHttpClientConnectionManager getConnectionManager() TimeUnit.MILLISECONDS ); + connectionManager.setValidateAfterInactivity(properties.getValidateAfterInactivityMillis()); connectionManager.setDefaultMaxPerRoute(properties.getDefaultMaxPerRoute()); connectionManager.setMaxTotal(properties.getMaxTotal()); connectionManager.setDefaultConnectionConfig(getConnectionConfig()); From 9afa1c448907e23500a63b4ab656a6be38b6f537 Mon Sep 17 00:00:00 2001 From: Ryan Tu Date: Wed, 24 Nov 2021 23:03:15 +0800 Subject: [PATCH 3/3] Add test cases for ValidateAfterInactivityMillis settings --- .../util/ClickHouseHttpClientBuilderTest.java | 96 ++++++++++++++++--- 1 file changed, 84 insertions(+), 12 deletions(-) diff --git a/clickhouse-jdbc/src/test/java/ru/yandex/clickhouse/util/ClickHouseHttpClientBuilderTest.java b/clickhouse-jdbc/src/test/java/ru/yandex/clickhouse/util/ClickHouseHttpClientBuilderTest.java index 8ecbcd1f8..26fc5d2b5 100644 --- a/clickhouse-jdbc/src/test/java/ru/yandex/clickhouse/util/ClickHouseHttpClientBuilderTest.java +++ b/clickhouse-jdbc/src/test/java/ru/yandex/clickhouse/util/ClickHouseHttpClientBuilderTest.java @@ -1,6 +1,7 @@ package ru.yandex.clickhouse.util; import org.apache.http.HttpHost; +import org.apache.http.HttpResponse; import org.apache.http.NoHttpResponseException; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpPost; @@ -8,6 +9,7 @@ import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.protocol.BasicHttpContext; import org.apache.http.protocol.HttpContext; +import org.apache.http.util.EntityUtils; import org.testng.annotations.AfterClass; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeClass; @@ -176,7 +178,7 @@ private static Object[][] provideAuthUserPasswordTestData() { }; } - private static WireMockServer newServer() { + private static WireMockServer newServer(int delayMillis) { WireMockServer server = new WireMockServer( WireMockConfiguration.wireMockConfig().dynamicPort()); server.start(); @@ -184,10 +186,14 @@ private static WireMockServer newServer() { .willReturn(WireMock.aResponse().withStatus(200).withHeader("Connection", "Keep-Alive") .withHeader("Content-Type", "text/plain; charset=UTF-8") .withHeader("Transfer-Encoding", "chunked").withHeader("Keep-Alive", "timeout=3") - .withBody("OK.........................").withFixedDelay(2))); + .withBody("OK.........................").withFixedDelay(delayMillis))); return server; } + private static WireMockServer newServer() { + return newServer(2); + } + private static void shutDownServerWithDelay(final WireMockServer server, final long delayMs) { new Thread() { public void run() { @@ -203,38 +209,104 @@ public void run() { }.start(); } - // @Test(dependsOnMethods = { "testWithRetry" }, expectedExceptions = { NoHttpResponseException.class }) - public void testWithoutRetry() throws Exception { - final WireMockServer server = newServer(); + @Test(expectedExceptions = { NoHttpResponseException.class }) + public void testReproduceFailedToResponseProblem() throws Exception { + final WireMockServer server = newServer(2); ClickHouseProperties props = new ClickHouseProperties(); + // Disable retry when "failed to respond" occurs. props.setMaxRetries(0); + // Disable validation to reproduce "failed to respond" problem + props.setValidateAfterInactivityMillis(0); + // Ensure there is exactly one TCP connection in connection pool and therefore be re-used between + // multiple http requests. + props.setMaxTotal(1); + props.setDefaultMaxPerRoute(1); + ClickHouseHttpClientBuilder builder = new ClickHouseHttpClientBuilder(props); CloseableHttpClient client = builder.buildClient(); HttpPost post = new HttpPost("http://localhost:" + server.port() + "/?db=system&query=select%201"); - shutDownServerWithDelay(server, 500); + try { + // Make the 1st http request to establish one tcp connection and keep it in the pool. + { + HttpResponse response = client.execute(post); + EntityUtils.consume(response.getEntity()); + } + + // Close the server, now the pooling tcp connection is half closed. + server.shutdownServer(); + server.stop(); + + // The 2nd http request will re-use the pooling tcp connection which is stale + // and "failed to respond" occurs. + { + HttpResponse response = client.execute(post); + EntityUtils.consume(response.getEntity()); + } + } finally { + client.close(); + } + } + + @Test(expectedExceptions = { HttpHostConnectException.class }) + public void testEnableValidation() throws Exception { + final WireMockServer server = newServer(2); + + ClickHouseProperties props = new ClickHouseProperties(); + // Disable retry when "failed to respond" occurs. + props.setMaxRetries(0); + // Disable validation to reproduce "failed to respond" problem + props.setValidateAfterInactivityMillis(1); + // Ensure there is exactly one TCP connection in connection pool and therefore be re-used between + // multiple http requests. + props.setMaxTotal(1); + props.setDefaultMaxPerRoute(1); + + ClickHouseHttpClientBuilder builder = new ClickHouseHttpClientBuilder(props); + CloseableHttpClient client = builder.buildClient(); + HttpPost post = new HttpPost("http://localhost:" + server.port() + "/?db=system&query=select%201"); try { - client.execute(post); + // Make the 1st http request to establish one tcp connection and keep it in the pool. + { + HttpResponse response = client.execute(post); + EntityUtils.consume(response.getEntity()); + } + + // Sleep a while to wait for the validation reaches inactivity timeout. + Thread.sleep(5); + + // Close the server, now the pooling tcp connection is half closed. + server.shutdownServer(); + server.stop(); + + // The 2nd http request re-uses the pooling tcp connection. + // But the validation checks that the connection has been stale, thus a + // new tcp connection is attempted to establish to the closed server + // which leads to HttpHostConnectException. + { + HttpResponse response = client.execute(post); + EntityUtils.consume(response.getEntity()); + } } finally { client.close(); } } - // @Test(expectedExceptions = { HttpHostConnectException.class }) + @Test(expectedExceptions = { HttpHostConnectException.class }) public void testWithRetry() throws Exception { - final WireMockServer server = newServer(); + final WireMockServer server = newServer(500); ClickHouseProperties props = new ClickHouseProperties(); - // props.setMaxRetries(3); + props.setMaxRetries(3); ClickHouseHttpClientBuilder builder = new ClickHouseHttpClientBuilder(props); CloseableHttpClient client = builder.buildClient(); HttpContext context = new BasicHttpContext(); context.setAttribute("is_idempotent", Boolean.TRUE); HttpPost post = new HttpPost("http://localhost:" + server.port() + "/?db=system&query=select%202"); - - shutDownServerWithDelay(server, 500); + + shutDownServerWithDelay(server, 100); try { client.execute(post, context);