launchdarkly / node-server-sdk

LaunchDarkly Server-side SDK for Node
Other
79 stars 65 forks source link

"listener argument must be of type function" error with v6.2.2+ #233

Closed deriegle closed 2 years ago

deriegle commented 2 years ago

Is this a support request? No.

Describe the bug I'm receiving an error when upgrading to version 6.2.2 of the launchdarkly-node-server-sdk library.

Error Message: The \"listener\" argument must be of type function. Received an instance of Object

TypeError [ERR_INVALID_ARG_TYPE]: The \"listener\" argument must be of type function. Received an instance of Object

The stack trace references these files:

at connect (/node_modules/launchdarkly-eventsource/lib/eventsource.js:290:11)
at new EventSource (/node_modules/launchdarkly-eventsource/lib/eventsource.js:314:4)
at Object.StreamProcessor.processor.start (/node_modules/launchdarkly-node-server-sdk/streaming.js:65:10)
at Object.newClient [as init] (/node_modules/launchdarkly-node-server-sdk/index.js:138:19)

To reproduce Use version 6.2.2 of launchdarkly-node-server-sdk.

const client = await LaunchDarkly.init(process.env.LAUNCH_DARKLY_API_KEY);

Expected behavior I would expect a minimum version to not introduce breaking changes and that this would not blow up. 6.2.1 worked fine.

Logs See Description

SDK version 6.2.2

Language version, developer tools Node v14.18.2

OS/platform Docker container node:14.18-bullseye

Additional context Add any other context about the problem here.

eli-darkly commented 2 years ago

This is very odd, because Node 14 is a supported version that is tested in our CI, and the error you're reporting is one that I've only seen in Node 8 and other EOL versions. It is complaining that https.request is being called with three parameters (url, options, callback) rather than two (options, callback), which is a recent change we made that's incompatible with older Nodes but should work fine in 14. I'll try to reproduce this.

deriegle commented 2 years ago

Thank you for looking into this @eli-darkly. I'm fairly confident that's the version we're using based on the Dockerfile. Please let me know if you need me to create a repo to reproduce the issue. I only put that single line, because that was the line our code was throwing on, but maybe additional context would be helpful.

eli-darkly commented 2 years ago

I've been unable to reproduce this. My steps were:

  1. Clone our minimal hello-world demo, https://github.com/launchdarkly/hello-node-server.
  2. Edit index.js to add a valid SDK key on line 4 (and, if desired, a valid flag key on line 7— although that's not really relevant since all we're testing is that it can connect to LaunchDarkly).
  3. Run npm install and then node index.js.

I tried this in Node 14.18.2, first using the Linux Docker image you mentioned, then using the same Node version on a Mac. Both times it worked as expected and did not show the error you reported.

Then I did the same thing with node:8.16.2-stretch and got the error— as I would expect in Node 8.

So... I apologize for the silly question, but are you very sure you're running Node 14?

eli-darkly commented 2 years ago

A quick web search did turn up this report of what sounds like the same problem, in Node 14. I just don't know how to account for that. Not only does it look like perfectly valid usage, but it's working fine for me.

Is it possible that there are any other modules you're using that could alter the behavior of https.request? One such tool, MockServiceWorker, was the reason that we made this change in the first place— MSW monkey-patches request, and it was doing so in a basically correct way, but it revealed a problem that was most easily fixed by using 3 parameters instead of 2. But if some other tool is doing something similar and does not know how to handle the 3-parameter syntax, it might cause something like this. So it might be helpful to see your project's dependencies.

Also just as a sanity check, I'd like to know whether you are able to reproduce this with an extremely minimal example like the one I mentioned above, rather than in your application.

deriegle commented 2 years ago

Thanks so much for looking into this further. The minimum example works fine for us too. Now that you mention that, I actually think it might another library that we're including. I'm looking into that. We include Google cloud tracer, that seems like a likely culprit.

I will try a few of the different libraries we include to get a minimum reproducible repo in the morning since it's so late.

wyardley commented 2 years ago

Just to tag on to @deriegle's response, yes, confident it's the right version; as he mentioned, a very minimal invocation of the latest version of the library doesn't seem to error from a quick test.

node@xxx:/usr/src/app$ cd /tmp/
node@xxx:/tmp$ npm install launchdarkly-node-server-sdk@latest
+ launchdarkly-node-server-sdk@6.2.2
added 14 packages from 29 contributors and audited 14 packages in 1.864s
found 0 vulnerabilities

node@xxx:/tmp$ node
Welcome to Node.js v14.18.2.
Type ".help" for more information.
> const LaunchDarkly = require('launchdarkly-node-server-sdk');
undefined
> LaunchDarkly.init(process.env.FOO);
info: [LaunchDarkly] Initializing stream processor to receive feature flag updates
EventEmitter {
  _events: [Object: null prototype] {},
  _eventsCount: 0,
  _maxListeners: undefined,
  initialized: [Function (anonymous)],
[...]

We will dig into this some more tomorrow and see if we can come up with a better reproduction case. Even with all our normal prod dependencies, I can't repro it with a very vanilla instantiation of the library, however, it is very possible that there is something else going on at the code level and / or it's something related to express middleware.

eli-darkly commented 2 years ago

It makes sense to me that middleware might be involved, but if so, I would've expected to see some reference to the middleware in the stacktrace. @deriegle, were there any lower-level calls in the stacktrace that weren't included in the part you copied?

If it turns out that there's some bad interaction with something like Google Cloud Trace, then we'll probably need to implement a workaround even if it might not be strictly our fault, since that's a pretty widely used tool.

deriegle commented 2 years ago

@eli-darkly I was able to get a reproduction of the issue in this repository. It seems like it was an interaction with @google-cloud/trace-agent. I had seen that in the stack trace, but had just assumed it was wrapping everything in some way. I didn't expect it was the interaction between the two libraries.

When using our specific version of @google-cloud/trace-agent (3.6.1), it blows up consistently. That version is quite old though (~3 years). If I upgrade to the next version that was released (4.0.0) (also ~3 years old) or the major version (5.1.6), I'm not seeing this issue.

This is likely a non-issue for you because of how old this library is. We will work on upgrading this and related libraries before upgrading to the next version of launchdarkly-node-server-sdk. Sorry for making you go down a rabbit hole! Thank you for being so helpful in investigating this issue.

Full Error Stack Trace

TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type function. Received an instance of Object
    at new NodeError (internal/errors.js:322:7)
    at checkListener (events.js:135:11)
    at ClientRequest.once (events.js:539:3)
    at new ClientRequest (_http_client.js:215:10)
    at Object.request (https.js:370:10)
    at /*/node_modules/@google-cloud/trace-agent/node_modules/agent-base/patch-core.js:25:22
    at Object.requestTrace [as request] (/*/node_modules/@google-cloud/trace-agent/build/src/plugins/plugin-http.js:97:20)
    at connect (/*/node_modules/launchdarkly-eventsource/lib/eventsource.js:290:11)
    at new EventSource (/*/node_modules/launchdarkly-eventsource/lib/eventsource.js:314:3)
    at Object.StreamProcessor.processor.start (/*/node_modules/launchdarkly-node-server-sdk/streaming.js:65:10)
    at Object.newClient [as init] (/*/node_modules/launchdarkly-node-server-sdk/index.js:138:19)
deriegle commented 2 years ago

I'm closing this issue as it's unrelated to this library itself. Feel free to open it again if there's something else that needs done. Again, appreciate the quick response!

eli-darkly commented 2 years ago

@deriegle Aha! Good to know— I was hoping that if it was Google Cloud Trace, that it was only a problem in older versions, and it sounds like that's true. I'll let our support team know for future reference in case they ever get a similar report. Thanks.

jkillian commented 2 years ago

Just hit a similar issue, but for us it was coming through our use of passport-azure-ad.

TypeError: The "listener" argument must be of type function. Received an instance of Object
    at checkListener (node:events:128:3)
    at ClientRequest.once (node:events:529:3)
    at new ClientRequest (node:_http_client:209:10)
    at Object.request (node:https:353:10)
    at Object.request (<omitted>/node_modules/passport-azure-ad/node_modules/agent-base/patch-core.js:25:22)
    at connect (<omitted>/node_modules/launchdarkly-eventsource/lib/eventsource.js:290:11)
    at new EventSource (<omitted>/node_modules/launchdarkly-eventsource/lib/eventsource.js:314:3)
    at Object.StreamProcessor.processor.start (<omitted>/node_modules/launchdarkly-node-server-sdk/streaming.js:65:10)
    at Object.newClient <omitted>/node_modules/launchdarkly-node-server-sdk/index.js:138:19)
    at Server.start (<omitted>/packages/server/server.ts:630:35) {
  code: 'ERR_INVALID_ARG_TYPE'

The resolution for us was to simply bump our version of passport-azure-ad. The more underlying root cause though was the same as in @deriegle's case I suspect: an old version of agent-base getting pulled in transitively. So if anyone else hits this issue, worth checking what versions of agent-base you have in your lockfiles!

eli-darkly commented 2 years ago

I've updated the issue title to more specifically mention the error message, so that anyone who runs into something similar and wants to see if it's a known issue has a better chance of finding it.

hoanglamhuynh commented 2 years ago

I'm having the same problem as well, basically anything that uses agent-base will emit the error. I'm using Node 16

EDIT: The issue is unrelated to launchdarkly, but rather, some other packages has pulled in an agent-base that somehow causes this error

eli-darkly commented 2 years ago

@hoanglamhuynh - Looking at how agent-base is implemented, I'm not seeing how it could cause this. It looks to me like it is just providing an explicitly constructed implementation of an agent, not changing the behavior of the top-level https.request function.

Could you say more about how you determined that "anything that uses agent-base will emit the error"? And provide an example of how to reproduce this, or at least what your dependencies are?

hoanglamhuynh commented 2 years ago

hi @eli-darkly, I've debugged and confirmed, the issue is not related to launchdarkly itself. I ran launchdarkly separately and it worked, but when put inside my express project it fails. I'll continue to see which one causes the error. Thank you

jkillian commented 2 years ago

Could you say more about how you determined that "anything that uses agent-base will emit the error"? And provide an example of how to reproduce this, or at least what your dependencies are?

It's only projects using old versions of agent-base that will have the error. Versions 4.x of agent-base and older contained a patch-core file which did change the top-level Node https.request behavior. This was changed for 5.0, so anyone using agent-base 5.0 or newer should be fine.

HarryMarch commented 2 years ago

@hoanglamhuynh - Looking at how agent-base is implemented, I'm not seeing how it could cause this. It looks to me like it is just providing an explicitly constructed implementation of an agent, not changing the behavior of the top-level https.request function.

Could you say more about how you determined that "anything that uses agent-base will emit the error"? And provide an example of how to reproduce this, or at least what your dependencies are?

Hi @eli-darkly There was a version conflict between dependencies that are using agent-base package. I think it's better to explicitly install it as a dependency of the sdk (agent-base seems to be a global package in node14)

corysimmons commented 1 year ago

Please downgrade back to whatever was working before (4 or 5?) and do a major bump. 🤦

eli-darkly commented 1 year ago

@corysimmons I'm not sure I understand what you're suggesting, or who you're suggesting it to.

corysimmons commented 1 year ago

I'm suggesting to the lib maintainers that it's a terrible UX to install this lib on an existing Next.js project (that might have some deps that use http.request or whatever the issue is) and run into this weird bug.

Downgrading to launchdarkly-node-server-sdk@5.14.5 solved the bug for me, so what's the difference between 5.14.5 and 6.x? Whatever it is, I think the maintainers should go back to what they were doing in previous versions, and make a major bump, so future users of LaunchDarkly can have a good experience.

eli-darkly commented 1 year ago

@corysimmons Here's some more detail on what the issue is.

Node's http.request function, like many Node APIs, can be called with several different kinds of parameter lists. You can call it with options alone, or with options and callback, or with url alone, or with url and options, or with all three. It figures out which parameter is which by checking the types.

The difference between the 5.x and 6.x versions of our SDKs in this regard is that previously our SSE client implementation was always calling request(options, callback), but now under some circumstances we use request(url, options, callback). The reason for this is that some valid properties of a URL are not valid in the options object. This change was made in https://github.com/launchdarkly/js-eventsource/pull/14. This allowed us to support the full range of HTTP options in Node while avoiding any incorrect API usage.

The problem is that some third-party middleware products work by monkey-patching http.request, but they do not do it correctly, so depending on which combination of parameters you use, the underlying http.request receives invalid parameters. And it's impossible for us to predict which kind of usage they will break, because that is completely up to whoever implemented the middleware. It could work correctly with (url, options, callback) but fail with (options, callback)or vice versa. So there is no single way we can write our code that will be resilient against all the ways other developers could break the API.

I agree that "it's a terrible UX to install this lib on an existing Next.js project (that might have some deps that use http.request or whatever the issue is) and run into this weird bug", but we are using Node APIs correctly and we cannot prevent bad behavior by third-party products— that's an inherent weakness of how easy Node.js makes it to modify fundamental APIs like http.request. It simply isn't a LaunchDarkly problem; some people happen to have been using LaunchDarkly along with these other products and so they reported it to LaunchDarkly, but if they'd been using some other web service client instead of LaunchDarkly, the odds are good that it would've broken those too. These are known bugs in particular versions of these particular products, such as Google Cloud Trace. The solution is not to downgrade LaunchDarkly, but to upgrade to non-broken versions of those products.

ColinGarner commented 1 year ago

I was also seeing this issue today. The dependency causing the issue wasn't mentioned in the thread, but I deleted my package-lock.json and reinstalled my dependencies and that resolved the issue for me.