fix: Only initailize the SDK once. #368

ivarconr · 2022-09-26T18:59:51Z

About the changes

Before this fix all calls to initialize would create a new SDK instance, all maintaining the connection towards the Unleash API. We have observed this causing accidental DDosing of the Unleash API.

This fix will make sure that the SDK is only initialized once (aka singleton) when using the initialize method.

Discussion points

Unsure if we should warn when the Unleash SDK has been initialized more than 10 times.

ivarconr · 2022-09-26T19:00:26Z

src/unleash.ts

+    Unleash.instance = this;
+  }
+
+  static getInstance(config: UnleashConfig) {


Introducing the singleton pattern here.

coveralls · 2022-09-26T19:01:06Z

Coverage: 92.093% (-0.2%) from 92.25% when pulling 985b9d7 on fix/initalize-once into 52e3c41 on main.

ivarconr · 2022-09-26T19:02:15Z

src/index.ts

-  }
-  instance = new Unleash(options);
-  instance.on('error', () => {});
+  instance = Unleash.getInstance(options);


this function was never intended to be used to create multiple unleash SDK instances. The old (replaced) implementation is quite strange, as they keep the instances around (in-memory) but all functions will always be moved to the fresh instance. To me this smells like a bug.

thomasheartman

Nice work and good call 🙌🏼 I've got a couple questions regarding how we should use the singleton inline plus some minor suggestions on wordings.

README.md

thomasheartman · 2022-09-27T07:55:49Z

README.md

@@ -67,7 +67,7 @@ const unleash = new Unleash({

 unleash.on('ready', console.log.bind(console, 'ready'));

-// required error handling when using unleash directly
+// optional error handling when using unleash directly


Oh, this is nice 😄

thomasheartman · 2022-09-27T08:03:54Z

src/unleash.ts

+    if(Unleash.instance) {
+      return Unleash.instance;
+    } 


If you call getInstance with a different config, does it make sense to update the config for the instance? Or is that just confusing? It feels a little misleading that you can pass in the config as an argument, but that it does nothing. That said, I'm not sure there is a better way? Could we make it parameterless somehow and have you pass config in other ways? 🤔

Good question. I think that you are spot on the getInstance() method in the singleton pattern usually does not take arguments (options). I am a bit unsure how/where to communicate the options in a clear and concise way.

Yeah, good point. If you remove the need completely and use default values, then I imagine almost everyone will just not configure the instance at all.

Without looking at the docs, I would probably read this as a multiton implementation. What about something like:

static buildInstance(options?: UnleashConfig){ if(!options){ Unleash.singletonConfig = makeDefaultOptions(); }else { Unleash.singletonConfig = options } } static getInstance() { if(!Unleash.singletonConfig){ throw new Error("You gotta call buildInstance first") } //setup and/or return instance }

you always makes me learn something new @sighphyre, thanks!

README.md

thomasheartman · 2022-09-27T13:53:39Z

src/unleash.ts

+    if(Unleash.instance) {
+      return Unleash.instance;
+    } 


Yeah, good point. If you remove the need completely and use default values, then I imagine almost everyone will just not configure the instance at all.

thomasheartman · 2022-09-27T13:56:40Z

src/unleash.ts

+    if(Unleash.instanceCount > 10) {
+      console.warn('The unleash SDK has been initialized more than 10 times')
+    }
+


Regarding whether we should warn or not. I'm into the idea, but maybe make it configurable? For instance, add a disableInstanceCountWarning variable that can turn it off or something?

Also, should it be console.warn or a configurable logger? I don't know how you usually do that in Node, but I'd expect to be able to redirect warnings to where I'd want them, I think. Also, if it's a configurable logger (or just a logWarning: (warning: string) => void function, then you could pass () => {} as an argument and disable the logging that way 🤷🏼

Good point. We currently do not support a log abstraction, and I think it would be more work to get that in than I am willing to do for this change. We do have the event emitter, but the purpose here is to want users of the SDK that something is off.

A suitable middle-ground would be to add the option to disable the warning entirely, and log it as an error to the event emitter, which default will console.error it, and the user can override that by registering an on("error") handler.

I think it would be more work to get that in than I am willing to do for this change

Aye, I agree with this.

A suitable middle-ground would be to add the option to disable the warning entirely, and log it as an error to the event emitter, which default will console.error it, and the user can override that by registering an on("error") handler.

Just to make sure I understand this: does it only log to the event emitter if you disable the warning, or does it log to the event emitter regardless? And do we have support "warning" events? This isn't an error, so logging it as such feels a little icky to me.

Just to make sure I understand this: does it only log to the event emitter if you disable the warning

No, the plan is then to skip the error entirely,

thomasheartman · 2022-11-01T13:59:38Z

It's been a little while: are we still working on this PR or should we close it or otherwise?

stale · 2022-12-01T16:56:40Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Before this fix all calls to `initalize` would create a new SDK instance, all maintaining the connection towards the Unleash API. We have observed this causing accidental DDosing of the Unleash API. This fix will make sure that the SDK is only initalized once (aka singleton) when using the `initalize` method.

Co-authored-by: Thomas Heartman <thomas@getunleash.ai>

examples/simple_usage.js

ivarconr · 2023-01-11T07:54:25Z

src/unleash.ts

+    Unleash.instanceCount++;
+
+    this.on(UnleashEvents.Error, (err) => {
+      console.error(err?.message);


we need to check if there are other event handlers registered and only console.error if no others have registered.

ivarconr · 2023-01-11T07:55:04Z

src/unleash.ts

+    });
+
+    if(!skipInstanceCountWarning && Unleash.instanceCount > 10) {
+      const error = new Error('The unleash SDK has been initialized more than 10 times');


need to wrap this in a process.nextTick() to ensure using code have a chance to register an event listener before it triggers.

ivarconr · 2023-01-11T08:47:47Z

src/unleash.ts

+
+  private static instance?: Unleash;
+
+  private static instanceCount: 0;


Oh, this is a very nice bug, I accidentally set the type to "0" which is definetley not what I tried to do here!

sighphyre

Nice, looks good to me

sighphyre · 2023-01-11T10:36:42Z

src/unleash.ts

+  static getInstance(config: UnleashConfig) {
+    const configSignature = generateHashOfObject(config);
+    if(Unleash.instance) {
+      if(configSignature !== Unleash.configSignature) {


This is pretty elegant, I like it

ivarconr requested review from sighphyre, thomasheartman and chriswk September 26, 2022 18:59

ivarconr commented Sep 26, 2022

View reviewed changes

thomasheartman requested changes Sep 27, 2022

View reviewed changes

thomasheartman reviewed Sep 27, 2022

View reviewed changes

stale bot added the stale label Dec 1, 2022

stale bot closed this Dec 11, 2022

ivarconr reopened this Jan 2, 2023

stale bot removed the stale label Jan 2, 2023

ivarconr and others added 4 commits January 10, 2023 16:23

Update README.md

8b759a5

Co-authored-by: Thomas Heartman <thomas@getunleash.ai>

Update README.md

f472660

Co-authored-by: Thomas Heartman <thomas@getunleash.ai>

Update README.md

32097da

Co-authored-by: Thomas Heartman <thomas@getunleash.ai>

ivarconr force-pushed the fix/initalize-once branch from c14f215 to 32097da Compare January 10, 2023 15:25

ivarconr added 2 commits January 10, 2023 16:37

fix: make instance count warning skippable

35ed017

fix: add unit test

5c476ac

ivarconr commented Jan 11, 2023

View reviewed changes

examples/simple_usage.js Outdated Show resolved Hide resolved

ivarconr commented Jan 11, 2023

View reviewed changes

ivarconr added 3 commits January 11, 2023 10:23

fix: more tests

3671c70

fix: add an instanceCount max test

9c05d89

fix: cleanup example

985b9d7

sighphyre approved these changes Jan 11, 2023

View reviewed changes

ivarconr requested a review from thomasheartman January 11, 2023 11:13

ivarconr merged commit bff4ad0 into main Jan 11, 2023

ivarconr deleted the fix/initalize-once branch January 11, 2023 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Only initailize the SDK once. #368

fix: Only initailize the SDK once. #368

ivarconr commented Sep 26, 2022

ivarconr Sep 26, 2022

coveralls commented Sep 26, 2022 •

edited

ivarconr Sep 26, 2022

thomasheartman left a comment

thomasheartman Sep 27, 2022

thomasheartman Sep 27, 2022

ivarconr Sep 27, 2022

thomasheartman Sep 27, 2022

sighphyre Oct 5, 2022 •

edited

ivarconr Oct 11, 2022

thomasheartman Sep 27, 2022

thomasheartman Sep 27, 2022

ivarconr Sep 28, 2022

thomasheartman Oct 3, 2022

ivarconr Jan 10, 2023

thomasheartman commented Nov 1, 2022

stale bot commented Dec 1, 2022

ivarconr Jan 11, 2023

ivarconr Jan 11, 2023

ivarconr Jan 11, 2023

sighphyre left a comment

sighphyre Jan 11, 2023


		private static instance?: Unleash;

		private static instanceCount: 0;

fix: Only initailize the SDK once. #368

fix: Only initailize the SDK once. #368

Conversation

ivarconr commented Sep 26, 2022

About the changes

Discussion points

Choose a reason for hiding this comment

coveralls commented Sep 26, 2022 • edited

Choose a reason for hiding this comment

thomasheartman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighphyre Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasheartman commented Nov 1, 2022

stale bot commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 26, 2022 •

edited

sighphyre Oct 5, 2022 •

edited