From 71024bd389205492c88263ef0b8d126fca14ef67 Mon Sep 17 00:00:00 2001
From: Mike Gerwitz <gerwitzm@lovullo.com>
Date: Mon, 13 Feb 2017 13:36:31 -0500
Subject: [PATCH] DataValidator: Queue validations when incomplete

* src/validate/DataValidator.js (validate): If a validation is
    ongoing, queue requests.

* test/validate/DataValidatorTest.js: Add test.

DEV-2299
---
 src/validate/DataValidator.js      | 28 ++++++++++++-
 test/validate/DataValidatorTest.js | 65 ++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/src/validate/DataValidator.js b/src/validate/DataValidator.js
index 3690ac5..1a7a90b 100644
--- a/src/validate/DataValidator.js
+++ b/src/validate/DataValidator.js
@@ -61,6 +61,12 @@ module.exports = Class( 'DataValidator',
      */
     'private _stores': {},
 
+    /**
+     * Pending validation
+     * @type {Promise}
+     */
+    'private _pending': null,
+
 
     /**
      * Initialize validator
@@ -103,6 +109,10 @@ 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
+     * states and race conditions.
+     *
      * The external validator `validatef` is a kluge while the system
      * undergoes refactoring.
      *
@@ -116,6 +126,16 @@ module.exports = Class( 'DataValidator',
     {
         const _self = this;
 
+        // queue requests if we're already processing
+        if ( this._pending )
+        {
+            this._pending.then(
+                () => this.validate( diff, classes, validatef )
+            );
+
+            return this._pending;
+        }
+
         let failures = {};
 
         if ( diff !== undefined )
@@ -132,8 +152,12 @@ module.exports = Class( 'DataValidator',
         }
 
         // XXX: this assumes that the above is synchronous
-        return this._populateStore( classes, this._stores.cstore, 'indexes' )
-            .then( () => this.updateFailures( diff, failures ) );
+        return this._pending =
+            this._populateStore(
+                classes, this._stores.cstore, 'indexes'
+            )
+            .then( () => this.updateFailures( diff, failures ) )
+            .then( () => this._pending = null );
     },
 
 
diff --git a/test/validate/DataValidatorTest.js b/test/validate/DataValidatorTest.js
index e2364d3..272aab0 100644
--- a/test/validate/DataValidatorTest.js
+++ b/test/validate/DataValidatorTest.js
@@ -251,6 +251,71 @@ describe( 'DataValidator', () =>
                     expect( cleared.b && cleared.c ).to.be.true
                 );
         } ) );
+
+
+        // otherwise system might get into an unexpected state
+        it( 'queues concurrent validations', () =>
+        {
+            const expected_failure = {};
+
+            let vcalled = 0;
+
+            const bvalidator = createMockBucketValidator( function( _, __, ___ )
+            {
+                vcalled++;
+            } );
+
+            const vmonitor    = sinon.createStubInstance( ValidStateMonitor );
+            const dep_factory = createMockDependencyFactory();
+            const getStore    = createStubStore();
+
+            const diff_a = { foo: [ 'a', 'b', 'c' ] };
+            const diff_b = { foo: [ 'd' ] };
+
+            const validatef = ( diff, failures ) =>
+            {
+                // not a real failure; just used to transfer state to stub
+                // (see below)
+                failures.failedon = diff;
+            };
+
+            return new Promise( ( accept, reject ) =>
+            {
+                // by the time it gets to this the second time, store could
+                // be in any sort of state depending on what callbacks were
+                // invoked first (implementation details)
+                vmonitor.update = ( _, failures ) =>
+                {
+                    const orig_diff = failures.failedon;
+
+                    // if the external validator was called twice, then they
+                    // didn't wait for us to finish
+                    if ( ( orig_diff === diff_a ) && ( vcalled !== 1 ) )
+                    {
+                        reject( Error( "Request not queued" ) );
+                    }
+
+                    // if this key doesn't exist, then the store has been
+                    // cleared (which happens before it's re-populated with
+                    // the new diff)
+                    return expect( getStore().bstore.get( 'foo' ) )
+                        .to.eventually.deep.equal( orig_diff.foo )
+                        .then( () => {
+                            // the second test, after which we're done
+                            if ( orig_diff === diff_b )
+                            {
+                                accept();
+                            }
+                        } )
+                        .catch( e => reject( e ) );
+                };
+
+                const sut = Sut( bvalidator, vmonitor, dep_factory, getStore );
+
+                sut.validate( diff_a, {}, validatef );
+                sut.validate( diff_b, {}, validatef );
+            } );
+        } );
     } );