Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Async error from initializeInMain when the network is offline #19

Closed
clemkoa opened this issue Mar 4, 2020 · 6 comments
Closed

Async error from initializeInMain when the network is offline #19

clemkoa opened this issue Mar 4, 2020 · 6 comments

Comments

@clemkoa
Copy link

clemkoa commented Mar 4, 2020

Describe the bug
When the computer is offline, calling initializeInMain fron the main process will throw an error. Trying to catch the error with something like this doesn't work:

try {
  const LDElectron = require("launchdarkly-electron-client-sdk");
  ldclient = LDElectron.initializeInMain(ld_client_id, lduser, ldoptions);
} catch (e) {
  // stuff
}

The error seems to be from a promise inside the initializeInMain code.

To reproduce
Run this from a computer with no network connection:

  const LDElectron = require("launchdarkly-electron-client-sdk");
  ldclient = LDElectron.initializeInMain(ld_client_id, lduser, ldoptions);

Expected behavior
A sync error would be acceptable, I think it is weird to have an async error.

Logs

[LaunchDarkly] network error (Error: getaddrinfo ENOTFOUND app.launchdarkly.com)
(node:49179) UnhandledPromiseRejectionWarning: LaunchDarklyFlagFetchError: network error (Error: getaddrinfo ENOTFOUND app.launchdarkly.com)
    at /Users/clementjoudet/dev/rastrum-agent/ui/node_modules/launchdarkly-js-sdk-common/dist/ldclient-common.cjs.js:1:20943
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
(node:49179) UnhandledPromiseRejectionWarning: LaunchDarklyFlagFetchError: network error (Error: getaddrinfo ENOTFOUND app.launchdarkly.com)
    at /Users/clementjoudet/dev/rastrum-agent/ui/node_modules/launchdarkly-js-sdk-common/dist/ldclient-common.cjs.js:1:20943
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
(node:49179) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:49179) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:49179) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:49179) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

SDK version
1.4.3

Language version, developer tools
node v13.7.0

OS/platform
MacOS

Additional context
N/A

@eli-darkly
Copy link
Contributor

Thanks for reporting this. I was able to reproduce it but I'm not yet sure how it's happening— we are not supposed to be allowing any uncaught promise rejections, since they are a major no-no that in some configurations can cause the process to exit. Also not yet sure if this is specific to the Electron SDK, or if it is an issue with our JS-based client-side SDKs in general. Continuing to investigate.

@eli-darkly
Copy link
Contributor

eli-darkly commented Mar 5, 2020

We should have a patch out shortly, but just to clarify something: at some point in your startup code, you should wait for the client to finish initializing, because until that happens you will not get valid feature flag values from it. I believe you'll find that the following code does allow you to catch the error:

    ldclient = LDElectron.initializeInMain(ld_client_id, lduser, ldoptions);
    try {
        await ldclient.waitForInitialization();
        // initialization succeeded
    } catch (err) {
        // initialization failed
    }

If you're not using async/await, this would be the equivalent with Promise handlers:

    ldclient = LDElectron.initializeInMain(ld_client_id, lduser, ldoptions);

    ldclient.waitForInitialization().then(() => {
        // initialization succeeded
    }).catch(err => {
        // initialization failed
    });

Failure of initialization is reported in three ways: an "error" event which you can listen for, a "ready" event which is triggered for failure or success, and a rejection of the waitForInitialization() promise. The bug that you've found is that if you do not call waitForInitialization(), that promise still exists and will cause an unhandled rejection. The fix will be for the SDK to lazily create that promise, so that if you choose to listen for events instead (or, although this isn't recommended, not listen for anything at all) there will not be any unhandled rejections. But if you do call waitForInitialization() you'll need to be sure to handle the error case (even if you don't do anything special in that case).

LaunchDarklyCI pushed a commit that referenced this issue Mar 7, 2020
update js-sdk-common and js-client-sdk to get event payload ID fix
@clemkoa
Copy link
Author

clemkoa commented Mar 8, 2020

Thanks for the feedback. I am already using the ldclient.on("ready") event, is there another event to listen for the error?

@eli-darkly
Copy link
Contributor

@clemkoa In Node, we support a few different mechanisms for this. The events you can listen for are described here, and the Promise-based methods are described here and here.

Normally, listening for the "error" event (or "failed", which would mean it's an error that has stopped the client from starting up at all) would be a perfectly fine way to detect errors, if you prefer to use events. But what I was getting at in my previous comment is that there is currently a bug that causes errors to propagate as unhandled Promise rejections if you do not use waitForInitialization(). So the workaround, until we release a patch for this (which I'm trying to get out as soon as possible; we're just having a problem with the CI build at the moment), is to do something like this:

    client.waitForInitialization.then(() => {
        // do whatever you want to do if startup has succeeded
    }).catch(err => {
        // do whatever you want to do if startup has failed
    });

It's functionally equivalent to listening for an event, since you have to provide a callback either way.

@eli-darkly
Copy link
Contributor

@clemkoa OK, we've released v1.5.1 with a fix for this, so you should be able to just keep doing what you were doing before rather than using the workaround I mentioned. Please let me know once you get a chance to try it out.

@clemkoa
Copy link
Author

clemkoa commented Mar 10, 2020

I tested it and it works! Thanks for being so responsive!

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

No branches or pull requests

2 participants