Merge pull request #219 from codeoverflow-org/fix/217-persistence-manager-race-condition

Fix race condition in PersitsenceManager

Author: hlxid

nodecg-io-core/extension/index.ts

@@ -32,7 +32,7 @@ module.exports = (nodecg: NodeCG): NodeCGIOCore => {
         persistenceManager,
     ).registerMessageHandlers();
 
-    registerExitHandlers(nodecg, bundleManager, instanceManager, serviceManager);
+    registerExitHandlers(nodecg, bundleManager, instanceManager, serviceManager, persistenceManager);
 
     // We use a extra object instead of returning a object containing all the managers and so on, because
     // any loaded bundle would be able to call any (public or private) of the managers which is not intended.
@@ -62,7 +62,12 @@ function onExit(
     bundleManager: BundleManager,
     instanceManager: InstanceManager,
     serviceManager: ServiceManager,
+    persistenceManager: PersistenceManager,
 ): void {
+    // Save everything
+    // This is especially important if some services update some configs (e.g. updated tokens) and they haven't been saved yet.
+    persistenceManager.save();
+
     // Unset all service instances in all bundles
     const bundles = bundleManager.getBundleDependencies();
     for (const bundleName in bundles) {
@@ -99,9 +104,10 @@ function registerExitHandlers(
     bundleManager: BundleManager,
     instanceManager: InstanceManager,
     serviceManager: ServiceManager,
+    persistenceManager: PersistenceManager,
 ): void {
     const handler = () => {
-        onExit(nodecg, bundleManager, instanceManager, serviceManager);
+        onExit(nodecg, bundleManager, instanceManager, serviceManager, persistenceManager);
     };
 
     // Normal exit

nodecg-io-core/extension/instanceManager.ts

@@ -140,19 +140,30 @@ export class InstanceManager extends EventEmitter {
      *                   Should only be false if it has been validated at a previous point in time, e.g. loading after startup.
      * @return void if everything went fine and a string describing the issue if something went wrong.
      */
-    async updateInstanceConfig(instanceName: string, config: unknown, validation = true): Promise<Result<void>> {
+    updateInstanceConfig(instanceName: string, config: unknown, validation = true): Promise<Result<void>> {
         // Check existence and get service instance.
         const inst = this.serviceInstances[instanceName];
         if (inst === undefined) {
-            return error("Service instance doesn't exist.");
+            return Promise.resolve(error("Service instance doesn't exist."));
         }
 
         const service = this.services.getService(inst.serviceType);
         if (service.failed) {
-            return error("The service of this instance couldn't be found.");
+            return Promise.resolve(error("The service of this instance couldn't be found."));
         }
 
-        if (validation || !service.result.requiresNoConfig) {
+        // If we don't need validation, because we are loading the configuration from disk, we can set it directly
+        // so that after we return the promise from updateInstanceClient the PersistenceManager can be sure that the
+        // config has been written.
+        // Can also be used when there is no configuration needed so that we don't spawn another promise.
+        if (!validation || service.result.requiresNoConfig) {
+            inst.config = config;
+            this.emit("change");
+            return this.updateInstanceClient(inst, instanceName, service.result);
+        }
+
+        // We need to do validation, spawn a Promise
+        return (async () => {
             const schemaValid = this.ajv.validate(service.result.schema, config);
             if (!schemaValid) {
                 return error("Config invalid: " + this.ajv.errorsText());
@@ -170,16 +181,16 @@ export class InstanceManager extends EventEmitter {
                 );
                 return error("Config invalid: " + err);
             }
-        }
 
-        // All checks passed. Set config.
-        inst.config = config;
+            // All checks passed. Set config and save it.
+            inst.config = config;
+            this.emit("change");
 
-        // Update client of this instance using the new config.
-        const updateResult = await this.updateInstanceClient(inst, instanceName, service.result);
+            // Update client of this instance using the new config.
+            const updateResult = await this.updateInstanceClient(inst, instanceName, service.result);
 
-        this.emit("change");
-        return updateResult;
+            return updateResult;
+        })();
     }
 
     /**