Make blockRequestUntil work with MaxSimultaneousRequests and handle e… (#35)

* Make blockRequestUntil work with MaxSimultaneousRequests and handle errors

CHANGELOG.md

@@ -0,0 +1,14 @@
+# Changelog
+All notable changes to this project will be documented in this file.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
+
+## [0.2.0] - 2018-08-29
+### Added
+- Unit tests verifying queueing/blocking requests and executing them in order
+
+### Changed
+- `GenericRestClient._blockRequestUntil` is now called every time when the request is on top of the pending requests queue rather than only once 
+- If `GenericRestClient._blockRequestUntil` rejects, the whole request is properly rejected
+- All the `assert`s now properly clear the request queues before throwing 
+

package.json

@@ -1,6 +1,6 @@
 {
   "name": "simplerestclients",
-  "version": "0.1.8",
+  "version": "0.2.0",
   "description": "A library of components for accessing RESTful services with javascript/typescript.",
   "author": "David de Regt <David.de.Regt@microsoft.com>",
   "scripts": {
@@ -9,7 +9,8 @@
     "build": "npm run tslint && tsc",
     "tslint": "tslint --project tsconfig.json -r tslint.json --fix || true",
     "test": "npm run clean && karma start --singleRun",
-    "test:watch": "npm run clean && karma start"
+    "test:watch": "npm run clean && karma start",
+    "test:browser": "npm run clean && karma start --browsers=Chrome --single-run=false --auto-watch"
   },
   "dependencies": {
     "@types/lodash": "^4.14.110",

src/GenericRestClient.ts

@@ -50,18 +50,7 @@ export class GenericRestClient {
         if (objToPost) {
             options.sendData = objToPost;
         }
-
-        const promise = this._blockRequestUntil(options);
-        if (!promise) {
-            return this._performApiCallInternal(apiPath, action, options);
-        }
-
-        return promise.then(() => this._performApiCallInternal(apiPath, action, options));
-    }
-
-    private _performApiCallInternal<T>(apiPath: string, action: HttpAction, options: ApiCallOptions)
-            : SyncTasks.Promise<WebResponse<T, ApiCallOptions>> {
-
+        
         if (options.eTag) {
             if (!options.augmentHeaders) {
                 options.augmentHeaders = {};
@@ -75,7 +64,8 @@ export class GenericRestClient {
 
         const finalUrl = options.excludeEndpointUrl ? apiPath : this._endpointUrl + apiPath;
 
-        return new SimpleWebRequest<T, ApiCallOptions>(action, finalUrl, options, () => this._getHeaders(options))
+        return new SimpleWebRequest<T, ApiCallOptions>(action, finalUrl, options, () => this._getHeaders(options), 
+                () => this._blockRequestUntil(options))
             .start()
             .then(response => {
                 this._processSuccessResponse<T>(response);
@@ -89,6 +79,7 @@ export class GenericRestClient {
     }
 
     // Override (but make sure to call super and chain appropriately) this function if you want to add more blocking criteria.
+    // Also, this might be called multiple times to check if the conditions changed
     protected _blockRequestUntil(options: ApiCallOptions): SyncTasks.Promise<void>|undefined {
         // No-op by default
         return undefined;

src/SimpleWebRequest.ts

@@ -189,6 +189,7 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
     protected _aborted = false;
     protected _timedOut = false;
     protected _paused = false;
+    protected _created = Date.now();
 
     // De-dupe result handling for two reasons so far:
     // 1. Various platforms have bugs where they double-resolves aborted xmlhttprequests
@@ -200,7 +201,8 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
 
     constructor(protected _action: string,
             protected _url: string, options: TOptions,
-            protected _getHeaders?: () => Headers) {
+            protected _getHeaders?: () => Headers,
+            protected _blockRequestUntil?: () => SyncTasks.Promise<void>|undefined) {
         this._options = _.defaults(options, DefaultOptions);
     }
 
@@ -213,8 +215,35 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
     protected static checkQueueProcessing() {
         while (requestQueue.length > 0 && executingList.length < SimpleWebRequestOptions.MaxSimultaneousRequests) {
             const req = requestQueue.shift()!!!;
-            executingList.push(req);
-            req._fire();
+            const blockPromise = (req._blockRequestUntil && req._blockRequestUntil()) || SyncTasks.Resolved();
+            blockPromise.then(() => {
+                if (executingList.length < SimpleWebRequestOptions.MaxSimultaneousRequests) {
+                    executingList.push(req);
+                    req._fire();
+                } else {
+                    req._enqueue();
+                }
+            }, err => {
+                // fail the request if the block promise is rejected
+                req._respond('_blockRequestUntil rejected: ' + err);
+            });
+        }
+    }
+
+    protected _removeFromQueue(): void {
+        // Pull it out of whichever queue it's sitting in
+        if (this._xhr) {
+            _.pull(executingList, this);
+        } else {
+            _.pull(requestQueue, this);
+        }
+    }
+
+    protected _assertAndClean(expression: any, message: string): void {
+        if (!expression) {
+            this._removeFromQueue();
+            console.error(message);
+            assert.ok(expression, message);
         }
     }
 
@@ -240,7 +269,7 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
             const timeoutSupported = timeoutSupportStatus;
              // Use manual timer if we don't know about timeout support
             if (timeoutSupported !== FeatureSupportStatus.Supported) {
-                assert.ok(!this._requestTimeoutTimer, 'Double-fired requestTimeoutTimer');
+                this._assertAndClean(!this._requestTimeoutTimer, 'Double-fired requestTimeoutTimer');
                 this._requestTimeoutTimer = SimpleWebRequestOptions.setTimeout(() => {
                     this._requestTimeoutTimer = undefined;
 
@@ -262,7 +291,7 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
                     this._timedOut = true;
                     // Set aborted flag to match simple timer approach, which aborts the request and results in an _respond call
                     this._aborted = true;
-                    this._respond();
+                    this._respond('TimedOut');
                 };
             }
         }
@@ -325,7 +354,7 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
             // so make sure we know that this is an abort.
             this._aborted = true;
 
-            this._respond();
+            this._respond('Aborted');
         };
 
         if (this._xhr.upload && this._options.onProgress) {
@@ -344,14 +373,14 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
         _.forEach(nextHeaders, (val, key) => {
             const headerLower = key.toLowerCase();
             if (headerLower === 'content-type') {
-                assert.ok(false, 'Don\'t set Content-Type with options.headers -- use it with the options.contentType property');
+                this._assertAndClean(false, 'Don\'t set Content-Type with options.headers -- use it with the options.contentType property');
                 return;
             }
             if (headerLower === 'accept') {
-                assert.ok(false, 'Don\'t set Accept with options.headers -- use it with the options.acceptType property');
+                this._assertAndClean(false, 'Don\'t set Accept with options.headers -- use it with the options.acceptType property');
                 return;
             }
-            assert.ok(!headersCheck[headerLower], 'Setting duplicate header key: ' + headersCheck[headerLower] + ' and ' + key);
+            this._assertAndClean(!headersCheck[headerLower], 'Setting duplicate header key: ' + headersCheck[headerLower] + ' and ' + key);
 
             if (val === undefined || val === null) {
                 console.warn('Tried to set header "' + key + '" on request with "' + val + '" value, header will be dropped');
@@ -499,11 +528,16 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
 
         // Throw it on the queue
         const index = _.findIndex(requestQueue, request =>
-            request.getPriority() < (this._options.priority || WebRequestPriority.DontCare));
+            // find a request with the same priority, but newer
+            (request.getPriority() === this.getPriority() && request._created > this._created) ||
+            // or a request with lower priority
+            (request.getPriority() < this.getPriority()));
 
         if (index > -1) {
+            //add me before the found request
             requestQueue.splice(index, 0, this);
         } else {
+            //add me at the end
             requestQueue.push(this);
         }
 
@@ -534,8 +568,9 @@ export class SimpleWebRequest<TBody, TOptions extends WebRequestOptions = WebReq
 
     private _deferred: SyncTasks.Deferred<WebResponse<TBody, TOptions>>;
 
-    constructor(action: string, url: string, options: TOptions, getHeaders?: () => Headers) {
-        super(action, url, options, getHeaders);
+    constructor(action: string, url: string, options: TOptions, getHeaders?: () => Headers,
+            blockRequestUntil?: () => SyncTasks.Promise<void>|undefined) {
+        super(action, url, options, getHeaders, blockRequestUntil);
     }
 
     abort(): void {
@@ -562,7 +597,7 @@ export class SimpleWebRequest<TBody, TOptions extends WebRequestOptions = WebReq
         }
 
         // Cannot rely on this._xhr.abort() to trigger this._xhr.onAbort() synchronously, thus we must trigger an early response here
-        this._respond();
+        this._respond('Aborted');
 
         if (this._xhr) {
             // Abort the in-flight request
@@ -599,12 +634,7 @@ export class SimpleWebRequest<TBody, TOptions extends WebRequestOptions = WebReq
 
         this._finishHandled = true;
 
-        // Pull it out of whichever queue it's sitting in
-        if (this._xhr) {
-            _.pull(executingList, this as SimpleWebRequestBase);
-        } else {
-            _.pull(requestQueue, this as SimpleWebRequestBase);
-        }
+        this._removeFromQueue();
 
         if (this._retryTimer) {
             SimpleWebRequestOptions.clearTimeout(this._retryTimer);
@@ -626,7 +656,7 @@ export class SimpleWebRequest<TBody, TOptions extends WebRequestOptions = WebReq
                 // Some browsers error when you try to read status off aborted requests
             }
         } else {
-            statusText = 'Browser Error - Possible CORS or Connectivity Issue';
+            statusText = errorStatusText || 'Browser Error - Possible CORS or Connectivity Issue';
         }
 
         let headers: Headers = {};

test/GenericRestClient.spec.ts

@@ -1,6 +1,7 @@
 import * as faker from 'faker';
 import { GenericRestClient, ApiCallOptions } from '../src/GenericRestClient';
 import { DETAILED_RESPONSE, REQUEST_OPTIONS } from './helpers';
+import * as SyncTasks from 'synctasks';
 
 class RestClient extends GenericRestClient { }
 const BASE_URL = faker.internet.url();
@@ -317,7 +318,7 @@ describe('GenericRestClient', () => {
         expect(onSuccess).toHaveBeenCalledWith(response);
     });
 
-    it('performs request with custum headers', () => {
+    it('performs request with custom headers', () => {
         const headers = {
             'Authorization': `Barrier ${faker.random.uuid()}`,
         };
@@ -370,4 +371,31 @@ describe('GenericRestClient', () => {
         expect(request.status).toEqual(statusCode);
         expect(onSuccess).toHaveBeenCalledWith(body.map((str: string) => str.trim()));
     });
+
+    it('blocks the request with custom method', () => { 
+        const blockDefer = SyncTasks.Defer<void>();
+
+        class Http extends GenericRestClient {
+            protected _blockRequestUntil() {
+                return blockDefer.promise();
+            }
+        }
+
+        const statusCode = 200;
+        const onSuccess = jasmine.createSpy('onSuccess');
+        const http = new Http(BASE_URL);
+        const path = '/auth';
+
+        http.performApiGet(path)
+            .then(onSuccess);
+
+        let request = jasmine.Ajax.requests.mostRecent();
+        
+        expect(request).toBeUndefined();
+        blockDefer.resolve(void 0);
+
+        request = jasmine.Ajax.requests.mostRecent();
+        request.respondWith({ status: statusCode });
+        expect(onSuccess).toHaveBeenCalled();
+    });
 });

test/SimpleWebRequest.spec.ts

@@ -1,10 +1,22 @@
 import * as faker from 'faker';
-import { SimpleWebRequest } from '../src/SimpleWebRequest';
+import * as _ from 'lodash';
+import { SimpleWebRequest, SimpleWebRequestOptions, WebErrorResponse, WebRequestPriority } from '../src/SimpleWebRequest';
 import { DETAILED_RESPONSE } from './helpers';
+import * as SyncTasks from 'synctasks';
 
 describe('SimpleWebRequest', () => {
-    beforeEach(() => jasmine.Ajax.install());
-    afterEach(() => jasmine.Ajax.uninstall());
+    let catchExceptions = false;
+    const status = 200;
+
+    beforeEach(() => {
+        catchExceptions = SyncTasks.config.catchExceptions;
+        SyncTasks.config.catchExceptions = false;
+        jasmine.Ajax.install();
+    });
+    afterEach(() => {
+        SyncTasks.config.catchExceptions = catchExceptions;
+        jasmine.Ajax.uninstall();
+    });
 
     it('performs GET request', () => {
         const requestOptions = { contentType: 'json' };
@@ -68,6 +80,8 @@ describe('SimpleWebRequest', () => {
 
         expect(request.requestHeaders['X-Requested-With']).toEqual(headers['X-Requested-With']);
         expect(request.requestHeaders['Max-Forwards']).toEqual(headers['Max-Forwards']);
+        
+        request.respondWith({ status });
     });
 
     it('forbids to set Accept header', () => {
@@ -79,7 +93,8 @@ describe('SimpleWebRequest', () => {
 
         expect(
             () => new SimpleWebRequest<string>(url, method, {}, () => headers).start()
-        ).toThrowError(`Don't set Accept with options.headers -- use it with the options.acceptType property`)
+        ).toThrowError(`Don't set Accept with options.headers -- use it with the options.acceptType property`);
+
     })
 
     it('forbids to set Content-Type header', () => {
@@ -88,11 +103,161 @@ describe('SimpleWebRequest', () => {
         };
         const method = 'GET';
         const url = faker.internet.url();
-
         expect(
             () => new SimpleWebRequest<string>(url, method, {}, () => headers).start()
         ).toThrowError(`Don't set Content-Type with options.headers -- use it with the options.contentType property`)
     })
 
+    describe('blocking', () => {
+        let maxRequests = 0;
+
+        beforeEach(() => {
+            maxRequests = SimpleWebRequestOptions.MaxSimultaneousRequests;
+            SimpleWebRequestOptions.MaxSimultaneousRequests = 0;
+            jasmine.clock().install();
+        });
+        afterEach(() => {
+            SimpleWebRequestOptions.MaxSimultaneousRequests = maxRequests; 
+            jasmine.clock().uninstall();
+        });
+
+        it('executes the requests by priority and age', () => {
+            const url = faker.internet.url();
+            const method = 'GET';
+            const onSuccessLow1 = jasmine.createSpy('onSuccessLow1');
+            const onSuccessCritical1 = jasmine.createSpy('onSuccessCritical1');
+            const onSuccessLow2 = jasmine.createSpy('onSuccessLow2');
+            const onSuccessCritical2 = jasmine.createSpy('onSuccessCritical2');
+            const status = 200;
+    
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.Low }).start().then(onSuccessLow1);
+            jasmine.clock().tick(10);
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.Critical }).start().then(onSuccessCritical1);
+            jasmine.clock().tick(10);
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.Low }).start().then(onSuccessLow2);
+            jasmine.clock().tick(10);
+            
+            SimpleWebRequestOptions.MaxSimultaneousRequests = 1;
+            // add a new request to kick the queue
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.Critical }).start().then(onSuccessCritical2);
+            
+            // only one is executed
+            expect(jasmine.Ajax.requests.count()).toBe(1);
+            jasmine.Ajax.requests.mostRecent().respondWith({status});
+            // they're executed in correct order
+            expect(onSuccessCritical1).toHaveBeenCalled();
+
+            jasmine.Ajax.requests.mostRecent().respondWith({status});
+            expect(onSuccessCritical2).toHaveBeenCalled();
+
+            jasmine.Ajax.requests.mostRecent().respondWith({status});
+            expect(onSuccessLow1).toHaveBeenCalled();
+
+            jasmine.Ajax.requests.mostRecent().respondWith({status});
+            expect(onSuccessLow2).toHaveBeenCalled();
+        });
+    
+        it('blocks the request with custom promise', () => {
+            SimpleWebRequestOptions.MaxSimultaneousRequests = 1;
+            const url = faker.internet.url();
+            const method = 'GET';
+            const blockDefer = SyncTasks.Defer<void>();
+            const onSuccess1 = jasmine.createSpy('onSuccess1');
+            new SimpleWebRequest<string>(url, method,{}, undefined, () => blockDefer.promise()).start().then(onSuccess1);
+
+            expect(jasmine.Ajax.requests.count()).toBe(0);
+            blockDefer.resolve(void 0);
+
+            const request = jasmine.Ajax.requests.mostRecent();
+            request.respondWith({ status: 200 });
+            expect(onSuccess1).toHaveBeenCalled();
+        });
+    
+        it('after the request is unblocked, it\'s returned to the queue with correct priority', () => {
+            const url = faker.internet.url();
+            const method = 'GET';
+            const blockDefer = SyncTasks.Defer<void>();
+            const onSuccessHigh = jasmine.createSpy('onSuccessHigh');
+            const onSuccessLow = jasmine.createSpy('onSuccessLow');
+            const onSuccessCritical = jasmine.createSpy('onSuccessCritical');
+
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.High }, undefined, () => blockDefer.promise()).start().then(onSuccessHigh);
+            jasmine.clock().tick(10);
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.Low }).start().then(onSuccessLow);
+            jasmine.clock().tick(10);
+
+            SimpleWebRequestOptions.MaxSimultaneousRequests = 1;
+            // add a new request to kick the queue
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.Critical }).start().then(onSuccessCritical);
+            
+            // unblock the request
+            blockDefer.resolve(void 0);
+
+            jasmine.Ajax.requests.mostRecent().respondWith({ status: 200 });
+            // first the critical one gets sent
+            expect(onSuccessCritical).toHaveBeenCalled();
+
+            // then the high, which was returned to the queue at after getting unblocked
+            jasmine.Ajax.requests.mostRecent().respondWith({ status: 200 });
+            expect(onSuccessHigh).toHaveBeenCalled();
+
+            // and the low priority one gets sent last 
+            jasmine.Ajax.requests.mostRecent().respondWith({ status: 200 });
+            expect(onSuccessLow).toHaveBeenCalled();
+        });
+        
+        it('checks the blocked function again, once the request is on top of the queue', () => {
+            const url = faker.internet.url();
+            const method = 'GET';
+            const blockDefer = SyncTasks.Defer<void>();
+            const onSuccessCritical = jasmine.createSpy('onSuccessCritical');
+            const onSuccessHigh = jasmine.createSpy('onSuccessHigh');
+            const onSuccessHigh2 = jasmine.createSpy('onSuccessHigh2');
+            const blockSpy = jasmine.createSpy('blockSpy').and.callFake(() => blockDefer.promise());
+
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.Critical }, undefined, blockSpy).start().then(onSuccessCritical);
+            jasmine.clock().tick(10);
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.High }).start().then(onSuccessHigh);
+            jasmine.clock().tick(10);
+
+            SimpleWebRequestOptions.MaxSimultaneousRequests = 1;
+            // add a new request to kick the queue
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.High }).start().then(onSuccessHigh2);
+            
+            expect(blockSpy).toHaveBeenCalled();
+
+            jasmine.Ajax.requests.mostRecent().respondWith({ status: 200 });
+            expect(onSuccessHigh).toHaveBeenCalled();
+
+            // unblock the request, it will go back to the queue after the currently executed request
+            blockDefer.resolve(void 0);
+
+            jasmine.Ajax.requests.mostRecent().respondWith({ status: 200 });
+            expect(onSuccessHigh2).toHaveBeenCalled();
+
+            // check if the request at the top of the queue got called again
+            expect(blockSpy).toHaveBeenCalledTimes(2);
+
+            jasmine.Ajax.requests.mostRecent().respondWith({ status: 200 });
+            expect(onSuccessCritical).toHaveBeenCalled();
+        });
+
+        it('fails the request, if the blocking promise rejects', done => {
+            SimpleWebRequestOptions.MaxSimultaneousRequests = 1;
+            const url = faker.internet.url();
+            const method = 'GET';
+            const blockDefer = SyncTasks.Defer<void>();
+            const errorString = 'Terrible error';
+            new SimpleWebRequest<string>(url, method, { priority: WebRequestPriority.Critical }, undefined, () => blockDefer.promise()).start()
+            .then(() => fail(), (err: WebErrorResponse) => {
+                expect(err.statusCode).toBe(0);
+                expect(err.statusText).toBe('_blockRequestUntil rejected: ' + errorString);
+                done();
+            });
+            
+            blockDefer.reject(errorString);
+        });
+    });
+
     // @TODO Add more unit tests
 });