Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle stream network error to avoid uncaught error #122

Merged

Conversation

francescorivola
Copy link
Contributor

@francescorivola francescorivola commented Jun 8, 2022

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.

@francescorivola francescorivola mentioned this pull request Jun 8, 2022
@TimonKK
Copy link
Owner

TimonKK commented Aug 15, 2022

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
Copy link
Contributor Author

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 TimonKK merged commit 8290a82 into TimonKK:master Aug 16, 2022
@TimonKK
Copy link
Owner

TimonKK commented Aug 16, 2022

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

@francescorivola
Copy link
Contributor Author

Awesome @TimonKK . Thank you so much.

@francescorivola francescorivola deleted the bug/stream-uncaught-network-error branch August 17, 2022 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants