Skip to content
New issue

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

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

Already on GitHub? # to your account

Request promises resolved and then rejected #123

Closed
richardbutler opened this issue Nov 11, 2022 · 3 comments · Fixed by #124
Closed

Request promises resolved and then rejected #123

richardbutler opened this issue Nov 11, 2022 · 3 comments · Fixed by #124

Comments

@richardbutler
Copy link

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

slvrtrn commented Nov 14, 2022

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

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

@richardbutler
Copy link
Author

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants