ERR_INVALID_ARG_TYPE for event source in sdk

[icarus-sullivan]

Is this a support request?
No

Describe the bug
In version 6.2.2 of the sdk, there was a dependency upgrade for the launchdarkly-eventsource from 1.4.1 -> 1.4.2. When running this in node 12 I get the following errors when the sdk initializes an event source:
{"error":{"code":"ERR_INVALID_ARG_TYPE"}, ...omitted }

To reproduce
Use the sdk version 6.2.2 and initialize the client without any options in a node12 environment.

Expected behavior
I would expect to see issues with user-agent and http calls being done by the eventsource lib

Logs
Cannot provide due to HIPAA

SDK version
6.2.2+

Language version, developer tools
Node v12.22.11

OS/platform
Mac

Additional context
Depending on how stable the launchdarkly-eventsource@1.4.4 is, it might just be a simple upgrade for that version.

[eli-darkly]

This sounds like it could be a similar type of issue to #233. Is there any chance that you are using a third-party package that modifies the behavior of http.request or https.request?

If it is a related type of issue, then no, I would not expect updating the launchdarkly-eventsource version to have any effect. The relevant change was made in 1.4.2 and that logic has not changed since then.

[icarus-sullivan]

@eli-darkly First off, thanks for the quick response! I wonder if a third-party package mutating those http(s) modules would be the culprit as downgrading to 6.2.1 it doesn't cause an issue. I looked through the eventsource code for any gotcha I could find and I figured it had to be something with the user-agent or http(s) modules since I've experienced issues launching an http agent in an https setting.

#233 looks identical to what I was seeing. Apologies for double posting, should have looked first.

I know libraries should have as minimal deps as possible but maybe pulling in axios or node-fetch to make these requests might help alleviate any request issues?

[eli-darkly]

I know libraries should have as minimal deps as possible but maybe pulling in axios or node-fetch to make these requests might help alleviate any request issues?

I'm not sure I understand your suggestion. http and https are standard Node modules; node-fetch is just a wrapper around those modules that provides a different interface to them. If there is a third-party module interfering with http.request and https.request, then I would expect node-fetch to be affected as well.

Again, I don't know for sure that that is what's happening here. But if it is... there's really nothing we can do to guarantee that requests will work if there is a badly-behaved third-party module that is monkey-patching things in a problematic way. In the similar cases that we've seen before, the issue seemed to be that the third-party module made too many assumptions about how request is called— like, of the following possible usages, all of which are correct in Node, it only allowed some of them and not others:

• request(url)
• request(url, callback)
• request(options)
• request(options, callback)
• request(url, options)
• request(url, options, callback)

Prior to version 6.2.2, we were always using request(options, callback)— except that the way we were setting options was slightly wrong. As part of the fix for that, the logic also changed so that we may call request(url, options, callback) instead. Again, that is valid in Node. If some badly-behaved module is interfering with calls of that form, then that is not a problem specific to LaunchDarkly— it will affect any code that uses request in that way— and, it wouldn't make sense for us to change our code to always use (options, callback) instead, because the fact that it's specifically (url, options, callback) that doesn't work is just an implementation detail of that particular badly-behaved module; there could be some other one that breaks things in the opposite way.

[eli-darkly]

It might be helpful if you could provide a list of your project's package dependencies.

[icarus-sullivan]

The reason I brought up those libraries is because if there is a disparate way they make requests to http-https they have probably already solved the issue. The suggestion was more of a pragmatic idea vs a slight of how you're currently making a request. I'll see if I can't get a list out by tonight for you though.

[eli-darkly]

The reason I brought up those libraries is because if there is a disparate way they make requests to http-https they have probably already solved the issue

If you look at the source code of node-fetch (here and here), it is calling http.request or http.request with the parameters (url, options). That is also what we do (except in the case where we've been configured with a web proxy, in which case everything must be in options and there is no url, but you didn't mention using a proxy).

I don't see any reason to think that "they have probably already solved the issue", because there isn't a single issue— there is no one right way to call request that will definitely avoid being broken by badly-behaved modules. Some might break one way, others might break another. I don't see any special code or comments in node-fetch regarding this, and I'm not surprised, because again there is no way to make Node code foolproof against monkey-patching.

If I had a strong reason to think node-fetch would work differently, then it might be worth trying, but otherwise I really do not want to rewrite the library (and add a dependency) and only then find out if it does any good— especially since so far I have no way to reproduce the problem you're seeing, so I would have no way to know if it's been fixed.

[eli-darkly]

I haven't ruled out the possibility that there is some other problem that might require a fix in launchdarkly-eventsource. But right now I have virtually nothing to go on, without a reproducible test case. The dependency list may help... but if it turns out that the issue is interference by another module (like for instance the problem with old versions of Google Cloud Trace in the linked issue), then that would need to be addressed by the authors of that module.

[corysimmons]

Prior to version 6.2.2, we were always using request(options, callback)— except that the way we were setting options was slightly wrong. As part of the fix for that, the logic also changed so that we may call request(url, options, callback) instead. Again, that is valid in Node. If some badly-behaved module is interfering with calls of that form, then that is not a problem specific to LaunchDarkly— it will affect any code that uses request in that way— and, it wouldn't make sense for us to change our code to always use (options, callback) instead, because the fact that it's specifically (url, options, callback) that doesn't work is just an implementation detail of that particular badly-behaved module; there could be some other one that breaks things in the opposite way.

Thanks for the context @eli-darkly

For posterity, if you're stuck on an old project and can't upgrade deps, downgrade to

launchdarkly-node-server-sdk@6.2.1

If you're on a newer project, try upgrading all your other deps with something like https://npm.im/npm-check-updates

If that doesn't work, you may be able to track down a bad package by grepping node_modules for request( and then posting all those dependencies here for a LaunchDarkly staff member to investigate further. Likely whoever the culprit is won't upgrade the pkg in your time frame, so try replacing it with another pkg that does the same thing.

Again, if nothing works, just downgrade to 6.2.1 🙃

[icarus-sullivan]

We upgraded to node 16 and the problem went away. Since ticket #223 is open you can probably close this one. Appreciate the help guys.

[corysimmons]

Hmm, we've been on 16 and it doesn't work for us. 🤷

[eli-darkly]

@icarus-sullivan What issue did you mean to link to when you said "ticket 223"? There's no open issue with that number; GitHub has linked it to a pull request for an old release.

[github-actions]

This issue is marked as stale because it has been open for 30 days without activity. Remove the stale label or comment, or this will be closed in 7 days.