From ac7ec6c5f2f1aa6eec58667dd8de09c4ea2099f2 Mon Sep 17 00:00:00 2001
From: Mike Gerwitz <gerwitzm@lovullo.com>
Date: Thu, 16 Feb 2017 16:53:43 -0500
Subject: [PATCH] DataValidator: Hold concurrent requests

Since we maintain state (as a kluge for the time being to integrate
with the rest of the system), we need to be careful to protect against
concurrent requests that might mess with it before the original
request is complete.

* src/validate/DataValidator.js
  (validate, updateFailures): Hold concurrent requests.
  (_onceReady): Add method.

* test/validate/DataValidatorTest.js: Add tests.
---
 src/validate/DataValidator.js      | 90 ++++++++++++++++++++----------
 test/validate/DataValidatorTest.js | 36 ++++++++++++
 2 files changed, 98 insertions(+), 28 deletions(-)

diff --git a/src/validate/DataValidator.js b/src/validate/DataValidator.js
index 5b5c38b..56ae480 100644
--- a/src/validate/DataValidator.js
+++ b/src/validate/DataValidator.js
@@ -109,8 +109,8 @@ module.exports = Class( 'DataValidator',
     /**
      * Validate diff and update field monitor
      *
-     * If a validation is pending completion, all further `#validate`
-     * requests will be queued to prevent unexpected/inconsistent system
+     * If an operation is pending completion, all further requests to this
+     * object will be queued to prevent unexpected/inconsistent system
      * states and race conditions.
      *
      * The external validator `validatef` is a kluge while the system
@@ -126,44 +126,40 @@ module.exports = Class( 'DataValidator',
     {
         const _self = this;
 
-        // queue requests if we're already processing
-        if ( this._pending )
+        return this._onceReady( () =>
         {
-            this._pending.then(
-                () => this.validate( diff, classes, validatef )
-            );
+            let failures = {};
 
-            return this._pending;
-        }
-
-        let failures = {};
-
-        if ( diff !== undefined )
-        {
-            _self._bucket_validator.validate( diff, ( name, value, i ) =>
+            if ( diff !== undefined )
             {
-                diff[ name ][ i ] = undefined;
+                _self._bucket_validator.validate( diff, ( name, value, i ) =>
+                {
+                    diff[ name ][ i ] = undefined;
 
-                ( failures[ name ] = failures[ name ] || {} )[ i ] =
-                    _self._factory.createFieldFailure( name, i, value );
-            }, true );
+                    ( failures[ name ] = failures[ name ] || {} )[ i ] =
+                        _self._factory.createFieldFailure( name, i, value );
+                }, true );
 
-            validatef && validatef( diff, failures );
-        }
+                validatef && validatef( diff, failures );
+            }
 
-        // XXX: this assumes that the above is synchronous
-        return this._pending =
-            this._populateStore(
-                classes, this._stores.cstore, 'indexes'
-            )
-            .then( () => this.updateFailures( diff, failures ) )
-            .then( () => this._pending = null );
+            // XXX: this assumes that the above is synchronous
+            return this._pending =
+                this._populateStore(
+                    classes, this._stores.cstore, 'indexes'
+                )
+                .then( () => this._doUpdateFailures( diff, failures ) );
+        } );
     },
 
 
     /**
      * Update failures from external validation
      *
+     * If an operation is pending completion, all further requests to this
+     * object will be queued to prevent unexpected/inconsistent system
+     * states and race conditions.
+     *
      * TODO: This is a transitional API---we should handle all validations,
      * not allow external systems to meddle in our affairs.
      *
@@ -173,6 +169,23 @@ module.exports = Class( 'DataValidator',
      * @return {Promise} promise to populate internal store
      */
     'public updateFailures'( diff, failures )
+    {
+        return this._onceReady( () =>
+        {
+            return this._doUpdateFailures( diff, failures );
+        } );
+    },
+
+
+    /**
+     * Update failures from external validation
+     *
+     * @param {Object} diff     bucket diff
+     * @param {Object} failures failures per field name and index
+     *
+     * @return {Promise} promise to populate internal store
+     */
+    'private _doUpdateFailures': function( diff, failures )
     {
         return this._populateStore( diff, this._stores.bstore ).then( () =>
             this._field_monitor.update(
@@ -198,10 +211,31 @@ module.exports = Class( 'DataValidator',
     'public clearFailures'( failures )
     {
         this._field_monitor.clearFailures( failures );
+
         return this;
     },
 
 
+    /**
+     * Wait until all requests are complete and then trigger callback
+     *
+     * @param {Function} callback callback to trigger when ready
+     *
+     * @return {Promise}
+     */
+    'private _onceReady': function( callback )
+    {
+        if ( this._pending )
+        {
+            this._pending.then( callback );
+            return this._pending;
+        }
+
+        return this._pending = callback()
+            .then( () => this._pending = null );
+    },
+
+
     /**
      * Populate store with data
      *
diff --git a/test/validate/DataValidatorTest.js b/test/validate/DataValidatorTest.js
index e209a13..ed9a932 100644
--- a/test/validate/DataValidatorTest.js
+++ b/test/validate/DataValidatorTest.js
@@ -316,6 +316,42 @@ describe( 'DataValidator', () =>
                     );
                 } );
         } );
+
+
+        it( 'queues concurrent requests', () =>
+        {
+            const { sut, vmonitor, getStore } = createStubs( {
+                vmonitor: sinon.createStubInstance( ValidStateMonitor ),
+            } );
+            const { bstore } = getStore();
+
+            const faila = {};
+            const failb = {};
+
+            let running_first = true;
+
+            vmonitor.update = ( _, fail ) =>
+            {
+                if ( fail === failb )
+                {
+                    if ( running_first === true )
+                    {
+                        return Promise.reject( Error(
+                            "Request not queued"
+                        ) );
+                    }
+                }
+
+                return Promise.resolve( true );
+            };
+
+            return Promise.all( [
+                sut.updateFailures( {}, faila )
+                    .then( () => running_first = false ),
+
+                sut.updateFailures( {}, failb ),
+            ] );
+        } );
     } );