launchdarkly / electron-client-sdk

LaunchDarkly Client-side SDK for Electron
Other
4 stars 9 forks source link

Async error from initializeInMain when the network is offline #19

Closed clemkoa closed 4 years ago

clemkoa commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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).

clemkoa commented 4 years ago

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

eli-darkly commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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