[1.11.2] Add enhanced stack traces and a few more try-catch blocks

CHANGELOG.md

@@ -1,3 +1,15 @@
+# 1.11.2 (Common, Node.js)
+
+A minor release to allow further investigation regarding uncaught error issues with [#410].
+
+## Improvements (Node.js)
+
+- Added a new configuration option: `capture_enhanced_stack_trace`; see the JS doc in the Node.js client package. Note that it is disabled by default due to a possible performance impact. ([#427])
+- Added more try-catch blocks to the Node.js connection layer. ([#427])
+
+[#410]: https://github.com/ClickHouse/clickhouse-js/pull/410
+[#427]: https://github.com/ClickHouse/clickhouse-js/pull/427
+
 # 1.11.1 (Common, Node.js, Web)
 
 ## Bug fixes

packages/client-common/__tests__/unit/parse_error.test.ts → packages/client-common/__tests__/unit/error.test.ts

@@ -1,4 +1,9 @@
-import { ClickHouseError, parseError } from '@clickhouse/client-common'
+import {
+  ClickHouseError,
+  enhanceStackTrace,
+  getCurrentStackTrace,
+  parseError,
+} from '@clickhouse/client-common'
 
 describe('parseError', () => {
   it('parses a single line error', () => {
@@ -77,6 +82,43 @@ describe('parseError', () => {
     })
   })
 
+  describe('getCurrentStackTrace', () => {
+    it('should trim the stack trace as expected', async () => {
+      const currentStack = getCurrentStackTrace()
+      expect(currentStack.split('\n').length).toBeGreaterThanOrEqual(3)
+      expect(currentStack).not.toContain(getCurrentStackTrace.name)
+      expect(currentStack).not.toContain('Error:')
+      expect(currentStack).not.toContain('UserContext')
+      expect(currentStack).not.toContain('error.test.ts')
+    })
+  })
+
+  describe('enhanceStackTrace', () => {
+    it('should no-op on an empty error or stack trace', async () => {
+      const err = new Error('test error')
+      err.stack = undefined
+
+      const result1 = enhanceStackTrace(err, 'ignored')
+      expect(result1.stack).toBeUndefined()
+      expect(result1.message).toBe('test error')
+
+      err.stack = ''
+      const result2 = enhanceStackTrace(err, 'ignored')
+      expect(result2.stack).toBe('')
+      expect(result2.message).toBe('test error')
+    })
+
+    it('should enhance the stack trace of an error', () => {
+      const err = new Error('test error')
+      err.stack = 'foo\n'
+      const stackTrace = 'bar\n'
+      const enhancedError = enhanceStackTrace(err, stackTrace)
+
+      expect(enhancedError.stack).toContain(`foo\nbar\n`)
+      expect(enhancedError.message).toBe('test error')
+    })
+  })
+
   xdescribe('Cluster mode errors', () => {
     // FIXME: https://github.com/ClickHouse/clickhouse-js/issues/39
     it('should work with TABLE_ALREADY_EXISTS', async () => {

packages/client-common/src/error/error.ts

@@ -0,0 +1,65 @@
+const errorRe =
+  /(Code|Error): (?<code>\d+).*Exception: (?<message>.+)\((?<type>(?=.+[A-Z]{3})[A-Z0-9_]+?)\)/s
+
+interface ParsedClickHouseError {
+  message: string
+  code: string
+  type?: string
+}
+
+/** An error that is thrown by the ClickHouse server. */
+export class ClickHouseError extends Error {
+  readonly code: string
+  readonly type: string | undefined
+  constructor({ message, code, type }: ParsedClickHouseError) {
+    super(message)
+    this.code = code
+    this.type = type
+
+    // Set the prototype explicitly, see:
+    // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
+    Object.setPrototypeOf(this, ClickHouseError.prototype)
+  }
+}
+
+export function parseError(input: string | Error): ClickHouseError | Error {
+  const inputIsError = input instanceof Error
+  const message = inputIsError ? input.message : input
+  const match = message.match(errorRe)
+  const groups = match?.groups as ParsedClickHouseError | undefined
+  if (groups) {
+    return new ClickHouseError(groups)
+  } else {
+    return inputIsError ? input : new Error(input)
+  }
+}
+
+/** Captures the current stack trace from the sync context before going async.
+ *  It is necessary since the majority of the stack trace is lost when an async callback is called. */
+export function getCurrentStackTrace(): string {
+  const stack = new Error().stack
+  if (!stack) return ''
+
+  // Skip the first three lines of the stack trace, containing useless information
+  // - Text `Error`
+  // - Info about this function call
+  // - Info about the originator of this function call, e.g., `request`
+  // Additionally, the original stack trace is, in fact, reversed.
+  return stack.split('\n').slice(3).reverse().join('\n')
+}
+
+/** Having the stack trace produced by the {@link getCurrentStackTrace} function,
+ *  add it to an arbitrary error stack trace. No-op if there is no additional stack trace to add.
+ *  It could happen if this feature was disabled due to its performance overhead. */
+export function enhanceStackTrace<E extends Error>(
+  err: E,
+  stackTrace: string | undefined,
+): E {
+  if (err.stack && stackTrace) {
+    const firstNewlineIndex = err.stack.indexOf('\n')
+    const firstLine = err.stack.substring(0, firstNewlineIndex)
+    const errStack = err.stack.substring(firstNewlineIndex + 1)
+    err.stack = `${firstLine}\n${stackTrace}\n${errStack}`
+  }
+  return err
+}

packages/client-common/src/error/index.ts

@@ -1 +1 @@
-export * from './parse_error'
+export * from './error'

packages/client-common/src/index.ts

@@ -80,7 +80,7 @@ export type {
 } from './parse'
 export { SimpleColumnTypes, parseColumnType } from './parse'
 
-/** For implementations usage only - should not be re-exported */
+/** For implementation usage only - should not be re-exported */
 export {
   formatQuerySettings,
   formatQueryParams,
@@ -112,7 +112,7 @@ export {
   isJWTAuth,
 } from './utils'
 export { LogWriter, DefaultLogger, type LogWriterParams } from './logger'
-export { parseError } from './error'
+export { parseError, getCurrentStackTrace, enhanceStackTrace } from './error'
 export type {
   CompressionSettings,
   Connection,

packages/client-common/src/version.ts

@@ -1 +1 @@
-export default '1.11.1'
+export default '1.11.2'

packages/client-node/__tests__/unit/node_client.test.ts

@@ -75,6 +75,7 @@ describe('[Node.js] createClient', () => {
         },
         set_basic_auth_header: true,
         http_agent: undefined,
+        capture_enhanced_stack_trace: false,
       } satisfies CreateConnectionParams)
       expect(createConnectionStub).toHaveBeenCalledTimes(1)
     })
@@ -105,6 +106,7 @@ describe('[Node.js] createClient', () => {
         },
         set_basic_auth_header: true,
         http_agent: undefined,
+        capture_enhanced_stack_trace: false,
       } satisfies CreateConnectionParams)
       expect(createConnectionStub).toHaveBeenCalledTimes(1)
     })
@@ -139,6 +141,7 @@ describe('[Node.js] createClient', () => {
         },
         set_basic_auth_header: true,
         http_agent: undefined,
+        capture_enhanced_stack_trace: false,
       } satisfies CreateConnectionParams)
       expect(createConnectionStub).toHaveBeenCalledTimes(1)
     })

packages/client-node/__tests__/unit/node_config.test.ts

@@ -104,6 +104,7 @@ describe('[Node.js] Config implementation details', () => {
         },
         http_agent: undefined,
         set_basic_auth_header: true,
+        capture_enhanced_stack_trace: false,
       } satisfies CreateConnectionParams)
       expect(createConnectionStub).toHaveBeenCalledTimes(1)
       expect(res).toEqual(fakeConnection)
@@ -129,6 +130,7 @@ describe('[Node.js] Config implementation details', () => {
         },
         http_agent: undefined,
         set_basic_auth_header: true,
+        capture_enhanced_stack_trace: false,
       } satisfies CreateConnectionParams)
       expect(createConnectionStub).toHaveBeenCalledTimes(1)
       expect(res).toEqual(fakeConnection)
@@ -158,6 +160,7 @@ describe('[Node.js] Config implementation details', () => {
         },
         http_agent: undefined,
         set_basic_auth_header: true,
+        capture_enhanced_stack_trace: false,
       } satisfies CreateConnectionParams)
       expect(createConnectionStub).toHaveBeenCalledTimes(1)
       expect(res).toEqual(fakeConnection)
@@ -187,6 +190,7 @@ describe('[Node.js] Config implementation details', () => {
         },
         http_agent: undefined,
         set_basic_auth_header: true,
+        capture_enhanced_stack_trace: false,
       } satisfies CreateConnectionParams)
       expect(createConnectionStub).toHaveBeenCalledTimes(1)
       expect(res).toEqual(fakeConnection)
@@ -215,6 +219,28 @@ describe('[Node.js] Config implementation details', () => {
         },
         http_agent: agent,
         set_basic_auth_header: false,
+        capture_enhanced_stack_trace: false,
+      } satisfies CreateConnectionParams)
+      expect(createConnectionStub).toHaveBeenCalledTimes(1)
+      expect(res).toEqual(fakeConnection)
+    })
+
+    it('should create a connection with enhanced stack traces option', async () => {
+      const nodeConfig: NodeClickHouseClientConfigOptions = {
+        url: new URL('https://localhost:8123'),
+        capture_enhanced_stack_trace: true,
+      }
+      const res = NodeConfigImpl.make_connection(nodeConfig as any, params)
+      expect(createConnectionStub).toHaveBeenCalledWith({
+        connection_params: params,
+        tls: undefined,
+        keep_alive: {
+          enabled: true,
+          idle_socket_ttl: 2500,
+        },
+        http_agent: undefined,
+        set_basic_auth_header: true,
+        capture_enhanced_stack_trace: true,
       } satisfies CreateConnectionParams)
       expect(createConnectionStub).toHaveBeenCalledTimes(1)
       expect(res).toEqual(fakeConnection)

packages/client-node/__tests__/unit/node_create_connection.test.ts

@@ -32,6 +32,7 @@ describe('[Node.js] createConnection', () => {
       keep_alive: keepAliveParams,
       http_agent: undefined,
       set_basic_auth_header: true,
+      capture_enhanced_stack_trace: false,
     })
     expect(adapter).toBeInstanceOf(NodeHttpConnection)
   })
@@ -46,6 +47,7 @@ describe('[Node.js] createConnection', () => {
       keep_alive: keepAliveParams,
       http_agent: undefined,
       set_basic_auth_header: true,
+      capture_enhanced_stack_trace: false,
     })
     expect(adapter).toBeInstanceOf(NodeHttpsConnection)
   })
@@ -61,6 +63,7 @@ describe('[Node.js] createConnection', () => {
         keep_alive: keepAliveParams,
         http_agent: undefined,
         set_basic_auth_header: true,
+        capture_enhanced_stack_trace: false,
       }),
     ).toThrowError('Only HTTP and HTTPS protocols are supported')
   })
@@ -76,6 +79,7 @@ describe('[Node.js] createConnection', () => {
           maxSockets: 2,
         }),
         set_basic_auth_header: false,
+        capture_enhanced_stack_trace: false,
       })
       expect(adapter).toBeInstanceOf(NodeCustomAgentConnection)
     })
@@ -90,6 +94,7 @@ describe('[Node.js] createConnection', () => {
           maxSockets: 2,
         }),
         set_basic_auth_header: true,
+        capture_enhanced_stack_trace: false,
       })
       expect(adapter).toBeInstanceOf(NodeCustomAgentConnection)
     })

packages/client-node/__tests__/utils/http_stubs.ts

@@ -115,6 +115,7 @@ export function buildHttpConnection(config: Partial<NodeConnectionParams>) {
       idle_socket_ttl: 2500,
     },
     set_basic_auth_header: true,
+    capture_enhanced_stack_trace: false,
     ...config,
   })
 }

packages/client-node/src/config.ts

@@ -20,7 +20,7 @@ export type NodeClickHouseClientConfigOptions =
     tls?: BasicTLSOptions | MutualTLSOptions
     /** HTTP Keep-Alive related settings */
     keep_alive?: {
-      /** Enable or disable HTTP Keep-Alive mechanism.
+      /** Enable or disable the HTTP Keep-Alive mechanism.
        *  @default true */
       enabled?: boolean
       /** For how long keep a particular idle socket alive on the client side (in milliseconds).
@@ -33,13 +33,25 @@ export type NodeClickHouseClientConfigOptions =
     /** Custom HTTP agent to use for the outgoing HTTP(s) requests.
      *  If set, {@link BaseClickHouseClientConfigOptions.max_open_connections}, {@link tls} and {@link keep_alive}
      *  options have no effect, as it is part of the default underlying agent configuration.
-     *  @experimental - unstable API, might be a subject to change in the future; please provide your feedback in the repository.
+     *  @experimental - unstable API; it might be a subject to change in the future;
+     *                  please provide your feedback in the repository.
      *  @default undefined */
     http_agent?: http.Agent | https.Agent
     /** Enable or disable the `Authorization` header with basic auth for the outgoing HTTP(s) requests.
-     *  @experimental - unstable API, might be a subject to change in the future; please provide your feedback in the repository.
+     *  @experimental - unstable API; it might be a subject to change in the future;
+     *                  please provide your feedback in the repository.
      *  @default true (enabled) */
     set_basic_auth_header?: boolean
+    /** You could try enabling this option if you encounter an error with an unclear or truncated stack trace;
+     *  as it might happen due to the way the Node.js handles the stack traces in the async code.
+     *  Note that it might have a noticeable performance impact due to
+     *  capturing the full stack trace on each client method call.
+     *  It could also be necessary to override `Error.stackTraceLimit` and increase it
+     *  to a higher value, or even to `Infinity`, as the default value Node.js is just `10`.
+     *  @experimental - unstable API; it might be a subject to change in the future;
+     *                  please provide your feedback in the repository.
+     *  @default false (disabled) */
+    capture_enhanced_stack_trace?: boolean
   }
 
 interface BasicTLSOptions {
@@ -112,6 +124,8 @@ export const NodeConfigImpl: Required<
     return createConnection({
       connection_params: params,
       set_basic_auth_header: nodeConfig.set_basic_auth_header ?? true,
+      capture_enhanced_stack_trace:
+        nodeConfig.capture_enhanced_stack_trace ?? false,
       http_agent: nodeConfig.http_agent,
       keep_alive,
       tls,