Fix an assertion if a request with timeout receives 2 callbacks from the Browser/OS. (#37)

This resulted in _respond being called twice, which (if the correct retry policy was applied) would result in the request being enqueued twice & this two timeout timers were setup.

Author: berickson1

package.json

@@ -1,6 +1,6 @@
 {
   "name": "simplerestclients",
-  "version": "0.2.0",
+  "version": "0.2.1",
   "description": "A library of components for accessing RESTful services with javascript/typescript.",
   "author": "David de Regt <David.de.Regt@microsoft.com>",
   "scripts": {

src/SimpleWebRequest.ts

@@ -173,6 +173,9 @@ const enum FeatureSupportStatus {
 // List of pending requests, sorted from most important to least important (numerically descending)
 let requestQueue: SimpleWebRequestBase[] = [];
 
+// List of requests blocked on _blockUNtil promises
+let blockedList: SimpleWebRequestBase[] = [];
+
 // List of executing (non-finished) requests -- only to keep track of number of requests to compare to the max
 let executingList: SimpleWebRequestBase[] = [];
 
@@ -215,8 +218,11 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
     protected static checkQueueProcessing() {
         while (requestQueue.length > 0 && executingList.length < SimpleWebRequestOptions.MaxSimultaneousRequests) {
             const req = requestQueue.shift()!!!;
+            blockedList.push(req);
             const blockPromise = (req._blockRequestUntil && req._blockRequestUntil()) || SyncTasks.Resolved();
-            blockPromise.then(() => {
+            blockPromise.finally(() => {
+                _.remove(blockedList, req);
+            }).then(() => {
                 if (executingList.length < SimpleWebRequestOptions.MaxSimultaneousRequests) {
                     executingList.push(req);
                     req._fire();
@@ -231,12 +237,10 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
     }
 
     protected _removeFromQueue(): void {
-        // Pull it out of whichever queue it's sitting in
-        if (this._xhr) {
-            _.pull(executingList, this);
-        } else {
-            _.pull(requestQueue, this);
-        }
+        // Only pull from request queue and executing queue here - pulling from the blocked queue can result in requests
+        // being queued up multiple times if _respond fires more than once (it shouldn't, but does happen in the wild)
+        _.remove(executingList, this);
+        _.remove(requestQueue, this);
     }
 
     protected _assertAndClean(expression: any, message: string): void {
@@ -526,6 +530,11 @@ export abstract class SimpleWebRequestBase<TOptions extends WebRequestOptions =
             return;
         }
 
+        // Check if the current queues, if the request is already in there, nothing to enqueue
+        if (_.includes(executingList, this) || _.includes(blockedList, this) || _.includes(requestQueue, this)) {
+            return;
+        }
+
         // Throw it on the queue
         const index = _.findIndex(requestQueue, request =>
             // find a request with the same priority, but newer

test/GenericRestClient.spec.ts

@@ -1,4 +1,5 @@
 import * as faker from 'faker';
+import { ErrorHandlingType, SimpleWebRequestBase, WebErrorResponse } from '../src/SimpleWebRequest';
 import { GenericRestClient, ApiCallOptions } from '../src/GenericRestClient';
 import { DETAILED_RESPONSE, REQUEST_OPTIONS } from './helpers';
 import * as SyncTasks from 'synctasks';
@@ -8,6 +9,22 @@ const BASE_URL = faker.internet.url();
 const http = new RestClient(BASE_URL);
 
 describe('GenericRestClient', () => {
+    beforeAll(() => {
+        jasmine.Ajax.install();
+        // Run an initial request to finish feature detection - this is needed so we can directly call onLoad
+        const statusCode = 200;
+        const onSuccess = jasmine.createSpy('onSuccess');
+        const path = '/auth';
+
+        http.performApiGet(path)
+            .then(onSuccess);
+
+        const request = jasmine.Ajax.requests.mostRecent();
+        request.respondWith({ status: statusCode });
+        expect(onSuccess).toHaveBeenCalled();
+        jasmine.Ajax.uninstall();
+    });
+
     beforeEach(() => jasmine.Ajax.install());
     afterEach(() => jasmine.Ajax.uninstall());
 
@@ -372,7 +389,7 @@ describe('GenericRestClient', () => {
         expect(onSuccess).toHaveBeenCalledWith(body.map((str: string) => str.trim()));
     });
 
-    it('blocks the request with custom method', () => { 
+    it('blocks the request with custom method', () => {
         const blockDefer = SyncTasks.Defer<void>();
 
         class Http extends GenericRestClient {
@@ -398,4 +415,108 @@ describe('GenericRestClient', () => {
         request.respondWith({ status: statusCode });
         expect(onSuccess).toHaveBeenCalled();
     });
+
+    it('aborting request after failure w/retry', () => {
+        let blockDefer = SyncTasks.Defer<void>();
+
+        class Http extends GenericRestClient {
+            constructor(endpointUrl: string) {
+                super(endpointUrl);
+                this._defaultOptions.customErrorHandler = this._customErrorHandler;
+                this._defaultOptions.timeout = 1;
+            }
+            protected _blockRequestUntil() {
+                return blockDefer.promise();
+            }
+
+            protected _customErrorHandler = (webRequest: SimpleWebRequestBase, errorResponse: WebErrorResponse) => {
+                if (errorResponse.canceled) {
+                    return ErrorHandlingType.DoNotRetry;
+                }
+                return ErrorHandlingType.RetryUncountedImmediately;
+            }
+        }
+
+        const statusCode = 400;
+        const onSuccess = jasmine.createSpy('onSuccess');
+        const onFailure = jasmine.createSpy('onFailure');
+        const http = new Http(BASE_URL);
+        const path = '/auth';
+
+        const req = http.performApiGet(path)
+            .then(onSuccess)
+            .catch(onFailure);
+
+        blockDefer.resolve(void 0);
+        const request1 = jasmine.Ajax.requests.mostRecent();
+        
+        // Reset blockuntil so retries may block
+        blockDefer = SyncTasks.Defer<void>();
+
+        request1.respondWith({ status: statusCode });
+        expect(onSuccess).not.toHaveBeenCalled();
+        expect(onFailure).not.toHaveBeenCalled();
+
+        // Calls abort function
+        req.cancel();
+
+        expect(onSuccess).not.toHaveBeenCalled();
+        expect(onFailure).toHaveBeenCalled();
+    });
+
+    describe('Timing related tests' , () => {
+        beforeEach(() => {
+            jasmine.clock().install();
+        });
+
+        afterEach(() => {
+            jasmine.clock().uninstall();
+        });
+
+        it('failed request with retry handles multiple _respond calls', () => {
+            let blockDefer = SyncTasks.Defer<void>();
+
+            class Http extends GenericRestClient {
+                constructor(endpointUrl: string) {
+                    super(endpointUrl);
+                    this._defaultOptions.customErrorHandler = this._customErrorHandler;
+                    this._defaultOptions.timeout = 1;
+                }
+                protected _blockRequestUntil() {
+                    return blockDefer.promise();
+                }
+
+                protected _customErrorHandler = () => {
+                    return ErrorHandlingType.RetryUncountedImmediately;
+                }
+            }
+
+            const statusCode = 400;
+            const onSuccess = jasmine.createSpy('onSuccess');
+            const http = new Http(BASE_URL);
+            const path = '/auth';
+
+            http.performApiGet(path)
+                .then(onSuccess);
+
+            blockDefer.resolve(void 0);
+            const request1 = jasmine.Ajax.requests.mostRecent();
+            
+            // Reset blockuntil so retries may block
+            blockDefer = SyncTasks.Defer<void>();
+
+            // Store this so we're able to emulate double-request callbacks
+            const onloadToCall = request1.onload as any;
+            request1.respondWith({ status: statusCode });
+            onloadToCall(undefined);
+            expect(onSuccess).not.toHaveBeenCalled();
+            blockDefer.resolve(void 0);
+
+            jasmine.clock().tick(100);
+
+            const request2 = jasmine.Ajax.requests.mostRecent();
+            request2.respondWith({ status: 200 });
+            expect(onSuccess).toHaveBeenCalled();
+        });
+    });
 });