refactor: set change number only if add/removeSplits operations don't fail, for storage consistency

Author: EmilianoSanchez

.github/workflows/ci.yml

@@ -24,7 +24,7 @@ jobs:
       - name: Set up nodejs
         uses: actions/setup-node@v4
         with:
-          node-version: 'lts/*'
+          node-version: '22'
           cache: 'npm'
 
       - name: npm CI

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "@splitsoftware/splitio-commons",
-  "version": "1.17.2-rc.0",
+  "version": "1.17.2-rc.1",
   "description": "Split JavaScript SDK common components",
   "main": "cjs/index.js",
   "module": "esm/index.js",

package.json

@@ -18,7 +18,6 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync {
   private readonly storageHash: string;
   private readonly flagSetsFilter: string[];
   private hasSync?: boolean;
-  private updateNewFilter?: boolean;
 
   /**
    * @param {KeyBuilderCS} keys
@@ -102,39 +101,29 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync {
   }
 
   addSplit(name: string, split: ISplit) {
-    try {
-      const splitKey = this.keys.buildSplitKey(name);
-      const splitFromLocalStorage = localStorage.getItem(splitKey);
-      const previousSplit = splitFromLocalStorage ? JSON.parse(splitFromLocalStorage) : null;
+    const splitKey = this.keys.buildSplitKey(name);
+    const splitFromLocalStorage = localStorage.getItem(splitKey);
+    const previousSplit = splitFromLocalStorage ? JSON.parse(splitFromLocalStorage) : null;
 
-      localStorage.setItem(splitKey, JSON.stringify(split));
+    localStorage.setItem(splitKey, JSON.stringify(split));
 
-      this._incrementCounts(split);
-      this._decrementCounts(previousSplit);
+    this._incrementCounts(split);
+    this._decrementCounts(previousSplit);
 
-      // if (previousSplit) this.removeFromFlagSets(previousSplit.name, previousSplit.sets);
-      // this.addToFlagSets(split);
+    // if (previousSplit) this.removeFromFlagSets(previousSplit.name, previousSplit.sets);
+    // this.addToFlagSets(split);
 
-      return true;
-    } catch (e) {
-      this.log.error(LOG_PREFIX + e);
-      return false;
-    }
+    return true;
   }
 
   removeSplit(name: string): boolean {
-    try {
-      const split = this.getSplit(name);
-      localStorage.removeItem(this.keys.buildSplitKey(name));
+    const split = this.getSplit(name);
+    localStorage.removeItem(this.keys.buildSplitKey(name));
 
-      this._decrementCounts(split);
-      // if (split) this.removeFromFlagSets(split.name, split.sets);
+    this._decrementCounts(split);
+    // if (split) this.removeFromFlagSets(split.name, split.sets);
 
-      return true;
-    } catch (e) {
-      this.log.error(LOG_PREFIX + e);
-      return false;
-    }
+    return true;
   }
 
   getSplit(name: string) {
@@ -143,19 +132,6 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync {
   }
 
   setChangeNumber(changeNumber: number): boolean {
-
-    // when using a new split query, we must update it at the store
-    if (this.updateNewFilter) {
-      this.log.info(LOG_PREFIX + 'SDK key, flags filter criteria or flags spec version was modified. Updating cache');
-      const storageHashKey = this.keys.buildHashKey();
-      try {
-        localStorage.setItem(storageHashKey, this.storageHash);
-      } catch (e) {
-        this.log.error(LOG_PREFIX + e);
-      }
-      this.updateNewFilter = false;
-    }
-
     try {
       localStorage.setItem(this.keys.buildSplitsTillKey(), changeNumber + '');
       // update "last updated" timestamp with current time
@@ -246,12 +222,12 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync {
 
     if (storageHash !== this.storageHash) {
       try {
-        // mark cache to update the new query filter on first successful splits fetch
-        this.updateNewFilter = true;
-
         // if there is cache, clear it
         if (this.checkCache()) this.clear();
 
+        this.log.info(LOG_PREFIX + 'SDK key, flags filter criteria or flags spec version was modified. Updating cache');
+        localStorage.setItem(storageHashKey, this.storageHash);
+
       } catch (e) {
         this.log.error(LOG_PREFIX + e);
       }

src/storages/inLocalStorage/SplitsCacheInLocal.ts

@@ -129,8 +129,8 @@ export function splitChangesUpdaterFactory(
   }
 
   /** Returns true if at least one split was updated */
-  function isThereUpdate(flagsChange: [boolean | void, void | boolean[], void | boolean[], boolean | void] | [any, any, any]) {
-    const [, added, removed] = flagsChange;
+  function isThereUpdate(flagsChange: [void | boolean[], void | boolean[], boolean | void] | [any, any, any]) {
+    const [added, removed] = flagsChange;
     // There is at least one added or modified feature flag
     if (added && added.some((update: boolean) => update)) return true;
     // There is at least one removed feature flag
@@ -158,38 +158,40 @@ export function splitChangesUpdaterFactory(
         splitChangesFetcher(since, noCache, till, _promiseDecorator)
       )
         .then((splitChanges: ISplitChangesResponse) => {
-          startingUp = false;
-
           const mutation = computeSplitsMutation(splitChanges.splits, splitFiltersValidation);
 
           log.debug(SYNC_SPLITS_NEW, [mutation.added.length]);
           log.debug(SYNC_SPLITS_REMOVED, [mutation.removed.length]);
           log.debug(SYNC_SPLITS_SEGMENTS, [mutation.segments.length]);
 
           // Write into storage
-          // @TODO call `setChangeNumber` only if the other storage operations have succeeded, in order to keep storage consistency
-          return Promise.all([
-            // calling first `setChangenumber` method, to perform cache flush if split filter queryString changed
-            splits.setChangeNumber(splitChanges.till),
-            splits.addSplits(mutation.added),
-            splits.removeSplits(mutation.removed),
-            segments.registerSegments(mutation.segments)
-          ]).then((flagsChange) => {
-            if (splitsEventEmitter) {
-              // To emit SDK_SPLITS_ARRIVED for server-side SDK, we must check that all registered segments have been fetched
-              return Promise.resolve(!splitsEventEmitter.splitsArrived || (since !== splitChanges.till && isThereUpdate(flagsChange) && (isClientSide || checkAllSegmentsExist(segments))))
-                .catch(() => false /** noop. just to handle a possible `checkAllSegmentsExist` rejection, before emitting SDK event */)
-                .then(emitSplitsArrivedEvent => {
-                  // emit SDK events
-                  if (emitSplitsArrivedEvent) splitsEventEmitter.emit(SDK_SPLITS_ARRIVED);
-                  return true;
-                });
-            }
-            return true;
+          // Wrap into a promise to handle LOCALSTORAGE exceptions and REDIS rejected promises uniformly
+          return Promise.resolve().then(() => {
+            return Promise.all([
+              splits.addSplits(mutation.added),
+              splits.removeSplits(mutation.removed),
+              segments.registerSegments(mutation.segments)
+            ]).then((flagsChange) => {
+              splits.setChangeNumber(splitChanges.till);
+              startingUp = false;
+
+              if (splitsEventEmitter) {
+                // To emit SDK_SPLITS_ARRIVED for server-side SDK, we must check that all registered segments have been fetched
+                return Promise.resolve(!splitsEventEmitter.splitsArrived || (since !== splitChanges.till && isThereUpdate(flagsChange) && (isClientSide || checkAllSegmentsExist(segments))))
+                  .catch(() => false /** noop. just to handle a possible `checkAllSegmentsExist` rejection, before emitting SDK event */)
+                  .then(emitSplitsArrivedEvent => {
+                    // emit SDK events
+                    if (emitSplitsArrivedEvent) splitsEventEmitter.emit(SDK_SPLITS_ARRIVED);
+                    return true;
+                  });
+              }
+              return true;
+            });
           });
         })
         .catch(error => {
           log.warn(SYNC_SPLITS_FETCH_FAILS, [error]);
+          console.log('startingUp', startingUp, 'retriesOnFailureBeforeReady', retriesOnFailureBeforeReady, 'retry', retry);
 
           if (startingUp && retriesOnFailureBeforeReady > retry) {
             retry += 1;