ClickHouse / clickhouse-js

Official JS client for ClickHouse DB
https://clickhouse.com
Apache License 2.0
225 stars 27 forks source link

Request promises resolved and then rejected #123

Closed richardbutler closed 1 year ago

richardbutler commented 1 year ago

Node version: 16.17.1

I use the log-process-errors package to log errors throughout my application and Clickhouse queries keep triggering the "multiple resolves" logger. This is because, in cases of a successful query, the promise is getting resolved and then rejected.

The HTTP Adapter code seems to remove listeners asynchronously, causing the steps to be taken in the following order:

  1. onResponse => resolves the promise
  2. onClose => calls setImmediate(removeRequestListeners)
  3. onAbort => rejects the promise, but should be ignored, and would be if the listeners were removed synchronously
  4. removeRequestListeners

Taking note of the following comment in the code, I see why this was done:

// Adapter uses 'close' event to clean up listeners after the successful response.
// It's necessary in order to handle 'abort' and 'timeout' events while response is streamed.
// setImmediate is a workaround. If a request cancelled before sent, the 'abort' happens after 'close'.
// Which contradicts the docs https://nodejs.org/docs/latest-v14.x/api/http.html#http_http_request_url_options_callback

However, even though multiple resolves is a minor issue, it is still noisy and poor hygiene.

slvrtrn commented 1 year ago

The fix will be available as soon as a new release is pushed (0.0.10) Can you please verify that it works as expected?

richardbutler commented 1 year ago

Sure, thanks for the quick turnaround. I'll look out for the release and report back.

richardbutler commented 1 year ago

@slvrtrn thanks for the fix, much appreciated. The multiple resolves warnings appear to have gone now.