TimonKK / clickhouse

NodeJS client for ClickHouse
Apache License 2.0
220 stars 122 forks source link

Handle stream network error to avoid uncaught error #122

Closed francescorivola closed 1 year ago

francescorivola commented 2 years ago

Due to some transient DNS failure our application crashed due to a bug in the stream() method that does not handle correctly network error. This issue has been already reported in issue #81

The following PR add the fix to handling network errors on the stream method. The fix avoid the uncaught error and allow the consumer of the stream method to handle the error.

TimonKK commented 1 year ago

Could you update test in your PR for this case?

  6 passing (13s)
  1 failing

  1) Select
       streams should handle network error:
     Uncaught Error: expected 'getaddrinfo ENOTFOUND non-existing-clickhouse-server non-existing-clickhouse-server:8123' to equal 'getaddrinfo ENOTFOUND non-existing-clickhouse-server'
      at Assertion.assert (node_modules/expect.js/index.js:96:13)
      at Assertion.be.Assertion.equal (node_modules/expect.js/index.js:216:10)
      at Function.equal (node_modules/expect.js/index.js:499:17)
      at Readable.ClickHouse.query.stream.on.on.error (test/test.js:159:33)
      at Request.<anonymous> (index.js:744:8)
      at Request.onRequestError (node_modules/request/request.js:881:8)
      at Socket.socketErrorListener (_http_client.js:392:9)
      at emitErrorNT (internal/streams/destroy.js:82:8)
      at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
francescorivola commented 1 year ago

Hi @TimonKK , not sure why you are getting a different error message than mine (maybe related to OS or node.js version). I have changed the expect to no longer rely on error.message. Could you please take a look now and tell me if test pass on your side? If it still failing I will change it to use to.contain instead of to.be.equal. Thank you.

TimonKK commented 1 year ago

Thanks for your PR. I published new version https://www.npmjs.com/package/clickhouse

francescorivola commented 1 year ago

Awesome @TimonKK . Thank you so much.