DevCycleHQ / js-sdks

DevCycle - JS Based SDKs
https://docs.devcycle.com/
30 stars 5 forks source link

Event queue not fully cleaned up on close #774

Open walmor-rematter opened 4 months ago

walmor-rematter commented 4 months ago

I'm integrating our backend with @devcycle/nodejs-server-sdk and having a very particular issue related to hot reloading and needing to initialize the lib multiple times by calling initializeDevCycle.

We have a custom hot reload mechanism that does not shut down the process. To summarize, it simply rebuilds, clears caches, and restarts the express app on the fly. Thus, imported packages (node_modules) are not reloaded after changing a file, making it very fast. However, when the app restarts and calls the initializeDevCycle for the second time, we get this error:

Event Queue already exists for sdkKey: ${sdkKey} you can only initialize the DevCycle SDK once per sdkKey

We're calling devCycleClient.close() when the app shuts down but that doesn't solve the problem. Looking at the code, I can see that the close() method on the DevCycleClient calls the cleanup method on the EventQueue but that's not enough. I believe that a potential solution is to also call the cleanupEventQueue(sdkKey) method when closing the client, but I'm not sure if that has any drawbacks.

That's what I'm doing to workaround the problem for now, essentially this:

import { getBucketingLib } from '@devcycle/nodejs-server-sdk/src/bucketing';

export const closeFeatureFlagsClient = async () => {
  if (devCycleClient) {
    await devCycleClient.close();
  }

  getBucketingLib().cleanupEventQueue(DEV_CYCLE_SERVER_KEY);
};

It works, but I believe the cleanupEventQueue method was only intended to be used internally.

Another solution I can see is to add an option to the initializeDevCycle method that would allow us to continue normally even if the queue was already created, or something like that.

cdruxerman commented 4 months ago

@walmor-rematter I'll have our team look into this for you

cdruxerman commented 4 months ago

Also @walmor-rematter sorry for all the name and label changes. We just started trying out the github/linear sync and I wasn't expecting those

elliotCamblor commented 4 months ago

Hey @walmor-rematter! I believe the reason we don't fully cleanup the event queue on close is because there are cases where new events can be added to the queue after the flushEvents() call in the close method, but which get picked up in the flush that runs on an interval.

That being said, we've discussed in the past that we would like to be able initializing the sdk multiple times for use cases like your own. I'll bring it back to the team to discuss and we will keep you posted on how we decide to handle it!

In the meantime, if manually cleaning up the event queue is working for you then I would suggest going forward with that.

Thanks very much for your feedback and let me know if you have any other questions!

walmor-rematter commented 4 months ago

Hey @elliotCamblor,

Got it, well, if fully cleaning up the queue on close is not possible then, yes, allowing us to initialize the SDK multiple times would be very helpful.

I'll keep my workaround for now until we have a better solution.

Thank you for the quick reply!