http client error events might be suppressed
- Dominant language
- JavaScript
- Stars
- 280
- Forks
- 157
- PR merge metrics
- No merged PRs in 30d
Description
I think [this check](https://github.com/aws/aws-xray-sdk-node/blob/0590805fe2591ba7cc2c6891caec4ef43fed40e1/packages/core/lib/patchers/http_p.js#L141) in http_p.js should be improved as it may result in not re-emitting an error event.
`this._events.error` is either an Array (if more then 1 listener is installed) or `Function` (only 1 listener installed).
Currently this check works because the installed listener `errorCapturer` has exactly 1 named parameter therefore `errorCapturer.length === 1`.
But if there is a module used which wraps the installed listeners (e.g. for CLS,...) or if `once` is used to install the listener this is broken. Besides that it relies on internals of `EventEmitter` (the `_events` field`).
I recommend to use the public `EventEmitter` Api to check that <= 1 listener is installed.
Recent NodeJs versions allow to listen on [errorMonitor](https://nodejs.org/dist/latest-v12.x/docs/api/events.html#events_eventemitter_errormonitor) to avoid the need to re-emit in case only 1 listener is present at all.
Edit:
Found that also [postgres](https://github.com/aws/aws-xray-sdk-node/blob/9cb04b6ddcabca33dc483c8e1d3e82d6fb108e80/packages/postgres/lib/postgres_p.js#L109) and [mysql](https://github.com/aws/aws-xray-sdk-node/blob/9cb04b6ddcabca33dc483c8e1d3e82d6fb108e80/packages/mysql/lib/mysql_p.js#L209) is effected.
Contributor guide
Assessment
This issue has not been assessed yet.