From 93eb15ecc2a3218609fda14f11d7914c02c4c768 Mon Sep 17 00:00:00 2001 From: Serge Klochkov <3175289+slvrtrn@users.noreply.github.com> Date: Tue, 15 Jul 2025 17:32:21 +0200 Subject: [PATCH 1/3] Add an error log on a socket closed without fully consuming the stream --- .docker/clickhouse/single_node_tls/Dockerfile | 2 +- docker-compose.cluster.yml | 4 ++-- docker-compose.yml | 2 +- packages/client-common/src/logger.ts | 6 +++-- .../src/connection/node_base_connection.ts | 22 +++++++++++++++---- 5 files changed, 26 insertions(+), 10 deletions(-) diff --git a/.docker/clickhouse/single_node_tls/Dockerfile b/.docker/clickhouse/single_node_tls/Dockerfile index 3e83e8f4..9506ac7e 100644 --- a/.docker/clickhouse/single_node_tls/Dockerfile +++ b/.docker/clickhouse/single_node_tls/Dockerfile @@ -1,4 +1,4 @@ -FROM clickhouse/clickhouse-server:25.2-alpine +FROM clickhouse/clickhouse-server:25.6-alpine COPY .docker/clickhouse/single_node_tls/certificates /etc/clickhouse-server/certs RUN chown clickhouse:clickhouse -R /etc/clickhouse-server/certs \ && chmod 600 /etc/clickhouse-server/certs/* \ diff --git a/docker-compose.cluster.yml b/docker-compose.cluster.yml index bd39c10c..c4845503 100644 --- a/docker-compose.cluster.yml +++ b/docker-compose.cluster.yml @@ -2,7 +2,7 @@ version: '2.3' services: clickhouse1: - image: 'clickhouse/clickhouse-server:${CLICKHOUSE_VERSION-25.2-alpine}' + image: 'clickhouse/clickhouse-server:${CLICKHOUSE_VERSION-25.6-alpine}' ulimits: nofile: soft: 262144 @@ -21,7 +21,7 @@ services: - './.docker/clickhouse/users.xml:/etc/clickhouse-server/users.xml' clickhouse2: - image: 'clickhouse/clickhouse-server:${CLICKHOUSE_VERSION-25.2-alpine}' + image: 'clickhouse/clickhouse-server:${CLICKHOUSE_VERSION-25.6-alpine}' ulimits: nofile: soft: 262144 diff --git a/docker-compose.yml b/docker-compose.yml index 08b21cd6..ae5c5f96 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,7 +1,7 @@ #version: '3.8' services: clickhouse: - image: 'clickhouse/clickhouse-server:${CLICKHOUSE_VERSION-25.2-alpine}' + image: 'clickhouse/clickhouse-server:${CLICKHOUSE_VERSION-25.6-alpine}' container_name: 'clickhouse-js-clickhouse-server' environment: CLICKHOUSE_SKIP_USER_SETUP: 1 diff --git a/packages/client-common/src/logger.ts b/packages/client-common/src/logger.ts index 93da94f6..469e2195 100644 --- a/packages/client-common/src/logger.ts +++ b/packages/client-common/src/logger.ts @@ -4,7 +4,7 @@ export interface LogParams { message: string args?: Record } -export type ErrorLogParams = LogParams & { err: Error } +export type ErrorLogParams = LogParams & { err?: Error } export type WarnLogParams = LogParams & { err?: Error } export interface Logger { trace(params: LogParams): void @@ -65,7 +65,9 @@ export class DefaultLogger implements Logger { if (args) { params.push('\nArguments:', args) } - params.push('\nCaused by:', err) + if (err) { + params.push('\nCaused by:', err) + } console.error(...params) } } diff --git a/packages/client-node/src/connection/node_base_connection.ts b/packages/client-node/src/connection/node_base_connection.ts index 5f2b6366..b616c053 100644 --- a/packages/client-node/src/connection/node_base_connection.ts +++ b/packages/client-node/src/connection/node_base_connection.ts @@ -72,6 +72,7 @@ export interface RequestParams { // if there are compression headers, attempt to decompress it try_decompress_response_stream?: boolean parse_summary?: boolean + query: string } export abstract class NodeBaseConnection @@ -115,6 +116,7 @@ export abstract class NodeBaseConnection url: transformUrl({ url: this.params.url, pathname: '/ping' }), abort_signal: abortController.signal, headers: this.buildRequestHeaders(), + query: 'ping', }, 'Ping', ) @@ -157,7 +159,7 @@ export abstract class NodeBaseConnection const enableResponseCompression = clickhouse_settings.enable_http_compression === 1 try { - const { stream, response_headers } = await this.request( + const { response_headers, stream } = await this.request( { method: 'POST', url: transformUrl({ url: this.params.url, searchParams }), @@ -165,13 +167,14 @@ export abstract class NodeBaseConnection abort_signal: controller.signal, enable_response_compression: enableResponseCompression, headers: this.buildRequestHeaders(params), + query: params.query, }, 'Query', ) return { stream, - query_id, response_headers, + query_id, } } catch (err) { controller.abort('Query HTTP request failed') @@ -216,6 +219,7 @@ export abstract class NodeBaseConnection enable_request_compression: this.params.compression.compress_request, parse_summary: true, headers: this.buildRequestHeaders(params), + query: params.query, }, 'Insert', ) @@ -445,6 +449,7 @@ export abstract class NodeBaseConnection this.params.compression.decompress_response, try_decompress_response_stream: tryDecompressResponseStream, headers: this.buildRequestHeaders(params), + query: params.query, }, params.op, ) @@ -493,12 +498,11 @@ export abstract class NodeBaseConnection reject(err) } + let responseStream: Stream.Readable const onResponse = async ( _response: Http.IncomingMessage, ): Promise => { this.logResponse(op, request, params, _response, start) - - let responseStream: Stream.Readable const tryDecompressResponseStream = params.try_decompress_response_stream ?? true // even if the stream decompression is disabled, we have to decompress it in case of an error @@ -637,6 +641,16 @@ export abstract class NodeBaseConnection this.logger.trace({ message: `Socket ${socketId} was closed or ended, 'free' listener removed`, }) + if (!responseStream.readableEnded) { + this.logger.error({ + message: `${op}: socket was closed or ended before the response was fully read. This can potentially result in an uncaught ECONNRESET error! Consider fully consuming, draining, or destroying the response stream`, + args: { + query: params.query, + query_id: + params.url.searchParams.get('query_id') ?? 'unknown', + }, + }) + } } socket.once('end', cleanup) socket.once('close', cleanup) From b6db9776bffe169e2b3b6ee98950da26056c89e1 Mon Sep 17 00:00:00 2001 From: Serge Klochkov <3175289+slvrtrn@users.noreply.github.com> Date: Tue, 15 Jul 2025 17:35:37 +0200 Subject: [PATCH 2/3] Use WARN log level, revert logger changes --- packages/client-common/src/logger.ts | 6 ++---- .../client-node/src/connection/node_base_connection.ts | 7 +++++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/client-common/src/logger.ts b/packages/client-common/src/logger.ts index 469e2195..93da94f6 100644 --- a/packages/client-common/src/logger.ts +++ b/packages/client-common/src/logger.ts @@ -4,7 +4,7 @@ export interface LogParams { message: string args?: Record } -export type ErrorLogParams = LogParams & { err?: Error } +export type ErrorLogParams = LogParams & { err: Error } export type WarnLogParams = LogParams & { err?: Error } export interface Logger { trace(params: LogParams): void @@ -65,9 +65,7 @@ export class DefaultLogger implements Logger { if (args) { params.push('\nArguments:', args) } - if (err) { - params.push('\nCaused by:', err) - } + params.push('\nCaused by:', err) console.error(...params) } } diff --git a/packages/client-node/src/connection/node_base_connection.ts b/packages/client-node/src/connection/node_base_connection.ts index b616c053..35360614 100644 --- a/packages/client-node/src/connection/node_base_connection.ts +++ b/packages/client-node/src/connection/node_base_connection.ts @@ -642,8 +642,11 @@ export abstract class NodeBaseConnection message: `Socket ${socketId} was closed or ended, 'free' listener removed`, }) if (!responseStream.readableEnded) { - this.logger.error({ - message: `${op}: socket was closed or ended before the response was fully read. This can potentially result in an uncaught ECONNRESET error! Consider fully consuming, draining, or destroying the response stream`, + this.logger.warn({ + message: + `${op}: socket was closed or ended before the response was fully read. ` + + 'This can potentially result in an uncaught ECONNRESET error! ' + + 'Consider fully consuming, draining, or destroying the response stream.', args: { query: params.query, query_id: From 32eceb8c58ea53c2ce034e53dce5d09b376ae74e Mon Sep 17 00:00:00 2001 From: Serge Klochkov <3175289+slvrtrn@users.noreply.github.com> Date: Tue, 15 Jul 2025 17:40:45 +0200 Subject: [PATCH 3/3] Add a missing undefined check --- packages/client-node/src/connection/node_base_connection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/client-node/src/connection/node_base_connection.ts b/packages/client-node/src/connection/node_base_connection.ts index 35360614..9f2c5027 100644 --- a/packages/client-node/src/connection/node_base_connection.ts +++ b/packages/client-node/src/connection/node_base_connection.ts @@ -641,7 +641,7 @@ export abstract class NodeBaseConnection this.logger.trace({ message: `Socket ${socketId} was closed or ended, 'free' listener removed`, }) - if (!responseStream.readableEnded) { + if (responseStream && !responseStream.readableEnded) { this.logger.warn({ message: `${op}: socket was closed or ended before the response was fully read. ` +